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