[ovs-dev,v6,1/2] mcast-snooping: Store IGMP/MLD protocol version.

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 <mheib@redhat.com>
---
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(-)

On Wed, Dec 27, 2023 at 01:15:22PM +0200, 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 <mheib@redhat.com>
> ---
> 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);

Hi Mohammad,

the code leading up to this hunk looks a bit like this:

	grp = mcast_snooping_lookup(ms, addr, vlan);
	if (!grp) {
		/* Create grp */
	} else {
		ovs_list_remove(&grp->group_node);
	}
	mcast_group_insert_bundle(ms, grp, port, ms->idle_time);

In v4 of the patchset grp->protocol_version was set inside the
(!grp) arm of the if condition. But now it is set below.

Is this intentional? If so, I have a few questions:

1. Is it ok to set grp->protocol_version after the
   mcast_group_insert_bundle() call?
2. Is it ok to reset grp->protocol_version for an existing grp?
3. Are there situations where 2 will change the value of
   grp->protocol_version?

>  
> +    /* 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;

...

Hi Simon,
Thank you for the review :)

On Thu, Jan 4, 2024 at 1:27 PM Simon Horman <horms@ovn.org> wrote:

> On Wed, Dec 27, 2023 at 01:15:22PM +0200, 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 <mheib@redhat.com>
> > ---
> > 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);
>
> Hi Mohammad,
>
> the code leading up to this hunk looks a bit like this:
>
>         grp = mcast_snooping_lookup(ms, addr, vlan);
>         if (!grp) {
>                 /* Create grp */
>         } else {
>                 ovs_list_remove(&grp->group_node);
>         }
>         mcast_group_insert_bundle(ms, grp, port, ms->idle_time);
>
> In v4 of the patchset grp->protocol_version was set inside the
> (!grp) arm of the if condition. But now it is set below.
>
> Is this intentional? If so, I have a few questions:
>
yes, i updated the code to cover cases where a new port that uses a
different igmp/mld version will be added to this group.

>
> 1. Is it ok to set grp->protocol_version after the
>    mcast_group_insert_bundle() call?
>

yes, That will update the group version to match the protocol version used
by the latest added port,
that will not have any effect because we only use the protocol version for
debuggability/user-inform and
not used in the code at all.

2. Is it ok to reset grp->protocol_version for an existing grp?
>

actually, in v5 I was setting the protocol version for newly created grps
only, but based on feedback on v5 for Eelco Chaudron
<https://patchwork.ozlabs.org/project/openvswitch/list/?submitter=70613>
[v5]
<https://patchwork.ozlabs.org/project/openvswitch/patch/20231130153821.855531-2-mheib@redhat.com/>
i updated the code to store the latest protocol used to join this group
which will be more accurate in the DB.

3. Are there situations where 2 will change the value of
>    grp->protocol_version?
>
only if the user uses two different ports each one using a different
igmp/mld version to join the same
mcast group, this is an infrequent scenario but it will be better to use
the latest join port protocol version.

>
> >
> > +    /* 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;
>
> ...
>


> Thanks,
>

On 12/27/23 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 <mheib@redhat.com>
> ---
> v6: Rebase on top of current master.
>     Address comments from Eelco:
>     - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
>       function.
> ---

Hi, Eelco and Simon, do you have any further comments on this patch set?
i.e. did you have a chance to review this version?  I see you both looked
at the previous ones.

I see some conversation here, but I'm not sure if it is resolved or not.

(I didn't review the code myself and will likley not have much time before
branching tomorrow.)

Best regards, Ilya Maximets.

On 16 Jan 2024, at 15:20, Ilya Maximets wrote:

> On 12/27/23 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 <mheib@redhat.com>
>> ---
>> v6: Rebase on top of current master.
>>     Address comments from Eelco:
>>     - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
>>       function.
>> ---
>
> Hi, Eelco and Simon, do you have any further comments on this patch set?
> i.e. did you have a chance to review this version?  I see you both looked
> at the previous ones.
>
> I see some conversation here, but I'm not sure if it is resolved or not.
>
> (I didn't review the code myself and will likley not have much time before
> branching tomorrow.)

My plan was to do it todays morning, but did not happen :) Will do it before the end of the day.

> Best regards, Ilya Maximets.

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 <mheib@redhat.com>

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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 1/16/24 15:26, Eelco Chaudron wrote:
> 
> 
> On 16 Jan 2024, at 15:20, Ilya Maximets wrote:
> 
>> On 12/27/23 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 <mheib@redhat.com>
>>> ---
>>> v6: Rebase on top of current master.
>>>     Address comments from Eelco:
>>>     - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
>>>       function.
>>> ---
>>
>> Hi, Eelco and Simon, do you have any further comments on this patch set?
>> i.e. did you have a chance to review this version?  I see you both looked
>> at the previous ones.
>>
>> I see some conversation here, but I'm not sure if it is resolved or not.
>>
>> (I didn't review the code myself and will likley not have much time before
>> branching tomorrow.)
> 
> My plan was to do it todays morning, but did not happen :) Will do it before the end of the day.

Thanks, Eelco.

I see the changes you requested are mostly cosmetic.  Will you be comfortable
making them yourself while applying the set?  Asking because I believe
Mohammad is on PTO and will not be able to follow up with a new version before
branching, i.e. the feature may miss the release.

Maybe also add a small NEWS entry to the ovs-appctl section that mdb/show now
provides multicast group protocol information.

Best regards, Ilya Maximets.

On 17 Jan 2024, at 12:16, Ilya Maximets wrote:

> On 1/16/24 15:26, Eelco Chaudron wrote:
>>
>>
>> On 16 Jan 2024, at 15:20, Ilya Maximets wrote:
>>
>>> On 12/27/23 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 <mheib@redhat.com>
>>>> ---
>>>> v6: Rebase on top of current master.
>>>>     Address comments from Eelco:
>>>>     - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
>>>>       function.
>>>> ---
>>>
>>> Hi, Eelco and Simon, do you have any further comments on this patch set?
>>> i.e. did you have a chance to review this version?  I see you both looked
>>> at the previous ones.
>>>
>>> I see some conversation here, but I'm not sure if it is resolved or not.
>>>
>>> (I didn't review the code myself and will likley not have much time before
>>> branching tomorrow.)
>>
>> My plan was to do it todays morning, but did not happen :) Will do it before the end of the day.
>
> Thanks, Eelco.
>
> I see the changes you requested are mostly cosmetic.  Will you be comfortable
> making them yourself while applying the set?  Asking because I believe
> Mohammad is on PTO and will not be able to follow up with a new version before
> branching, i.e. the feature may miss the release.
>
> Maybe also add a small NEWS entry to the ovs-appctl section that mdb/show now
> provides multicast group protocol information.

Sound good to me, let me prepare this. If you see any issues with my suggestions let me know, else I’ll commit.

//Eelco

On 1/17/24 13:14, Eelco Chaudron wrote:
> 
> 
> On 17 Jan 2024, at 12:16, Ilya Maximets wrote:
> 
>> On 1/16/24 15:26, Eelco Chaudron wrote:
>>>
>>>
>>> On 16 Jan 2024, at 15:20, Ilya Maximets wrote:
>>>
>>>> On 12/27/23 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 <mheib@redhat.com>
>>>>> ---
>>>>> v6: Rebase on top of current master.
>>>>>     Address comments from Eelco:
>>>>>     - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
>>>>>       function.
>>>>> ---
>>>>
>>>> Hi, Eelco and Simon, do you have any further comments on this patch set?
>>>> i.e. did you have a chance to review this version?  I see you both looked
>>>> at the previous ones.
>>>>
>>>> I see some conversation here, but I'm not sure if it is resolved or not.
>>>>
>>>> (I didn't review the code myself and will likley not have much time before
>>>> branching tomorrow.)
>>>
>>> My plan was to do it todays morning, but did not happen :) Will do it before the end of the day.
>>
>> Thanks, Eelco.
>>
>> I see the changes you requested are mostly cosmetic.  Will you be comfortable
>> making them yourself while applying the set?  Asking because I believe
>> Mohammad is on PTO and will not be able to follow up with a new version before
>> branching, i.e. the feature may miss the release.
>>
>> Maybe also add a small NEWS entry to the ovs-appctl section that mdb/show now
>> provides multicast group protocol information.
> 
> Sound good to me, let me prepare this. If you see any issues with my suggestions let me know, else I’ll commit.

Your style/formatting suggestions LGTM.

> 
> //Eelco
>

On Tue, Jan 16, 2024 at 03:20:08PM +0100, Ilya Maximets wrote:
> On 12/27/23 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 <mheib@redhat.com>
> > ---
> > v6: Rebase on top of current master.
> >     Address comments from Eelco:
> >     - hardcode MCAST_GROUP_IGMPV3 inside mcast_snooping_add_report
> >       function.
> > ---
> 
> Hi, Eelco and Simon, do you have any further comments on this patch set?
> i.e. did you have a chance to review this version?  I see you both looked
> at the previous ones.
> 
> I see some conversation here, but I'm not sure if it is resolved or not.
> 
> (I didn't review the code myself and will likley not have much time before
> branching tomorrow.)

I believe things have moved on and these patches.
So sorry for not responding earlier.

For the record, I am happy with these patches.