Hi Eelco, On Thu, Nov 16, 2023 at 5:23 PM Eelco Chaudron <[email protected]> wrote:
> > > On 16 Nov 2023, at 15:58, Eelco Chaudron wrote: > > > On 16 Nov 2023, at 15:08, Mohammad Heib wrote: > > > >> Store the igmp/mld protocol version into the > >> mcast_group internally. > >> > >> This can be used by ovs consumers to update > >> about the igmp/mld version of each group. > >> > >> Signed-off-by: Mohammad Heib <[email protected]> > > > > Hi Mohammad, > > > > Thanks for the patch, I have not reviewed the actual code yet, but it > would be good to include a use case for this patch (maybe expand this to a > series). This way it’s clear why we need to store this information. > > > > Cheers, > > > > Eelco > > After a quick code review I have one comment, see below. > > >> --- > >> lib/mcast-snooping.c | 16 +++++++++------- > >> lib/mcast-snooping.h | 9 ++++++--- > >> ofproto/ofproto-dpif-xlate.c | 6 ++++-- > >> 3 files changed, 19 insertions(+), 12 deletions(-) > >> > >> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c > >> index 029ca2855..926daf9ac 100644 > >> --- a/lib/mcast-snooping.c > >> +++ b/lib/mcast-snooping.c > >> @@ -389,7 +389,7 @@ mcast_snooping_prune_expired(struct mcast_snooping > *ms, > >> bool > >> mcast_snooping_add_group(struct mcast_snooping *ms, > >> const struct in6_addr *addr, > >> - uint16_t vlan, void *port) > >> + uint16_t vlan, void *port, uint8_t grp_proto) > >> OVS_REQ_WRLOCK(ms->rwlock) > >> { > >> bool learned; > >> @@ -415,6 +415,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms, > >> hmap_insert(&ms->table, &grp->hmap_node, hash); > >> grp->addr = *addr; > >> grp->vlan = vlan; > >> + grp->protocol_version = grp_proto; > >> ovs_list_init(&grp->bundle_lru); > >> learned = true; > >> ms->need_revalidate = true; > >> @@ -431,17 +432,17 @@ mcast_snooping_add_group(struct mcast_snooping > *ms, > >> > >> bool > >> mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4, > >> - uint16_t vlan, void *port) > >> + uint16_t vlan, void *port, uint8_t grp_proto) > >> OVS_REQ_WRLOCK(ms->rwlock) > >> { > >> struct in6_addr addr = in6_addr_mapped_ipv4(ip4); > >> - return mcast_snooping_add_group(ms, &addr, vlan, port); > >> + return mcast_snooping_add_group(ms, &addr, vlan, port, grp_proto); > >> } > >> > >> int > >> mcast_snooping_add_report(struct mcast_snooping *ms, > >> const struct dp_packet *p, > >> - uint16_t vlan, void *port) > >> + uint16_t vlan, void *port, uint8_t grp_proto) > >> { > >> ovs_be32 ip4; > >> size_t offset; > >> @@ -478,7 +479,7 @@ mcast_snooping_add_report(struct mcast_snooping *ms, > >> || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) { > >> ret = mcast_snooping_leave_group4(ms, ip4, vlan, port); > >> } else { > >> - ret = mcast_snooping_add_group4(ms, ip4, vlan, port); > >> + ret = mcast_snooping_add_group4(ms, ip4, vlan, port, > grp_proto); > >> } > >> if (ret) { > >> count++; > >> @@ -513,7 +514,7 @@ mcast_snooping_add_mld(struct mcast_snooping *ms, > >> > >> switch (mld->type) { > >> case MLD_REPORT: > >> - ret = mcast_snooping_add_group(ms, addr, vlan, port); > >> + ret = mcast_snooping_add_group(ms, addr, vlan, port, > MLD_REPORT); > >> if (ret) { > >> count++; > >> } > >> @@ -545,7 +546,8 @@ mcast_snooping_add_mld(struct mcast_snooping *ms, > >> || record->type == > IGMPV3_CHANGE_TO_INCLUDE_MODE)) { > >> ret = mcast_snooping_leave_group(ms, addr, vlan, > port); > >> } else { > >> - ret = mcast_snooping_add_group(ms, addr, vlan, > port); > >> + ret = mcast_snooping_add_group(ms, addr, vlan, > port, > >> + MLD2_REPORT); > >> } > >> if (ret) { > >> count++; > >> diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h > >> index f120405da..6321b63ab 100644 > >> --- a/lib/mcast-snooping.h > >> +++ b/lib/mcast-snooping.h > >> @@ -51,6 +51,9 @@ struct mcast_group { > >> /* VLAN tag. */ > >> uint16_t vlan; > >> > >> + /* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or > MLDv1,2 */ > >> + uint8_t protocol_version; > > Not sure if this is the correct way to store the data. You are storing the > IGMP type definition for IGMP, and for MLD the MLD ICMPv6 value. > I would prefer a newly defined type (enum) that is globally unique because > a new protocol in the future might use an overlapping value. > good point, yes that will cause an issue if a new protocol with overlapping values is created. i updated that by adding an enum in v3. thank you for the quick review :). > >> /* Node in parent struct mcast_snooping group_lru. */ > >> struct ovs_list group_node OVS_GUARDED; > >> > >> @@ -185,14 +188,14 @@ mcast_snooping_lookup4(const struct > mcast_snooping *ms, ovs_be32 ip4, > >> /* Learning. */ > >> bool mcast_snooping_add_group(struct mcast_snooping *ms, > >> const struct in6_addr *addr, > >> - uint16_t vlan, void *port) > >> + uint16_t vlan, void *port, uint8_t > grp_proto) > >> OVS_REQ_WRLOCK(ms->rwlock); > >> bool mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4, > >> - uint16_t vlan, void *port) > >> + uint16_t vlan, void *port, uint8_t > grp_proto) > >> OVS_REQ_WRLOCK(ms->rwlock); > >> int mcast_snooping_add_report(struct mcast_snooping *ms, > >> const struct dp_packet *p, > >> - uint16_t vlan, void *port) > >> + uint16_t vlan, void *port, uint8_t > grp_proto) > >> OVS_REQ_WRLOCK(ms->rwlock); > >> int mcast_snooping_add_mld(struct mcast_snooping *ms, > >> const struct dp_packet *p, > >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > >> index e24377330..26bd678cd 100644 > >> --- a/ofproto/ofproto-dpif-xlate.c > >> +++ b/ofproto/ofproto-dpif-xlate.c > >> @@ -2781,7 +2781,8 @@ update_mcast_snooping_table4__(const struct > xlate_ctx *ctx, > >> switch (ntohs(flow->tp_src)) { > >> case IGMP_HOST_MEMBERSHIP_REPORT: > >> case IGMPV2_HOST_MEMBERSHIP_REPORT: > >> - if (mcast_snooping_add_group4(ms, ip4, vlan, > in_xbundle->ofbundle)) { > >> + if (mcast_snooping_add_group4(ms, ip4, vlan, > in_xbundle->ofbundle, > >> + ntohs(flow->tp_src))) { > >> xlate_report_debug(ctx, OFT_DETAIL, > >> "multicast snooping learned that " > >> IP_FMT" is on port %s in VLAN %d", > >> @@ -2805,7 +2806,8 @@ update_mcast_snooping_table4__(const struct > xlate_ctx *ctx, > >> break; > >> case IGMPV3_HOST_MEMBERSHIP_REPORT: > >> count = mcast_snooping_add_report(ms, packet, vlan, > >> - in_xbundle->ofbundle); > >> + in_xbundle->ofbundle, > >> + ntohs(flow->tp_src)); > >> if (count) { > >> xlate_report_debug(ctx, OFT_DETAIL, "multicast snooping > processed " > >> "%d addresses on port %s in VLAN %d", > >> -- > >> 2.34.3 > >> > >> _______________________________________________ > >> dev mailing list > >> [email protected] > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
