On Thu, May 29, 2014 at 12:55:24PM -0700, Ben Pfaff wrote:
> 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 <f...@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.
Makes sense to me. I will take the union out.
> 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;
Good one, I will add those.
> 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.
I think the same thing should be applied to the switch statement
in mcast_snooping_is_membership() ?
...
switch (igmp_type) {
case htons(IGMP_HOST_MEMBERSHIP_REPORT):
case htons(IGMPV2_HOST_MEMBERSHIP_REPORT):
case htons(IGMP_HOST_LEAVE_MESSAGE):
return true;
}
return false;
> 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.
You're right. The idea is to make that useful when we add IPv6 support,
so the proto will differentiate between ipv4 and ipv6. And the ip4 and ip6
will be inside of a union.
struct mcast_ip {
union {
ovs_be32 ip4;
struct in6_addr ip6;
} u;
ovs_be16 proto;
};
Currently the limitation is explicit in the entry point of multicast
processing: xlate_normal()
...
/* Determine output bundle. */
if (mcast_snooping_enabled(ctx->xbridge->ms)
&&!eth_addr_is_broadcast(flow->dl_dst)
&& eth_addr_is_multicast(flow->dl_dst)
&& flow->dl_type == htons(ETH_TYPE_IP)) {
^^^^^^^^^^^
So for other packets that are not IPv4, the code works as if mcast
snooping is disabled.
> 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.
Of course, I will fix that.
As before, I will wait other patches to be reviewed and follow-up
with v3.
Thanks,
fbl
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
