[ovs-dev] [PATCH 15/18] dpif-xlate: Add functions to update multicast snooping db

Ben Pfaff blp at nicira.com
Tue May 20 22:53:26 UTC 2014


On Fri, Apr 11, 2014 at 06:34:20PM -0300, Flavio Leitner wrote:
> This patch adds the functions needed to update the multicast
> snooping database based on the Report/Leave/Query received.
> 
> They are marked with OVS_UNUSED for now.
> 
> Signed-off-by: Flavio Leitner <fbl at redhat.com>

The body of the switch statement should be indented 4 fewer spaces, to
match the existing style elsewhere.

ofproto-dpif-*.c is already quite big, so when I can reasonably put
new code elsewhere I like to do so.  update_mcast_snooping_table__()
seems like a candidate: can mcast-snooping.c just have a function to
process such a packet?  Or, if not, could the individual functions
that update_mcast_snooping_table__() calls do the logging internally?
I guess that they would have trouble logging the names; maybe that's
good enough reason to keep them here.

I find that it's easier to follow code flow with locks if there's only
one place the lock gets unlocked.  In this case,
update_mcast_snooping_table() doesn't do that, but it's easily
transformed to do so, e.g. change:
    if (mcast_xbundle && mcast_xbundle == in_xbundle) {
        ovs_rwlock_unlock(&xbridge->ms->rwlock);
        return;
    }

    maddr.proto = flow->dl_type;
    maddr.u.ip4 = flow->igmp_group.u.ip4;
    update_mcast_snooping_table__(xbridge, flow, &maddr, vlan, in_xbundle);
    ovs_rwlock_unlock(&xbridge->ms->rwlock);
to:
    if (!mcast_xbundle || mcast_xbundle != in_xbundle) {
        maddr.proto = flow->dl_type;
        maddr.u.ip4 = flow->igmp_group.u.ip4;
        update_mcast_snooping_table__(xbridge, flow, &maddr, vlan, in_xbundle);
    }
    ovs_rwlock_unlock(&xbridge->ms->rwlock);
and you could even move the declaration of maddr inward.

I suspect that unconditionally taking the write-lock in
update_mcast_snooping_table() is going to serialize flow translation,
in practice.  That's why update_learning_table() checks whether any
change is needed under the read-lock first, then grabs the write-lock
if it's really needed.  But in the long run RCU is better (and we
should probably adapt mac-learning to use it, or we could switch to a
fatlock there as an easy first step).



More information about the dev mailing list