[ovs-dev] [PATCH v2 1/7] lib: Add IGMP snooping library bits

Ben Pfaff blp at nicira.com
Thu May 29 19:55:24 UTC 2014


On Wed, May 28, 2014 at 10:10:28PM -0300, Flavio Leitner wrote:
> This patch adds generic IGMP snooping library code
> that is used in follow-up patches.
> 
> Signed-off-by: Flavio Leitner <fbl at redhat.com>

Thanks for v2!

Some structures in mcast-learning.h have union members that look like
this:

    union {
        void *p;
        ofp_port_t ofp_port;
    } port OVS_GUARDED;

I guess this is because of the similar member in struct mac_entry.
I'm not really happy with that member there.  It is there because
lib/learning-switch.c uses the 'ofp_port' member, whereas
learning-switch is in turn used only by tests/test-controller.  I
don't expect test-controller to want to use mcast-learning.  Unless
you have a specific reason to have the union in these places, I'd
rather just have a single void pointer without a nested union.

In mcast-learning.h, I would add to the comments on mcast_snooping the
structures referenced, because that makes the data structures easier
to understand, e.g.:

    /* Contains struct mcast_mrouter_bundles, least recently used at the front,
     * most recently used at the back. */
    struct list lrus OVS_GUARDED;

    /* Contains struct mcast_mrouter_bundles for ports connected to mcast
     * routers, least recently used at the front, most recently used at the
     * back. */
    struct list mrouter_lru OVS_GUARDED;

    /* Contains struct mcast_fport_bundles that list the ports to be flooded
     * with multicast packets. */
    struct list fport_list OVS_GUARDED;

In mcast_snooping_is_query(), please write this:
+    if (ntohs(igmp_type) == IGMP_HOST_MEMBERSHIP_QUERY) {
as:
+    if (igmp_type == htons(IGMP_HOST_MEMBERSHIP_QUERY)) {
because the compiler can easily optimize the constant expression on
the right side.

After reading through the code, I am confused about the meaning, if
any, of "mcast_ip"s with proto != ETH_TYPE_IP.  These are treated as
special cases all over, but it doesn't look like they're useful.  For
example, mcast_entry_match_group() will never report that a non-IP
mcast_ip is a match, so it looks like every time
mcast_snooping_add_group() is called with a non-IP mcast_ip, it will
add a new entry, but that entry will never get found.  Also, all
non-IP mcast_ips hash to the same value (0).  So I suspect that non-IP
just doesn't make sense.

I think that the "if (needs_update)" code in
mcast_snooping_add_group() could be moved inside the block that sets
needs_update, and then there wouldn't be a need for the "if" test.

Thanks,

Ben.



More information about the dev mailing list