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 <mh...@redhat.com>
>
> 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.
>> /* 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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
