| Message ID | 20191104132234.11704-1-i.maximets@ovn.org |
|---|---|
| State | RFC |
| Headers | show |
| Series | [ovs-dev,RFC,v2] bridge: Allow manual notifications about interfaces' updates. | expand |
On 4 Nov 2019, at 14:22, 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 manual notifier introduced. Its function > 'if_notifier_manual_report()' could be called directly by the code > that aware about changes. This new notifier is thread safe. > > 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. > > Version 2: > * Main functions moved from bridge.c to the new > lib/if-notifier-manual.c > to allow using from other lib code. > > lib/automake.mk | 3 ++- > lib/if-notifier-manual.c | 55 > ++++++++++++++++++++++++++++++++++++++++ > lib/if-notifier.h | 7 +++++ > vswitchd/bridge.c | 2 ++ > 4 files changed, 66 insertions(+), 1 deletion(-) > create mode 100644 lib/if-notifier-manual.c > > diff --git a/lib/automake.mk b/lib/automake.mk > index 17b36b43d..ebf714501 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -111,6 +111,8 @@ lib_libopenvswitch_la_SOURCES = \ > lib/hmapx.h \ > lib/id-pool.c \ > lib/id-pool.h \ > + lib/if-notifier-manual.c \ > + lib/if-notifier.h \ > lib/ipf.c \ > lib/ipf.h \ > lib/jhash.c \ > @@ -394,7 +396,6 @@ lib_libopenvswitch_la_SOURCES += \ > lib/dpif-netlink-rtnl.c \ > lib/dpif-netlink-rtnl.h \ > lib/if-notifier.c \ > - lib/if-notifier.h \ > lib/netdev-linux.c \ > lib/netdev-linux.h \ > lib/netdev-linux-private.h \ > diff --git a/lib/if-notifier-manual.c b/lib/if-notifier-manual.c > new file mode 100644 > index 000000000..72d6d8b8d > --- /dev/null > +++ b/lib/if-notifier-manual.c > @@ -0,0 +1,55 @@ > +/* > + * Copyright (c) 2019 Ilya Maximets <i.maximets@ovn.org>. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, > software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions > and > + * limitations under the License. > + */ > + > +#include <config.h> > +#include "openvswitch/compiler.h" > +#include "openvswitch/thread.h" > +#include "openvswitch/util.h" > +#include "if-notifier.h" > + > +/* Implementation of a manual interface notifier. > + * > + * Intended for catching interface events that could not be tracked > by > + * OS specific interface notifiers, e.g. iface updates in > netdev-dpdk. > + * For that purpose 'if_notifier_manual_report()' should be called > directly > + * by the code that aware of interface changes. > + * > + * Thread-safety > + * ============= > + * This notifier is thread-safe in terms of calling its functions > from > + * different thread contexts, however if the callback passed to > + * 'if_notifier_manual_set_cb' is used by some other code (i.e. by OS > + * specific notifiers) it must be thread-safe itself. > + */ > + > +static struct ovs_mutex manual_notifier_mutex = > OVS_MUTEX_INITIALIZER; > +static if_notify_func *notify OVS_GUARDED_BY(manual_notifier_mutex) = > NULL; > + > +void if_notifier_manual_set_cb(if_notify_func *cb) > +{ > + ovs_mutex_lock(&manual_notifier_mutex); > + notify = cb; > + ovs_mutex_unlock(&manual_notifier_mutex); > +} > + > +void if_notifier_manual_report(void) > +{ > + ovs_mutex_lock(&manual_notifier_mutex); > + if (notify) { > + notify(NULL); > + } > + ovs_mutex_unlock(&manual_notifier_mutex); > +} > diff --git a/lib/if-notifier.h b/lib/if-notifier.h > index 259138f70..dae852e9f 100644 > --- a/lib/if-notifier.h > +++ b/lib/if-notifier.h > @@ -27,4 +27,11 @@ void if_notifier_destroy(struct if_notifier *); > void if_notifier_run(void); > void if_notifier_wait(void); > > +/* Below functions are reserved for if-notifier-manual , i.e. for > manual > + * notifications from the OVS code. > + * Must not be implemented by OS specific notifiers. */ > + > +void if_notifier_manual_set_cb(if_notify_func *); > +void if_notifier_manual_report(void); > + > #endif /* if-notifier.h */ > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 9095ebf5d..37131712d 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -529,11 +529,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_notifier_manual_set_cb(if_change_cb); > } > > void > bridge_exit(bool delete_datapath) > { > + if_notifier_manual_set_cb(NULL); > if_notifier_destroy(ifnotifier); > seq_destroy(ifaces_changed); > > -- > 2.17.1 This looks good to me, please make it an official patch ;) //Eelco
On Mon, Nov 04, 2019 at 02:22:34PM +0100, 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 manual notifier introduced. Its function > 'if_notifier_manual_report()' could be called directly by the code > that aware about changes. This new notifier is thread safe. > > 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. I *think* I understand what's going on here. It's that the proposed dpdk-notifier (https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364154.html) takes more locks, etc., so it's going to be slow. This one doesn't so it's cheaper. This one, however, will only work if the OVS code that makes the device changes calls into it, whereas the other one gets notified by DPDK itself. I think that this one could avoid introducing a mutex by using an atomic pointer, but I don't know whether that's worthwhile.
On 05.11.2019 20:20, Ben Pfaff wrote: > On Mon, Nov 04, 2019 at 02:22:34PM +0100, 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 manual notifier introduced. Its function >> 'if_notifier_manual_report()' could be called directly by the code >> that aware about changes. This new notifier is thread safe. >> >> 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. > > I *think* I understand what's going on here. It's that the proposed > dpdk-notifier > (https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364154.html) > takes more locks, etc., so it's going to be slow. This one doesn't so > it's cheaper. This one, however, will only work if the OVS code that > makes the device changes calls into it, whereas the other one gets > notified by DPDK itself. Both implementations doesn't receive notifications from DPDK itself. dpdk-notifier is just a more complex variant of the same thing with dynamic allocations and list of notifiers. In practice, the only way to trigger it is to call dpdk_notifierr_report_link() from the OVS code. The part that really receives DPDK events is 'dpdk_eth_event_callback()' from https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html There is a possibility to make everything super right (not sure). This should look like this: * Allow more than one type of notifiers. i.e. introduce ifnotifier-provider.h, make a common interface in ifnotifier.{c,h}. Move code of the existing notifiers to ifnotifier-{linux,bsd}.{c.h} and make them both implement ifnotifier with type='system'. * Introduce dpdk_notifier (analogue of rtnetlink_notifier) that will subscribe itself to DPDK events with rte_eth_dev_callback_register() and receive actual DPDK events via callback. * Introduce ifnotifier-dpdk.{c,h} that will register itself as ifnotifier with type='dpdk'. Subscribe ifnotifier-dpdk to dpdk_notifier with dpdk_notifier_create(). * bridge.c should subscribe itself to all registered types of ifnotifiers to increase 'ifaces_changed' sequence number. * netdev-dpdk.c should subscribe to dpdk_notifier to receive DPDK events and perform required actions on devices. * Something else to resolve issues with DPDK intialization. (we can't register dpdk callbacks before dpdk initialization that could happen way after bridge initialization) I'm not sure if it's really possible to properly implement, but it looks like an over-engineering anyway. BTW, I'm not sure if any of our solutions are good enough. > > I think that this one could avoid introducing a mutex by using an atomic > pointer, but I don't know whether that's worthwhile. > The case is that we need to be sure that 'ifaces_changed' sequence number will not be destroyed while (before, actually) we're using it. Main thread Notification thread create seq set cb <-- func notification appeared func = read cb func() set cb <-- NULL destroy seq seq_change() // change of destroyed seq will lead to crash. So, the mutex ensures that callback is still registered while we're executing it. Best regards, Ilya Maximets.
On Tue, Nov 05, 2019 at 09:40:26PM +0100, Ilya Maximets wrote: > On 05.11.2019 20:20, Ben Pfaff wrote: > > On Mon, Nov 04, 2019 at 02:22:34PM +0100, 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 manual notifier introduced. Its function > > > 'if_notifier_manual_report()' could be called directly by the code > > > that aware about changes. This new notifier is thread safe. > > > > > > 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. > > > > I *think* I understand what's going on here. It's that the proposed > > dpdk-notifier > > (https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364154.html) > > takes more locks, etc., so it's going to be slow. This one doesn't so > > it's cheaper. This one, however, will only work if the OVS code that > > makes the device changes calls into it, whereas the other one gets > > notified by DPDK itself. > > Both implementations doesn't receive notifications from DPDK itself. > dpdk-notifier is just a more complex variant of the same thing with > dynamic allocations and list of notifiers. In practice, the only > way to trigger it is to call dpdk_notifierr_report_link() from the > OVS code. > > The part that really receives DPDK events is 'dpdk_eth_event_callback()' > from https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html > > There is a possibility to make everything super right (not sure). > This should look like this: > > * Allow more than one type of notifiers. i.e. introduce > ifnotifier-provider.h, make a common interface in ifnotifier.{c,h}. > Move code of the existing notifiers to ifnotifier-{linux,bsd}.{c.h} > and make them both implement ifnotifier with type='system'. > > * Introduce dpdk_notifier (analogue of rtnetlink_notifier) that will > subscribe itself to DPDK events with rte_eth_dev_callback_register() > and receive actual DPDK events via callback. > > * Introduce ifnotifier-dpdk.{c,h} that will register itself as ifnotifier > with type='dpdk'. Subscribe ifnotifier-dpdk to dpdk_notifier with > dpdk_notifier_create(). > > * bridge.c should subscribe itself to all registered types of ifnotifiers > to increase 'ifaces_changed' sequence number. > > * netdev-dpdk.c should subscribe to dpdk_notifier to receive DPDK > events and perform required actions on devices. > > * Something else to resolve issues with DPDK intialization. > (we can't register dpdk callbacks before dpdk initialization that > could happen way after bridge initialization) > > I'm not sure if it's really possible to properly implement, but it looks > like an over-engineering anyway. > > BTW, I'm not sure if any of our solutions are good enough. OK. Thanks for all the details. Given that we have three solutions, of which it's possible none may be "good enough", I favor the simplest (the "manual" one). > > I think that this one could avoid introducing a mutex by using an atomic > > pointer, but I don't know whether that's worthwhile. > > > > The case is that we need to be sure that 'ifaces_changed' sequence number > will not be destroyed while (before, actually) we're using it. > > Main thread Notification thread > > create seq > set cb <-- func > notification appeared > func = read cb > func() > set cb <-- NULL > destroy seq > seq_change() // change of destroyed seq will lead to crash. > > > So, the mutex ensures that callback is still registered while we're > executing it. Ah, OK. That makes sense.
On 05.11.2019 22:23, Ben Pfaff wrote: > On Tue, Nov 05, 2019 at 09:40:26PM +0100, Ilya Maximets wrote: >> On 05.11.2019 20:20, Ben Pfaff wrote: >>> On Mon, Nov 04, 2019 at 02:22:34PM +0100, 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 manual notifier introduced. Its function >>>> 'if_notifier_manual_report()' could be called directly by the code >>>> that aware about changes. This new notifier is thread safe. >>>> >>>> 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. >>> >>> I *think* I understand what's going on here. It's that the proposed >>> dpdk-notifier >>> (https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364154.html) >>> takes more locks, etc., so it's going to be slow. This one doesn't so >>> it's cheaper. This one, however, will only work if the OVS code that >>> makes the device changes calls into it, whereas the other one gets >>> notified by DPDK itself. >> >> Both implementations doesn't receive notifications from DPDK itself. >> dpdk-notifier is just a more complex variant of the same thing with >> dynamic allocations and list of notifiers. In practice, the only >> way to trigger it is to call dpdk_notifierr_report_link() from the >> OVS code. >> >> The part that really receives DPDK events is 'dpdk_eth_event_callback()' >> from https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364155.html >> >> There is a possibility to make everything super right (not sure). >> This should look like this: >> >> * Allow more than one type of notifiers. i.e. introduce >> ifnotifier-provider.h, make a common interface in ifnotifier.{c,h}. >> Move code of the existing notifiers to ifnotifier-{linux,bsd}.{c.h} >> and make them both implement ifnotifier with type='system'. >> >> * Introduce dpdk_notifier (analogue of rtnetlink_notifier) that will >> subscribe itself to DPDK events with rte_eth_dev_callback_register() >> and receive actual DPDK events via callback. >> >> * Introduce ifnotifier-dpdk.{c,h} that will register itself as ifnotifier >> with type='dpdk'. Subscribe ifnotifier-dpdk to dpdk_notifier with >> dpdk_notifier_create(). >> >> * bridge.c should subscribe itself to all registered types of ifnotifiers >> to increase 'ifaces_changed' sequence number. >> >> * netdev-dpdk.c should subscribe to dpdk_notifier to receive DPDK >> events and perform required actions on devices. >> >> * Something else to resolve issues with DPDK intialization. >> (we can't register dpdk callbacks before dpdk initialization that >> could happen way after bridge initialization) >> >> I'm not sure if it's really possible to properly implement, but it looks >> like an over-engineering anyway. >> >> BTW, I'm not sure if any of our solutions are good enough. > > OK. Thanks for all the details. > > Given that we have three solutions, of which it's possible none may be > "good enough", I favor the simplest (the "manual" one). OK. Same for me. I've sent an official non-RFC patch here: https://mail.openvswitch.org/pipermail/ovs-dev/2019-November/364368.html It's identical to this RFCv2. > >>> I think that this one could avoid introducing a mutex by using an atomic >>> pointer, but I don't know whether that's worthwhile. >>> >> >> The case is that we need to be sure that 'ifaces_changed' sequence number >> will not be destroyed while (before, actually) we're using it. >> >> Main thread Notification thread >> >> create seq >> set cb <-- func >> notification appeared >> func = read cb >> func() >> set cb <-- NULL >> destroy seq >> seq_change() // change of destroyed seq will lead to crash. >> >> >> So, the mutex ensures that callback is still registered while we're >> executing it. > > Ah, OK. That makes sense. >
diff --git a/lib/automake.mk b/lib/automake.mk index 17b36b43d..ebf714501 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -111,6 +111,8 @@ lib_libopenvswitch_la_SOURCES = \ lib/hmapx.h \ lib/id-pool.c \ lib/id-pool.h \ + lib/if-notifier-manual.c \ + lib/if-notifier.h \ lib/ipf.c \ lib/ipf.h \ lib/jhash.c \ @@ -394,7 +396,6 @@ lib_libopenvswitch_la_SOURCES += \ lib/dpif-netlink-rtnl.c \ lib/dpif-netlink-rtnl.h \ lib/if-notifier.c \ - lib/if-notifier.h \ lib/netdev-linux.c \ lib/netdev-linux.h \ lib/netdev-linux-private.h \ diff --git a/lib/if-notifier-manual.c b/lib/if-notifier-manual.c new file mode 100644 index 000000000..72d6d8b8d --- /dev/null +++ b/lib/if-notifier-manual.c @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2019 Ilya Maximets <i.maximets@ovn.org>. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> +#include "openvswitch/compiler.h" +#include "openvswitch/thread.h" +#include "openvswitch/util.h" +#include "if-notifier.h" + +/* Implementation of a manual interface notifier. + * + * Intended for catching interface events that could not be tracked by + * OS specific interface notifiers, e.g. iface updates in netdev-dpdk. + * For that purpose 'if_notifier_manual_report()' should be called directly + * by the code that aware of interface changes. + * + * Thread-safety + * ============= + * This notifier is thread-safe in terms of calling its functions from + * different thread contexts, however if the callback passed to + * 'if_notifier_manual_set_cb' is used by some other code (i.e. by OS + * specific notifiers) it must be thread-safe itself. + */ + +static struct ovs_mutex manual_notifier_mutex = OVS_MUTEX_INITIALIZER; +static if_notify_func *notify OVS_GUARDED_BY(manual_notifier_mutex) = NULL; + +void if_notifier_manual_set_cb(if_notify_func *cb) +{ + ovs_mutex_lock(&manual_notifier_mutex); + notify = cb; + ovs_mutex_unlock(&manual_notifier_mutex); +} + +void if_notifier_manual_report(void) +{ + ovs_mutex_lock(&manual_notifier_mutex); + if (notify) { + notify(NULL); + } + ovs_mutex_unlock(&manual_notifier_mutex); +} diff --git a/lib/if-notifier.h b/lib/if-notifier.h index 259138f70..dae852e9f 100644 --- a/lib/if-notifier.h +++ b/lib/if-notifier.h @@ -27,4 +27,11 @@ void if_notifier_destroy(struct if_notifier *); void if_notifier_run(void); void if_notifier_wait(void); +/* Below functions are reserved for if-notifier-manual , i.e. for manual + * notifications from the OVS code. + * Must not be implemented by OS specific notifiers. */ + +void if_notifier_manual_set_cb(if_notify_func *); +void if_notifier_manual_report(void); + #endif /* if-notifier.h */ diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 9095ebf5d..37131712d 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -529,11 +529,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_notifier_manual_set_cb(if_change_cb); } void bridge_exit(bool delete_datapath) { + if_notifier_manual_set_cb(NULL); if_notifier_destroy(ifnotifier); seq_destroy(ifaces_changed);
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 manual notifier introduced. Its function 'if_notifier_manual_report()' could be called directly by the code that aware about changes. This new notifier is thread safe. 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. Version 2: * Main functions moved from bridge.c to the new lib/if-notifier-manual.c to allow using from other lib code. lib/automake.mk | 3 ++- lib/if-notifier-manual.c | 55 ++++++++++++++++++++++++++++++++++++++++ lib/if-notifier.h | 7 +++++ vswitchd/bridge.c | 2 ++ 4 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 lib/if-notifier-manual.c