Hi Simon, Thank you for the review :) On Thu, Jan 4, 2024 at 1:27 PM Simon Horman <[email protected]> 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 <[email protected]> > > --- > > 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/[email protected]/> 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, > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
