| Message ID | 20231110175204.3846196-2-david.marchand@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,1/2] mcast-snooping: Test per port explicit flooding. | expand |
| 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 |
On Fri, Nov 10, 2023 at 06:52:04PM +0100, David Marchand wrote: > When a configuration change triggers an interface destruction/creation > (like for example, setting ofport_request), a port object may still be > referenced as a fport or a rport in the mdb. > > Before the fix, when flooding multicast traffic: > bridge("br0") > ------------- > 0. priority 32768 > NORMAL > -> forwarding to mcast group port > >> mcast flood port is unknown, dropping > -> mcast flood port is input port, dropping > -> forwarding to mcast flood port > > Before the fix, when flooding igmp report traffic: > bridge("br0") > ------------- > 0. priority 32768 > NORMAL > >> mcast port is unknown, dropping the report > -> forwarding report to mcast flagged port > -> mcast port is input port, dropping the Report > -> forwarding report to mcast flagged port > > Add relevant cleanup and update unit tests. > > Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration changed.") > Signed-off-by: David Marchand <david.marchand@redhat.com> Acked-by: Simon Horman <horms@ovn.org>
David Marchand <david.marchand@redhat.com> writes: > When a configuration change triggers an interface destruction/creation > (like for example, setting ofport_request), a port object may still be > referenced as a fport or a rport in the mdb. > > Before the fix, when flooding multicast traffic: > bridge("br0") > ------------- > 0. priority 32768 > NORMAL > -> forwarding to mcast group port > >> mcast flood port is unknown, dropping > -> mcast flood port is input port, dropping > -> forwarding to mcast flood port > > Before the fix, when flooding igmp report traffic: > bridge("br0") > ------------- > 0. priority 32768 > NORMAL > >> mcast port is unknown, dropping the report > -> forwarding report to mcast flagged port > -> mcast port is input port, dropping the Report > -> forwarding report to mcast flagged port > > Add relevant cleanup and update unit tests. > > Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration changed.") > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Changes since v1: > - updated the test on report flooding, > Acked-by: Paolo Valerio <pvalerio@redhat.com>
On 10 Nov 2023, at 18:52, David Marchand wrote: > When a configuration change triggers an interface destruction/creation > (like for example, setting ofport_request), a port object may still be > referenced as a fport or a rport in the mdb. > > Before the fix, when flooding multicast traffic: > bridge("br0") > ------------- > 0. priority 32768 > NORMAL > -> forwarding to mcast group port > >> mcast flood port is unknown, dropping > -> mcast flood port is input port, dropping > -> forwarding to mcast flood port > > Before the fix, when flooding igmp report traffic: > bridge("br0") > ------------- > 0. priority 32768 > NORMAL > >> mcast port is unknown, dropping the report > -> forwarding report to mcast flagged port > -> mcast port is input port, dropping the Report > -> forwarding report to mcast flagged port > > Add relevant cleanup and update unit tests. > > Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration changed.") > Signed-off-by: David Marchand <david.marchand@redhat.com> Thanks for the fix, one small nit, and a request for a comment in the test case. Cheers, Eelco > --- > Changes since v1: > - updated the test on report flooding, > > --- > lib/mcast-snooping.c | 15 +++++++++++++++ > tests/mcast-snooping.at | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c > index 029ca28558..34755447f8 100644 > --- a/lib/mcast-snooping.c > +++ b/lib/mcast-snooping.c > @@ -948,6 +948,7 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, void *port) > { > struct mcast_group *g; > struct mcast_mrouter_bundle *m; > + struct mcast_port_bundle *p; nit: while we are at it, can we move this to reverse Christmas tree? struct mcast_mrouter_bundle *m; struct mcast_port_bundle *p; struct mcast_group *g; > > if (!mcast_snooping_enabled(ms)) { > return; > @@ -971,5 +972,19 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, void *port) > } > } > > + LIST_FOR_EACH_SAFE (p, node, &ms->fport_list) { > + if (p->port == port) { > + mcast_snooping_flush_port(p); > + ms->need_revalidate = true; > + } > + } > + > + LIST_FOR_EACH_SAFE (p, node, &ms->rport_list) { > + if (p->port == port) { > + mcast_snooping_flush_port(p); > + ms->need_revalidate = true; > + } > + } > + > ovs_rwlock_unlock(&ms->rwlock); > } > diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at > index b5474cf392..1ce31168e8 100644 > --- a/tests/mcast-snooping.at > +++ b/tests/mcast-snooping.at > @@ -207,6 +207,24 @@ Megaflow: recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e > Datapath actions: 1,2 > ]) > > +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4]) Can we add a comment here (and below) to indicate why we do this? Just to understand what we test here. > + > +AT_CHECK([ovs-appctl ofproto/trace "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"], [0], [dnl > +Flow: udp,in_port=3,vlan_tci=0x0000,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000 > + > +bridge("br0") > +------------- > + 0. priority 32768 > + NORMAL > + -> forwarding to mcast group port > + -> mcast flood port is input port, dropping > + -> forwarding to mcast flood port > + > +Final flow: unchanged > +Megaflow: recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no > +Datapath actions: 1,2 > +]) > + > OVS_VSWITCHD_STOP > AT_CLEANUP > > @@ -381,6 +399,26 @@ This flow is handled by the userspace slow path because it: > - Uses action(s) not supported by datapath. > ]) > > +AT_CHECK([ovs-vsctl set interface p3 ofport_request=4]) > + > +AT_CHECK([ovs-appctl ofproto/trace "in_port(1)" '01005E010101000C29A027A108004500001C000100004002CBAEAC10221EE001010112140CE9E0010101'], [0], [dnl > +Flow: ip,in_port=1,vlan_tci=0x0000,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_src=172.16.34.30,nw_dst=224.1.1.1,nw_proto=2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=18,tp_dst=20 > + > +bridge("br0") > +------------- > + 0. priority 32768 > + NORMAL > + -> forwarding report to mcast flagged port > + -> mcast port is input port, dropping the Report > + -> forwarding report to mcast flagged port > + > +Final flow: unchanged > +Megaflow: recirc_id=0,eth,ip,in_port=1,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_proto=2,nw_frag=no > +Datapath actions: 2,3 > +This flow is handled by the userspace slow path because it: > + - Uses action(s) not supported by datapath. > +]) > + > OVS_VSWITCHD_STOP > AT_CLEANUP > > -- > 2.41.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Nov 16, 2023 at 10:38 AM Eelco Chaudron <echaudro@redhat.com> wrote: > > diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at > > index b5474cf392..1ce31168e8 100644 > > --- a/tests/mcast-snooping.at > > +++ b/tests/mcast-snooping.at > > @@ -207,6 +207,24 @@ Megaflow: recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e > > Datapath actions: 1,2 > > ]) > > > > +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4]) > > Can we add a comment here (and below) to indicate why we do this? Just to understand what we test here. Wdyt of: +# Change p2 ofport to force a ofbundle change and check that the mdb contains +# no stale port.
On 16 Nov 2023, at 12:24, David Marchand wrote: > On Thu, Nov 16, 2023 at 10:38 AM Eelco Chaudron <echaudro@redhat.com> wrote: >>> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at >>> index b5474cf392..1ce31168e8 100644 >>> --- a/tests/mcast-snooping.at >>> +++ b/tests/mcast-snooping.at >>> @@ -207,6 +207,24 @@ Megaflow: recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e >>> Datapath actions: 1,2 >>> ]) >>> >>> +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4]) >> >> Can we add a comment here (and below) to indicate why we do this? Just to understand what we test here. > > Wdyt of: > +# Change p2 ofport to force a ofbundle change and check that the mdb contains > +# no stale port. Yes this looks good to me! Thanks, Eelco
diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c index 029ca28558..34755447f8 100644 --- a/lib/mcast-snooping.c +++ b/lib/mcast-snooping.c @@ -948,6 +948,7 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, void *port) { struct mcast_group *g; struct mcast_mrouter_bundle *m; + struct mcast_port_bundle *p; if (!mcast_snooping_enabled(ms)) { return; @@ -971,5 +972,19 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, void *port) } } + LIST_FOR_EACH_SAFE (p, node, &ms->fport_list) { + if (p->port == port) { + mcast_snooping_flush_port(p); + ms->need_revalidate = true; + } + } + + LIST_FOR_EACH_SAFE (p, node, &ms->rport_list) { + if (p->port == port) { + mcast_snooping_flush_port(p); + ms->need_revalidate = true; + } + } + ovs_rwlock_unlock(&ms->rwlock); } diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at index b5474cf392..1ce31168e8 100644 --- a/tests/mcast-snooping.at +++ b/tests/mcast-snooping.at @@ -207,6 +207,24 @@ Megaflow: recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e Datapath actions: 1,2 ]) +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4]) + +AT_CHECK([ovs-appctl ofproto/trace "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"], [0], [dnl +Flow: udp,in_port=3,vlan_tci=0x0000,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000 + +bridge("br0") +------------- + 0. priority 32768 + NORMAL + -> forwarding to mcast group port + -> mcast flood port is input port, dropping + -> forwarding to mcast flood port + +Final flow: unchanged +Megaflow: recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no +Datapath actions: 1,2 +]) + OVS_VSWITCHD_STOP AT_CLEANUP @@ -381,6 +399,26 @@ This flow is handled by the userspace slow path because it: - Uses action(s) not supported by datapath. ]) +AT_CHECK([ovs-vsctl set interface p3 ofport_request=4]) + +AT_CHECK([ovs-appctl ofproto/trace "in_port(1)" '01005E010101000C29A027A108004500001C000100004002CBAEAC10221EE001010112140CE9E0010101'], [0], [dnl +Flow: ip,in_port=1,vlan_tci=0x0000,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_src=172.16.34.30,nw_dst=224.1.1.1,nw_proto=2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=18,tp_dst=20 + +bridge("br0") +------------- + 0. priority 32768 + NORMAL + -> forwarding report to mcast flagged port + -> mcast port is input port, dropping the Report + -> forwarding report to mcast flagged port + +Final flow: unchanged +Megaflow: recirc_id=0,eth,ip,in_port=1,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_proto=2,nw_frag=no +Datapath actions: 2,3 +This flow is handled by the userspace slow path because it: + - Uses action(s) not supported by datapath. +]) + OVS_VSWITCHD_STOP AT_CLEANUP
When a configuration change triggers an interface destruction/creation (like for example, setting ofport_request), a port object may still be referenced as a fport or a rport in the mdb. Before the fix, when flooding multicast traffic: bridge("br0") ------------- 0. priority 32768 NORMAL -> forwarding to mcast group port >> mcast flood port is unknown, dropping -> mcast flood port is input port, dropping -> forwarding to mcast flood port Before the fix, when flooding igmp report traffic: bridge("br0") ------------- 0. priority 32768 NORMAL >> mcast port is unknown, dropping the report -> forwarding report to mcast flagged port -> mcast port is input port, dropping the Report -> forwarding report to mcast flagged port Add relevant cleanup and update unit tests. Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration changed.") Signed-off-by: David Marchand <david.marchand@redhat.com> --- Changes since v1: - updated the test on report flooding, --- lib/mcast-snooping.c | 15 +++++++++++++++ tests/mcast-snooping.at | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+)