[ovs-dev] bridge: Allow manual notifications about interfaces' updates.
diff mbox series

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.
Related show

Commit Message

Ilya Maximets Nov. 5, 2019, 5:20 p.m. UTC
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

Comments

Eelco Chaudron Nov. 6, 2019, 1:32 p.m. UTC | #1
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>
Eelco Chaudron Dec. 17, 2019, 1:45 p.m. UTC | #2
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
Ben Pfaff Dec. 17, 2019, 6:25 p.m. UTC | #3
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?
Ilya Maximets Dec. 17, 2019, 6:39 p.m. UTC | #4
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.
Ilya Maximets Dec. 18, 2019, 12:30 a.m. UTC | #5
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.

Patch
diff mbox series

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);