On 27 Dec 2023, at 12:15, Mohammad Heib wrote:
> Store igmp/mld protocol version into the
> mcast_group internally, the multicast snooping feature
> is used by many OVS consumers and those consumers heavily rely
> on the OVS implementation to manage/deal with mcast groups,
> some of those consumers also need to deal/expose the mcast protocol
> to the end user for debuggability purposes.
>
> OVN for example needs to expose the protocol version to the end user
> to match between the protocol version used in the OVN logical switches
> and the uplink ports
>
> Therefore, instead of implementing this in each OVS consumer that needs
> to deal mcast group protocol version which will be very complicated
> implementation since it rely on the OVS code, saving the protocol to
> the mdb inside OVS will give that consumer access to the protocol version
> very easily.
>
> Signed-off-by: Mohammad Heib <[email protected]>
Hi Mohammad,
I found one small nits which I missed in my previous review :( The rest looks
good.
//Eelco
> ---
> v6: Rebase on top of current master.
> Address comments from Eelco:
> - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
> function.
> ---
> lib/mcast-snooping.c | 20 ++++++++++++++------
> lib/mcast-snooping.h | 18 ++++++++++++++++--
> ofproto/ofproto-dpif-xlate.c | 7 ++++++-
> 3 files changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 43805ae4d..995216a4b 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -389,7 +389,8 @@ 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,
> + mcast_group_proto grp_proto)
> OVS_REQ_WRLOCK(ms->rwlock)
> {
> bool learned;
> @@ -424,6 +425,9 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
> }
> mcast_group_insert_bundle(ms, grp, port, ms->idle_time);
>
> + /* update the protocol version. */
> + grp->protocol_version = grp_proto;
> +
> /* Mark 'grp' as recently used. */
> ovs_list_push_back(&ms->group_lru, &grp->group_node);
> return learned;
> @@ -431,11 +435,12 @@ 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,
> + mcast_group_proto 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
> @@ -478,7 +483,8 @@ 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,
> + MCAST_GROUP_IGMPV3);
> }
> if (ret) {
> count++;
> @@ -513,7 +519,8 @@ 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,
> + MCAST_GROUP_MLDV1);
> if (ret) {
> count++;
> }
> @@ -545,7 +552,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,
> + MCAST_GROUP_MLDV2);
> }
> if (ret) {
> count++;
> diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
> index f120405da..8cc8fb0fb 100644
> --- a/lib/mcast-snooping.h
> +++ b/lib/mcast-snooping.h
> @@ -39,6 +39,15 @@ struct mcast_snooping;
> /* Time, in seconds, before expiring a mrouter_port due to inactivity. */
> #define MCAST_MROUTER_PORT_IDLE_TIME 180
>
> +/* Multicast group protocol. */
> +typedef enum {
> + MCAST_GROUP_IGMPV1 = 0,
> + MCAST_GROUP_IGMPV2,
> + MCAST_GROUP_IGMPV3,
> + MCAST_GROUP_MLDV1,
> + MCAST_GROUP_MLDV2,
> +} mcast_group_proto;
> +
> /* Multicast group entry.
> * Guarded by owning 'mcast_snooping''s rwlock. */
> struct mcast_group {
> @@ -51,6 +60,9 @@ struct mcast_group {
> /* VLAN tag. */
> uint16_t vlan;
>
> + /* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or MLDv1,2 */
> + mcast_group_proto protocol_version;
> +
> /* Node in parent struct mcast_snooping group_lru. */
> struct ovs_list group_node OVS_GUARDED;
>
> @@ -185,10 +197,12 @@ 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,
> + mcast_group_proto 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,
> + mcast_group_proto grp_proto)
> OVS_REQ_WRLOCK(ms->rwlock);
> int mcast_snooping_add_report(struct mcast_snooping *ms,
> const struct dp_packet *p,
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 289f8a736..12e13b0be 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2796,6 +2796,7 @@ update_mcast_snooping_table4__(const struct xlate_ctx
> *ctx,
> OVS_REQ_WRLOCK(ms->rwlock)
> {
> const struct igmp_header *igmp;
> + mcast_group_proto grp_proto;
> int count;
> size_t offset;
> ovs_be32 ip4 = flow->igmp_group_ip4;
> @@ -2813,7 +2814,11 @@ 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)) {
> + grp_proto = (ntohs(flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT)
> + ? MCAST_GROUP_IGMPV1
> + : MCAST_GROUP_IGMPV2;
Guess these need to be aligned to the (, or maybe remove the () altogether.
+ grp_proto = ntohs(flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT
+ ? MCAST_GROUP_IGMPV1
+ : MCAST_GROUP_IGMPV2;
> + if (mcast_snooping_add_group4(ms, ip4, vlan, in_xbundle->ofbundle,
> + grp_proto)) {
> xlate_report_debug(ctx, OFT_DETAIL,
> "multicast snooping learned that "
> IP_FMT" is 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
