Re: [ovs-dev] [PATCH] netlink-notifier: Support multiple groups.

[Jarno Rajahalme]

Thanks for the review!

Applied to master with changes indicated, with one response below,

  Jarno

> On Jun 2, 2016, at 7:19 AM, Thadeu Lima de Souza Cascardo 
> <casca...@redhat.com> wrote:
> 
> On Thu, May 26, 2016 at 04:29:28PM -0700, Jarno Rajahalme wrote:
>> A netlink notifier ('nln') already supports multiple notifiers.  This
>> patch allows each of these notifiers to subscribe to a different
>> multicast group.  Sharing a single socket for multiple event types
>> (each on their own multicast group) provides serialization of events
>> when reordering of different event types could be problematic.  For
>> example, if a 'create' event and 'delete' event are on different
>> netlink multicast group, we may want to process those events in the
>> order in which kernel issued them, rather than in the order we happen
>> to check for them.
>> 
>> Moving the multicast group argument from nln_create() to
>> nln_notifier_create() allows each notifier to specify a different
>> multicast group.  The parse callback needs to identify the group the
>> message belonged to by returning the corresponding group number, or 0
>> when an parse error occurs.
>> 
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
> 
> Hi, Jarno.
> 
> Just two nitpicks below. Besides that,
> 
> Acked-by: Thadeu Lima de Souza Cascardo <casca...@redhat.com 
> <mailto:casca...@redhat.com>>
> 
>> ---
>> lib/netlink-notifier.c         | 47 
>> ++++++++++++++++++++++++++----------------
>> lib/netlink-notifier.h         | 13 ++++++------
>> lib/route-table.c              | 40 +++++++++++++++--------------------
>> lib/rtnetlink.c                | 10 ++++-----
>> tests/test-netlink-conntrack.c | 32 +++++++++++++++++-----------
>> 5 files changed, 78 insertions(+), 64 deletions(-)
>> 
>> diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
>> index c2b4f7b..0e4ed9a 100644
>> --- a/lib/netlink-notifier.c
>> +++ b/lib/netlink-notifier.c
>> 

(snip)

>> @@ -126,11 +121,21 @@ nln_notifier_create(struct nln *nln, nln_notify_func 
>> *cb, void *aux)
>>         nln_run(nln);
>>     }
>> 
>> +    error = nl_sock_join_mcgroup(nln->notify_sock, multicast_group);
>> +    if (error) {
>> +        VLOG_WARN("could not join netlink multicast group: %s",
>> +                  ovs_strerror(error));
>> +        return NULL;
>> +    }
>> +
>>     notifier = xmalloc(sizeof *notifier);
>> -    ovs_list_push_back(&nln->all_notifiers, &notifier->node);
>> +    notifier->multicast_group = multicast_group;
>>     notifier->cb = cb;
>>     notifier->aux = aux;
>>     notifier->nln = nln;
>> +
>> +    ovs_list_push_back(&nln->all_notifiers, &notifier->node);
>> +
>>     return notifier;
>> }
>> 
>> @@ -142,6 +147,8 @@ nln_notifier_destroy(struct nln_notifier *notifier)
>>     if (notifier) {
>>         struct nln *nln = notifier->nln;
>> 
>> +        nl_sock_leave_mcgroup(nln->notify_sock, notifier->multicast_group);
>> +
> 
> I was afraid this could be a problem, but we hardly destroy any notifier, we
> just call if_notifier_destroy when exiting the bridge. It just means that if 
> we
> ever have notifiers with a shorter lifetime, this either means we have a 
> single
> notifier per group, or we need some refcounting. I think you should keep this
> change, as it seems the best decision right now.
> 

I changed this to leave the group only if no other notifier is interested in 
the same group, like this:

diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
index 0e4ed9a..f6d1e4d 100644
--- a/lib/netlink-notifier.c
+++ b/lib/netlink-notifier.c
@@ -146,10 +146,21 @@ nln_notifier_destroy(struct nln_notifier *notifier)
 {
     if (notifier) {
         struct nln *nln = notifier->nln;
-
-        nl_sock_leave_mcgroup(nln->notify_sock, notifier->multicast_group);
+        struct nln_notifier *iter;
+        int count = 0;
 
         ovs_list_remove(&notifier->node);
+
+        /* Leave the group if no other notifier is interested in it. */
+        LIST_FOR_EACH (iter, node, &nln->all_notifiers) {
+            if (iter->multicast_group == notifier->multicast_group) {
+                count++;
+            }
+        }
+        if (count == 0) {
+            nl_sock_leave_mcgroup(nln->notify_sock, notifier->multicast_group);
+        }
+
         if (ovs_list_is_empty(&nln->all_notifiers)) {
             nl_sock_destroy(nln->notify_sock);
             nln->notify_sock = NULL;


_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev