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