| Message ID | 20191101123158.28847-1-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,RFC] bridge: Allow manual notifications about interfaces' updates. | expand |
On 1 Nov 2019, at 13:31, Ilya Maximets wrote: > Sometimes interface updates could happen in a way ifnotifier is not > able to catch. For example some heavy operations (device reset) in > netdev-dpdk could require re-applying of the bridge configuration. > > For this purpose new function 'bridge_report_if_update' introduced. > It could be called directly by the code that aware about changes. > > Since the calls could go from the different thread contexts we need > to protect the sequence number itself from updates before allocation > and after destruction. Using the same seq mutex for this purpose to > not introduce another one. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > > Sending this as RFC that might be useful in context of the following > patch: > https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html > > It will allow us to not introduce heavy dpdk notifier just to update > one sequence number on RTE_ETH_EVENT_INTR_RESET events. We could also > use it to report other changes from DPDK, e.g. LSC interrupts. > > vswitchd/bridge.c | 26 +++++++++++++++++++++++++- > vswitchd/bridge.h | 2 ++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 9095ebf5d..68239783f 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -252,6 +252,7 @@ static long long int aa_refresh_timer = LLONG_MIN; > static struct if_notifier *ifnotifier; > static struct seq *ifaces_changed; > static uint64_t last_ifaces_changed; > +static bool ifaces_updates_enabled = false; > > /* Default/min/max packet-in queue sizes towards the controllers. */ > #define BRIDGE_CONTROLLER_PACKET_QUEUE_DEFAULT_SIZE 100 > @@ -403,7 +404,19 @@ bridge_init_ofproto(const struct > ovsrec_open_vswitch *cfg) > static void > if_change_cb(void *aux OVS_UNUSED) > { > - seq_change(ifaces_changed); > + seq_lock(); > + if (ifaces_updates_enabled) { > + seq_change_protected(ifaces_changed); > + } > + seq_unlock(); > +} > + > +static void > +if_updates_enable(bool enable) > +{ > + seq_lock(); > + ifaces_updates_enabled = enable; > + seq_unlock(); > } > > static bool > @@ -422,6 +435,15 @@ if_notifier_changed(struct if_notifier *notifier > OVS_UNUSED) > > /* Public functions. */ > > +/* Raports that some interfaces were changed from the outside of OVS. > + * Could be used for catching events that could not be tracked by > + * if_notifier, e.g. port updates in netdev-dpdk. */ How would I call this directly from the netdev-dpdk code? The later is in /lib which should this not be isolated from /vswitchd, i.e. /vswitchd can call into /lib function but not the other way around? > +void > +bridge_report_if_update(void) > +{ > + if_change_cb(NULL); > +} > + > /* Initializes the bridge module, configuring it to obtain its > configuration > * from an OVSDB server accessed over 'remote', which should be a > string in a > * form acceptable to ovsdb_idl_create(). */ > @@ -529,11 +551,13 @@ bridge_init(const char *remote) > ifaces_changed = seq_create(); > last_ifaces_changed = seq_read(ifaces_changed); > ifnotifier = if_notifier_create(if_change_cb, NULL); > + if_updates_enable(true); > } > > void > bridge_exit(bool delete_datapath) > { > + if_updates_enable(false); > if_notifier_destroy(ifnotifier); > seq_destroy(ifaces_changed); > > diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h > index 8b2fce451..69a3db2bc 100644 > --- a/vswitchd/bridge.h > +++ b/vswitchd/bridge.h > @@ -28,4 +28,6 @@ void bridge_wait(void); > > void bridge_get_memory_usage(struct simap *usage); > > +void bridge_report_if_update(void); > + > #endif /* bridge.h */ > -- > 2.17.1
On 04.11.2019 12:04, Eelco Chaudron wrote: > > > On 1 Nov 2019, at 13:31, Ilya Maximets wrote: > >> Sometimes interface updates could happen in a way ifnotifier is not >> able to catch. For example some heavy operations (device reset) in >> netdev-dpdk could require re-applying of the bridge configuration. >> >> For this purpose new function 'bridge_report_if_update' introduced. >> It could be called directly by the code that aware about changes. >> >> Since the calls could go from the different thread contexts we need >> to protect the sequence number itself from updates before allocation >> and after destruction. Using the same seq mutex for this purpose to >> not introduce another one. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> >> Sending this as RFC that might be useful in context of the following patch: >> https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html >> >> It will allow us to not introduce heavy dpdk notifier just to update >> one sequence number on RTE_ETH_EVENT_INTR_RESET events. We could also >> use it to report other changes from DPDK, e.g. LSC interrupts. >> >> vswitchd/bridge.c | 26 +++++++++++++++++++++++++- >> vswitchd/bridge.h | 2 ++ >> 2 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> index 9095ebf5d..68239783f 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -252,6 +252,7 @@ static long long int aa_refresh_timer = LLONG_MIN; >> static struct if_notifier *ifnotifier; >> static struct seq *ifaces_changed; >> static uint64_t last_ifaces_changed; >> +static bool ifaces_updates_enabled = false; >> >> /* Default/min/max packet-in queue sizes towards the controllers. */ >> #define BRIDGE_CONTROLLER_PACKET_QUEUE_DEFAULT_SIZE 100 >> @@ -403,7 +404,19 @@ bridge_init_ofproto(const struct ovsrec_open_vswitch *cfg) >> static void >> if_change_cb(void *aux OVS_UNUSED) >> { >> - seq_change(ifaces_changed); >> + seq_lock(); >> + if (ifaces_updates_enabled) { >> + seq_change_protected(ifaces_changed); >> + } >> + seq_unlock(); >> +} >> + >> +static void >> +if_updates_enable(bool enable) >> +{ >> + seq_lock(); >> + ifaces_updates_enabled = enable; >> + seq_unlock(); >> } >> >> static bool >> @@ -422,6 +435,15 @@ if_notifier_changed(struct if_notifier *notifier OVS_UNUSED) >> >> /* Public functions. */ >> >> +/* Raports that some interfaces were changed from the outside of OVS. >> + * Could be used for catching events that could not be tracked by >> + * if_notifier, e.g. port updates in netdev-dpdk. */ > > How would I call this directly from the netdev-dpdk code? The later is in /lib which should this not be isolated from /vswitchd, i.e. /vswitchd can call into /lib function but not the other way around? Yes, you're right. I'll move the code to 'lib'. Best regards, Ilya Maximets.
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 9095ebf5d..68239783f 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -252,6 +252,7 @@ static long long int aa_refresh_timer = LLONG_MIN; static struct if_notifier *ifnotifier; static struct seq *ifaces_changed; static uint64_t last_ifaces_changed; +static bool ifaces_updates_enabled = false; /* Default/min/max packet-in queue sizes towards the controllers. */ #define BRIDGE_CONTROLLER_PACKET_QUEUE_DEFAULT_SIZE 100 @@ -403,7 +404,19 @@ bridge_init_ofproto(const struct ovsrec_open_vswitch *cfg) static void if_change_cb(void *aux OVS_UNUSED) { - seq_change(ifaces_changed); + seq_lock(); + if (ifaces_updates_enabled) { + seq_change_protected(ifaces_changed); + } + seq_unlock(); +} + +static void +if_updates_enable(bool enable) +{ + seq_lock(); + ifaces_updates_enabled = enable; + seq_unlock(); } static bool @@ -422,6 +435,15 @@ if_notifier_changed(struct if_notifier *notifier OVS_UNUSED) /* Public functions. */ +/* Raports that some interfaces were changed from the outside of OVS. + * Could be used for catching events that could not be tracked by + * if_notifier, e.g. port updates in netdev-dpdk. */ +void +bridge_report_if_update(void) +{ + if_change_cb(NULL); +} + /* Initializes the bridge module, configuring it to obtain its configuration * from an OVSDB server accessed over 'remote', which should be a string in a * form acceptable to ovsdb_idl_create(). */ @@ -529,11 +551,13 @@ bridge_init(const char *remote) ifaces_changed = seq_create(); last_ifaces_changed = seq_read(ifaces_changed); ifnotifier = if_notifier_create(if_change_cb, NULL); + if_updates_enable(true); } void bridge_exit(bool delete_datapath) { + if_updates_enable(false); if_notifier_destroy(ifnotifier); seq_destroy(ifaces_changed); diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h index 8b2fce451..69a3db2bc 100644 --- a/vswitchd/bridge.h +++ b/vswitchd/bridge.h @@ -28,4 +28,6 @@ void bridge_wait(void); void bridge_get_memory_usage(struct simap *usage); +void bridge_report_if_update(void); + #endif /* bridge.h */
Sometimes interface updates could happen in a way ifnotifier is not able to catch. For example some heavy operations (device reset) in netdev-dpdk could require re-applying of the bridge configuration. For this purpose new function 'bridge_report_if_update' introduced. It could be called directly by the code that aware about changes. Since the calls could go from the different thread contexts we need to protect the sequence number itself from updates before allocation and after destruction. Using the same seq mutex for this purpose to not introduce another one. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- Sending this as RFC that might be useful in context of the following patch: https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html It will allow us to not introduce heavy dpdk notifier just to update one sequence number on RTE_ETH_EVENT_INTR_RESET events. We could also use it to report other changes from DPDK, e.g. LSC interrupts. vswitchd/bridge.c | 26 +++++++++++++++++++++++++- vswitchd/bridge.h | 2 ++ 2 files changed, 27 insertions(+), 1 deletion(-)