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 <f...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
