[ovs-dev] [PATCH 04/18] lib: Add igmp generic snooping library bits

Flavio Leitner fbl at redhat.com
Tue May 20 22:26:31 UTC 2014


On Tue, May 20, 2014 at 02:38:08PM -0700, Ben Pfaff wrote:
> On Fri, Apr 11, 2014 at 06:34:09PM -0300, Flavio Leitner wrote:
> > This patch adds generic IPv4 library code to deal with
> > igmp snooping that is implemented in follow-up patches.
> > 
> > Signed-off-by: Cong Wang <amwang at redhat.com>
> > Signed-off-by: Daniel Borkmann <dborkman at redhat.com>
> > Acked-by: Thomas Graf <tgraf at redhat.com>
> > Signed-off-by: Flavio Leitner <fbl at redhat.com>
> 
> Clang says:
> 
>     In file included from ../lib/mcast-snooping.c:20:
>     ../lib/mcast-snooping.h:142:20: error: use of undeclared identifier 'ml'
>         OVS_REQ_WRLOCK(ml->rwlock);
>                        ^
>     ../lib/compiler.h:117:45: note: expanded from macro 'OVS_REQ_WRLOCK'
>         __attribute__((exclusive_locks_required(__VA_ARGS__)))
>                                                 ^
>     In file included from ../lib/mcast-snooping.c:20:
>     ../lib/mcast-snooping.h:145:20: error: use of undeclared identifier 'ml'
>         OVS_REQ_WRLOCK(ml->rwlock);
>                        ^
>     ../lib/compiler.h:117:45: note: expanded from macro 'OVS_REQ_WRLOCK'
>         __attribute__((exclusive_locks_required(__VA_ARGS__)))
> 
> sparse says:
> 
>     ../lib/mcast-snooping.c:89:28: warning: restricted __be16 degrades to integer
>     ../lib/mcast-snooping.c:90:34: warning: incorrect type in argument 1 (different base types)
>     ../lib/mcast-snooping.c:90:34:    expected unsigned int [unsigned] [usertype] <noident>
>     ../lib/mcast-snooping.c:90:34:    got restricted __be32 const [usertype] ip4
>     ../lib/mcast-snooping.c:112:28: warning: restricted __be16 degrades to integer
>     ../lib/mcast-snooping.c:137:15: warning: incorrect type in assignment (different base types)
>     ../lib/mcast-snooping.c:137:15:    expected unsigned char [unsigned] [assigned] [usertype] proto
>     ../lib/mcast-snooping.c:137:15:    got restricted __be16
> 
> It looks like a comment in mcast-snooping.h copied a spelling error in
> mac-learning.h (s/hodling/holding/):
>      * over or accessing a mcast_entry without hodling the parent
> 
> Indentation in mcast_snooping_is_membership() doesn't quite match the
> OVS standard, the 'break' is spurious after the 'return', and the ';'
> on the switch is odd.  So this:
>     switch (ntohs(igmp_type)) {
>         case IGMP_HOST_MEMBERSHIP_REPORT:
>         case IGMPV2_HOST_MEMBERSHIP_REPORT:
>         case IGMP_HOST_LEAVE_MESSAGE:
>             return true;
>             break;
>     };
> would ordinarily be written as:
>     switch (ntohs(igmp_type)) {
>     case IGMP_HOST_MEMBERSHIP_REPORT:
>     case IGMPV2_HOST_MEMBERSHIP_REPORT:
>     case IGMP_HOST_LEAVE_MESSAGE:
>         return true;
>     }
> 
> mcast_table_hash() compares the 8-bit 'proto' against a 16-bit htons()
> return value.  (I guess sparse complained about that above, but I
> noticed too.)  Ditto in mcast_entry_match_group().
> 
> In mcast_snooping_create() would you mind reordering the assignments
> slightly to put them in the same order as the member list within the
> struct definition?  It is easier to visually verify that every member
> gets initialized that way.
> 
> OVS style has moved along slightly since you wrote this code, to use a
> "struct ovs_refcount" instead of direct atomic ops, would you mind
> switching this code over to that?  You can follow the model in
> mac-learning.c.
> 
> The definition of mcast_snooping_set_idle_time(), not just the
> prototype, should include the OVS_REQ_WRLOCK annotation, because Clang
> isn't too good about remembering such things from the prototypes.
> 
> I don't think that the code in mcast_entry_insert_bundle() is really
> safe because LIST_FOR_EACH leaves 'b' pointing to somewhere near the
> list head (essentially garbage) if it terminates in the usual way (not
> via break).  Same problem in mcast_snooping_add_mrouter().
> 
> We don't usually do assertions like the one at the beginning of
> mcast_entry_has_bundles():
>     ovs_assert(e != NULL);
>     return !list_is_empty(&e->lru_bundles);
> because the following list_is_empty() call will pretty effectively
> also give a good indication of the failure ;-)
> 
> This:
> bool mcast_snooping_add_group(struct mcast_snooping *ms,
>                               const struct mcast_ip *grp, uint16_t vlan,
>                               void *port)
> should be:
> bool
> mcast_snooping_add_group(struct mcast_snooping *ms,
>                          const struct mcast_ip *grp, uint16_t vlan,
>                          void *port)
> 
> Similarly for mcast_snooping_leave_group().
> 
> We usually write a space after LIST_FOR_EACH, e.g.
>     LIST_FOR_EACH (mrouter, lru_node, &ms->mrouter_lru) {
> 
> mcast_snooping_port_disable_flood() ends with a spurious 'return'.
> 
> mcast_snooping_wait__() begins with a spurious blank line.

OK, I can fix those issues and send a v2. I will wait for
feedbacks on the remaining patches to address all them together.

> How much of a common case do you expect multicast lookups to be?  Do
> you think it will be worth trying to RCUify this, to avoid taking a
> lock on the read path?

Enough to worth to do that.  There was no RCU when this work has
started and rwlock is more simple to implement and review. 

Anyway, it's on my ToDo list, along with adding support for IGMPv3 and IPv6.

Flavio



More information about the dev mailing list