| Message ID | 20231130153821.855531-1-mheib@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v5,1/2] mcast-snooping: Store IGMP/MLD protocol version. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | fail | test: fail |
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 30 Nov 2023, at 16:38, 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> Thanks Mohammad, for following up on this patch set. I have one small comment below, the rest looks good to me. Cheers, Eelco > --- > lib/mcast-snooping.c | 20 +++++++++++++------- > lib/mcast-snooping.h | 21 ++++++++++++++++++--- > ofproto/ofproto-dpif-xlate.c | 10 ++++++++-- > 3 files changed, 39 insertions(+), 12 deletions(-) > > diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c > index 43805ae4d..5046e35d2 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) > This function assumes the dp_packlet is IGMPv3, so we do not need to have grp_proto as an input parameter, we can hardcode it to MCAST_GROUP_IGMPV3. > { > 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", > -- > 2.34.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c index 43805ae4d..5046e35d2 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",
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> --- lib/mcast-snooping.c | 20 +++++++++++++------- lib/mcast-snooping.h | 21 ++++++++++++++++++--- ofproto/ofproto-dpif-xlate.c | 10 ++++++++-- 3 files changed, 39 insertions(+), 12 deletions(-)