[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:59 UTC 2014


On Tue, May 20, 2014 at 03:53:26PM -0700, Ben Pfaff wrote:
> 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).

Oh, also again I'd prefer to introduce these functions in the same
commit as their first users.



More information about the dev mailing list