| Message ID | 20191105172051.16370-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] bridge: Allow manual notifications about interfaces' updates. | expand |
On 5 Nov 2019, at 18:20, 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> Reviewed and tested this patch in combination with the “netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event”. LGTM, Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 6 Nov 2019, at 14:32, Eelco Chaudron wrote: > On 5 Nov 2019, at 18:20, 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> > > Reviewed and tested this patch in combination with the “netdev-dpdk: > add support for the RTE_ETH_EVENT_INTR_RESET event”. > > LGTM, > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Is there anything waiting to get this included so my other patch could get included? Cheers, Eelco
On Tue, Dec 17, 2019 at 02:45:03PM +0100, Eelco Chaudron wrote: > > > On 6 Nov 2019, at 14:32, Eelco Chaudron wrote: > > > On 5 Nov 2019, at 18:20, 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> > > > > Reviewed and tested this patch in combination with the “netdev-dpdk: add > > support for the RTE_ETH_EVENT_INTR_RESET event”. > > > > LGTM, > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Is there anything waiting to get this included so my other patch could get > included? Seems reasonable. The only thing I noticed in it is two function definitions where the function name should be moved to the beginning of a line. Ilya, I'm assuming you'll apply it when you're satisfied?
On 17.12.2019 19:25, Ben Pfaff wrote: > On Tue, Dec 17, 2019 at 02:45:03PM +0100, Eelco Chaudron wrote: >> >> >> On 6 Nov 2019, at 14:32, Eelco Chaudron wrote: >> >>> On 5 Nov 2019, at 18:20, 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> >>> >>> Reviewed and tested this patch in combination with the “netdev-dpdk: add >>> support for the RTE_ETH_EVENT_INTR_RESET event”. >>> >>> LGTM, >>> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> >> Is there anything waiting to get this included so my other patch could get >> included? > > Seems reasonable. The only thing I noticed in it is two function > definitions where the function name should be moved to the beginning of > a line. Thanks for spotting. Will fix before applying. > > Ilya, I'm assuming you'll apply it when you're satisfied? I'm taking the last look at the dependent patch https://patchwork.ozlabs.org/patch/1192944/ It seems OK so far and I'm going to apply both patches soon. Best regards, Ilya Maximets.
On 17.12.2019 19:39, Ilya Maximets wrote: > On 17.12.2019 19:25, Ben Pfaff wrote: >> On Tue, Dec 17, 2019 at 02:45:03PM +0100, Eelco Chaudron wrote: >>> >>> >>> On 6 Nov 2019, at 14:32, Eelco Chaudron wrote: >>> >>>> On 5 Nov 2019, at 18:20, 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> >>>> >>>> Reviewed and tested this patch in combination with the “netdev-dpdk: add >>>> support for the RTE_ETH_EVENT_INTR_RESET event”. >>>> >>>> LGTM, >>>> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> >>> Is there anything waiting to get this included so my other patch could get >>> included? >> >> Seems reasonable. The only thing I noticed in it is two function >> definitions where the function name should be moved to the beginning of >> a line. > > Thanks for spotting. Will fix before applying. > >> >> Ilya, I'm assuming you'll apply it when you're satisfied? > > I'm taking the last look at the dependent patch > https://patchwork.ozlabs.org/patch/1192944/ > It seems OK so far and I'm going to apply both patches soon. > > Best regards, Ilya Maximets. > Thanks Eelco and Ben. Fixed and applied. Best regards, Ilya Maximets.
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> --- This functionality 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 1: * Sending as an official patch. * No changes since RFC v2. RFC v2: * 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