diff mbox series

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

Message ID 20231120142245.1835897-1-mheib@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v4,1/2] mcast-snooping: Store IGMP/MLD protocol version. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Mohammad Heib Nov. 20, 2023, 2:22 p.m. UTC 
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 <mheib@redhat.com>
---
 lib/mcast-snooping.c         | 20 +++++++++++++-------
 lib/mcast-snooping.h         | 21 ++++++++++++++++++---
 ofproto/ofproto-dpif-xlate.c | 10 ++++++++--
 3 files changed, 39 insertions(+), 12 deletions(-)

Comments

Simon Horman Nov. 20, 2023, 3:41 p.m. UTC | #1 
On Mon, Nov 20, 2023 at 04:22:44PM +0200, 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.

Thanks Mohammad,

I see in patch 2/2 that the user can now gain access to the igmp/mld
version of each group. But I am wondering if we could add some text
to the commit message to explain, perhaps via an example, why
a user might want such information.
Mohammad Heib Nov. 20, 2023, 6:52 p.m. UTC | #2 
On Mon, Nov 20, 2023 at 6:09 PM Simon Horman <horms@ovn.org> wrote:

> On Mon, Nov 20, 2023 at 04:22:44PM +0200, 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.
>
> Thanks Mohammad,
>
> I see in patch 2/2 that the user can now gain access to the igmp/mld
> version of each group. But I am wondering if we could add some text
> to the commit message to explain, perhaps via an example, why
> a user might want such information.
>

Hi Simon,

Thank you for the review.
actually, i don't really have a good reason why the user will need the
group protocol in OVS stand-alone case,
but I'm trying to expand that here and save the protocol because i need it
in the OVN/OVS case where we store
each Mcast group information inside ovn-sb DB as raw in *the MCAST_GROUP*
table, and i have to expose a protocol version
for each Group in this table, cause OVN relies on the ovs mcast
implementation to maintain this table, i thought this would be the cleaner
way to accomplish that.

so now i don't really know which example can be good to add :(
do you think adding a small example of extracting the protocol will be good
enough?

Thanks,
Simon Horman Dec. 8, 2023, 12:56 p.m. UTC | #3 
On Mon, Nov 20, 2023 at 08:52:00PM +0200, Mohammad Heib wrote:
> On Mon, Nov 20, 2023 at 6:09 PM Simon Horman <horms@ovn.org> wrote:
> 
> > On Mon, Nov 20, 2023 at 04:22:44PM +0200, 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.
> >
> > Thanks Mohammad,
> >
> > I see in patch 2/2 that the user can now gain access to the igmp/mld
> > version of each group. But I am wondering if we could add some text
> > to the commit message to explain, perhaps via an example, why
> > a user might want such information.
> >
> 
> Hi Simon,
> 
> Thank you for the review.
> actually, i don't really have a good reason why the user will need the
> group protocol in OVS stand-alone case,
> but I'm trying to expand that here and save the protocol because i need it
> in the OVN/OVS case where we store
> each Mcast group information inside ovn-sb DB as raw in *the MCAST_GROUP*
> table, and i have to expose a protocol version
> for each Group in this table, cause OVN relies on the ovs mcast
> implementation to maintain this table, i thought this would be the cleaner
> way to accomplish that.
> 
> so now i don't really know which example can be good to add :(
> do you think adding a small example of extracting the protocol will be good
> enough?

Thanks Mohammad,

and sorry for the slow response.

I think a good way forwards would be to include an explanation
along the lines of usage in the OVN/OVS use-case.

Perhaps also add that exposing this to user tooling is expected
to aid debugging and test coverage (if that is true).
diff mbox series

Patch

diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index 029ca2855..99516bd43 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;
@@ -415,6 +416,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 +433,19 @@  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
 mcast_snooping_add_report(struct mcast_snooping *ms,
                           const struct dp_packet *p,
-                          uint16_t vlan, void *port)
+                          uint16_t vlan, void *port,
+                          mcast_group_proto grp_proto)
 {
     ovs_be32 ip4;
     size_t offset;
@@ -478,7 +482,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 +517,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 +550,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..f54007740 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,14 +197,17 @@  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,
-                              uint16_t vlan, void *port)
+                              uint16_t vlan, void *port,
+                              mcast_group_proto 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 289f8a736..07a147bbc 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;
+        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",
@@ -2837,7 +2842,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,
+                                          MCAST_GROUP_IGMPV3);
         if (count) {
             xlate_report_debug(ctx, OFT_DETAIL, "multicast snooping processed "
                                "%d addresses on port %s in VLAN %d",