[ovs-dev,v2,1/2] mcast-snooping: Test per port explicit flooding.

Various options affect how the mcast snooping module work.

When multicast snooping is enabled and a reporter is known, it is still
possible to flood associated packets to some other port via the
mcast-snooping-flood option.

If flooding unregistered traffic is disabled, it is still possible to
flood multicast traffic too with the mcast-snooping-flood option.

IGMP reports may have to be flooded to some ports explicitly with the
mcast-snooping-flood-reports option.

Test those parameters.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- fixed dest mac address,
- added tests for mcast-snooping-disable-flood-unregistered=true and
  mcast-snooping-flood-reports,

---
 tests/mcast-snooping.at | 280 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 280 insertions(+)

On Fri, Nov 10, 2023 at 06:52:03PM +0100, David Marchand wrote:
> Various options affect how the mcast snooping module work.
> 
> When multicast snooping is enabled and a reporter is known, it is still
> possible to flood associated packets to some other port via the
> mcast-snooping-flood option.
> 
> If flooding unregistered traffic is disabled, it is still possible to
> flood multicast traffic too with the mcast-snooping-flood option.
> 
> IGMP reports may have to be flooded to some ports explicitly with the
> mcast-snooping-flood-reports option.
> 
> Test those parameters.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Simon Horman <horms@ovn.org>

David Marchand <david.marchand@redhat.com> writes:

> Various options affect how the mcast snooping module work.
>
> When multicast snooping is enabled and a reporter is known, it is still
> possible to flood associated packets to some other port via the
> mcast-snooping-flood option.
>
> If flooding unregistered traffic is disabled, it is still possible to
> flood multicast traffic too with the mcast-snooping-flood option.
>
> IGMP reports may have to be flooded to some ports explicitly with the
> mcast-snooping-flood-reports option.
>
> Test those parameters.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Thanks David.
The patch lgtm.

Acked-by: Paolo Valerio <pvalerio@redhat.com>

On 10 Nov 2023, at 18:52, David Marchand wrote:

> Various options affect how the mcast snooping module work.
>
> When multicast snooping is enabled and a reporter is known, it is still
> possible to flood associated packets to some other port via the
> mcast-snooping-flood option.
>
> If flooding unregistered traffic is disabled, it is still possible to
> flood multicast traffic too with the mcast-snooping-flood option.
>
> IGMP reports may have to be flooded to some ports explicitly with the
> mcast-snooping-flood-reports option.
>
> Test those parameters.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Thanks David for adding more test cases!

One small nit, and a question below.

Cheers,

Eelco


> ---
> Changes since v1:
> - fixed dest mac address,
> - added tests for mcast-snooping-disable-flood-unregistered=true and
>   mcast-snooping-flood-reports,
>
> ---
>  tests/mcast-snooping.at | 280 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 280 insertions(+)
>
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index d5b7c4774c..b5474cf392 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -105,6 +105,286 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +
> +AT_SETUP([mcast - check multicast per port flooding])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +    ovs-vsctl set bridge br0 \
> +    datapath_type=dummy \
> +    mcast_snooping_enable=true \
> +    other-config:mcast-snooping-disable-flood-unregistered=false
> +], [0])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +AT_CHECK([
> +    ovs-vsctl add-port br0 p1 \
> +    -- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:01 ofport_request=1 \
> +    -- add-port br0 p2 \
> +    -- set Interface p2 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 ofport_request=2 \
> +    -- add-port br0 p3 \
> +    -- set Interface p3 type=dummy other-config:hwaddr=aa:55:aa:55:00:03 ofport_request=3 \
> +], [0])
> +
> +ovs-appctl time/stop
> +
> +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], [stdout])
> +AT_CHECK([grep -v 'Datapath actions:' stdout], [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
> +     -> unregistered multicast, flooding
> +
> +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
> +])
> +AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" | sort -n], [0], [dnl
> +1
> +2
> +100
> +])
> +
> +# send report packets

Please add capital and dots to all comments.

+# Send report packets.

> +AT_CHECK([
> +    ovs-appctl netdev-dummy/receive p1  \
> +        '01005E010101000C29A027A108004500001C000100004002CBAEAC10221EE001010112140CE9E0010101'
> +], [0])
> +AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
> + port  VLAN  GROUP                Age
> +    1     0  224.1.1.1           0
> +])
> +
> +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
> +
> +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
> +])
> +
> +AT_CHECK([ovs-vsctl set port p2 other_config:mcast-snooping-flood=true])
> +
> +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
> +     -> 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


Are we sure the order here is always 1,2 vs the first test you sorted them? Same for all the other multi-port tests below?

I did run the test 200+ times, and it seems ok. Trying to understand this, as I can see the first one reporting 100,1,2 and 100,2,1.

> +])
> +
> +AT_CHECK([ovs-vsctl set port p3 other_config:mcast-snooping-flood=true])
> +
> +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
> +     -> forwarding to mcast flood port
> +     -> mcast flood port is input port, dropping
> +
> +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
> +
> +
> +AT_SETUP([mcast - check multicast per port flooding (unregistered flood disabled)])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +    ovs-vsctl set bridge br0 \
> +    datapath_type=dummy \
> +    mcast_snooping_enable=true \
> +    other-config:mcast-snooping-disable-flood-unregistered=true
> +], [0])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +AT_CHECK([
> +    ovs-vsctl add-port br0 p1 \
> +    -- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:01 ofport_request=1 \
> +    -- add-port br0 p2 \
> +    -- set Interface p2 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 ofport_request=2 \
> +    -- add-port br0 p3 \
> +    -- set Interface p3 type=dummy other-config:hwaddr=aa:55:aa:55:00:03 ofport_request=3 \
> +], [0])
> +
> +ovs-appctl time/stop
> +
> +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
> +
> +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: drop
> +])
> +
> +AT_CHECK([ovs-vsctl set port p2 other_config:mcast-snooping-flood=true])
> +
> +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 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: 2
> +])
> +
> +AT_CHECK([ovs-vsctl set port p3 other_config:mcast-snooping-flood=true])
> +
> +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 flood port
> +     -> mcast flood port is input port, dropping
> +
> +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: 2
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +
> +AT_SETUP([mcast - check reports per port flooding])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +    ovs-vsctl set bridge br0 \
> +    datapath_type=dummy \
> +    mcast_snooping_enable=true \
> +    other-config:mcast-snooping-disable-flood-unregistered=false
> +], [0])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +AT_CHECK([
> +    ovs-vsctl add-port br0 p1 \
> +    -- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:01 ofport_request=1 \
> +    -- add-port br0 p2 \
> +    -- set Interface p2 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 ofport_request=2 \
> +    -- add-port br0 p3 \
> +    -- set Interface p3 type=dummy other-config:hwaddr=aa:55:aa:55:00:03 ofport_request=3 \
> +], [0])
> +
> +ovs-appctl time/stop
> +
> +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
> +     -> learned that 00:0c:29:a0:27:a1 is on port p1 in VLAN 0
> +     -> multicast snooping learned that 224.1.1.1 is on port p1 in VLAN 0
> +
> +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: drop
> +This flow is handled by the userspace slow path because it:
> +  - Uses action(s) not supported by datapath.
> +])
> +
> +AT_CHECK([ovs-vsctl set port p3 other_config:mcast-snooping-flood-reports=true])
> +
> +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
> +
> +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: 3
> +This flow is handled by the userspace slow path because it:
> +  - Uses action(s) not supported by datapath.
> +])
> +
> +AT_CHECK([ovs-vsctl set port p2 other_config:mcast-snooping-flood-reports=true])
> +
> +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
> +     -> 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: 3,2
> +This flow is handled by the userspace slow path because it:
> +  - Uses action(s) not supported by datapath.
> +])
> +
> +AT_CHECK([ovs-vsctl set port p1 other_config:mcast-snooping-flood-reports=true])
> +
> +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
> +     -> forwarding report to mcast flagged port
> +     -> mcast port is input port, dropping the Report
> +
> +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: 3,2
> +This flow is handled by the userspace slow path because it:
> +  - Uses action(s) not supported by datapath.
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +
>  AT_SETUP([mcast - delete the port mdb when vlan configuration changed])
>  OVS_VSWITCHD_START([])
>
> -- 
> 2.41.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Hello Eelco,

On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron <echaudro@redhat.com> wrote:

[snip]

> > +bridge("br0")
> > +-------------
> > + 0. priority 32768
> > +    NORMAL
> > +     -> forwarding to mcast group port
> > +     -> 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
>
>
> Are we sure the order here is always 1,2 vs the first test you sorted them? Same for all the other multi-port tests below?
>
> I did run the test 200+ times, and it seems ok. Trying to understand this, as I can see the first one reporting 100,1,2 and 100,2,1.

        struct mcast_output out = MCAST_OUTPUT_INIT;
...
        if (grp) {
            xlate_normal_mcast_send_group(ctx, ms, grp, in_xbundle, &out);
            xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, &out);
            xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan,
                                             &out);
...
        mcast_output_finish(ctx, &out, in_xbundle, &xvlan);

With:
static void
mcast_output_finish(struct xlate_ctx *ctx, struct mcast_output *out,
                    struct xbundle *in_xbundle, struct xvlan *xvlan)
{
    if (out->flood) {
        xlate_normal_flood(ctx, in_xbundle, xvlan);
    } else {
        for (size_t i = 0; i < out->n; i++) {
            output_normal(ctx, out->xbundles[i], xvlan);
        }
    }
...


In this case, there is no flooding (contrary to previous tests) over
all the ports from this bridge.
There is only one "group" port and one "flood" port and the order is fixed.

On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron <echaudro@redhat.com> wrote:
> On 10 Nov 2023, at 18:52, David Marchand wrote:
> > +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
> > +])
> > +AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" | sort -n], [0], [dnl
> > +1
> > +2
> > +100
> > +])
> > +
> > +# send report packets
>
> Please add capital and dots to all comments.

I don't mind, but the rest of this file is not consistent to this convention.

$ git grep \\# origin/master -- tests/mcast-snooping.at
...
origin/master:tests/mcast-snooping.at:# send report packets
origin/master:tests/mcast-snooping.at:# send query packets
origin/master:tests/mcast-snooping.at:# send report packets
origin/master:tests/mcast-snooping.at:# send query packets

On 16 Nov 2023, at 12:10, David Marchand wrote:

> Hello Eelco,
>
> On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
> [snip]
>
>>> +bridge("br0")
>>> +-------------
>>> + 0. priority 32768
>>> +    NORMAL
>>> +     -> forwarding to mcast group port
>>> +     -> 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
>>
>>
>> Are we sure the order here is always 1,2 vs the first test you sorted them? Same for all the other multi-port tests below?
>>
>> I did run the test 200+ times, and it seems ok. Trying to understand this, as I can see the first one reporting 100,1,2 and 100,2,1.
>
>         struct mcast_output out = MCAST_OUTPUT_INIT;
> ...
>         if (grp) {
>             xlate_normal_mcast_send_group(ctx, ms, grp, in_xbundle, &out);
>             xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, &out);
>             xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan,
>                                              &out);
> ...
>         mcast_output_finish(ctx, &out, in_xbundle, &xvlan);
>
> With:
> static void
> mcast_output_finish(struct xlate_ctx *ctx, struct mcast_output *out,
>                     struct xbundle *in_xbundle, struct xvlan *xvlan)
> {
>     if (out->flood) {
>         xlate_normal_flood(ctx, in_xbundle, xvlan);
>     } else {
>         for (size_t i = 0; i < out->n; i++) {
>             output_normal(ctx, out->xbundles[i], xvlan);
>         }
>     }
> ...
>
>
> In this case, there is no flooding (contrary to previous tests) over
> all the ports from this bridge.
> There is only one "group" port and one "flood" port and the order is fixed.

Thanks, now it makes sense :)

//Eelco

On 16 Nov 2023, at 12:13, David Marchand wrote:

> On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>> On 10 Nov 2023, at 18:52, David Marchand wrote:
>>> +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
>>> +])
>>> +AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" | sort -n], [0], [dnl
>>> +1
>>> +2
>>> +100
>>> +])
>>> +
>>> +# send report packets
>>
>> Please add capital and dots to all comments.
>
> I don't mind, but the rest of this file is not consistent to this convention.
>
> $ git grep \\# origin/master -- tests/mcast-snooping.at
> ...
> origin/master:tests/mcast-snooping.at:# send report packets
> origin/master:tests/mcast-snooping.at:# send query packets
> origin/master:tests/mcast-snooping.at:# send report packets
> origin/master:tests/mcast-snooping.at:# send query packets

Agreed, but we like to be adherent for new additions.

On 11/10/23 18:52, David Marchand wrote:
> Various options affect how the mcast snooping module work.
> 
> When multicast snooping is enabled and a reporter is known, it is still
> possible to flood associated packets to some other port via the
> mcast-snooping-flood option.
> 
> If flooding unregistered traffic is disabled, it is still possible to
> flood multicast traffic too with the mcast-snooping-flood option.
> 
> IGMP reports may have to be flooded to some ports explicitly with the
> mcast-snooping-flood-reports option.
> 
> Test those parameters.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - fixed dest mac address,
> - added tests for mcast-snooping-disable-flood-unregistered=true and
>   mcast-snooping-flood-reports,
> 
> ---
>  tests/mcast-snooping.at | 280 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 280 insertions(+)
> 
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index d5b7c4774c..b5474cf392 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -105,6 +105,286 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +
> +AT_SETUP([mcast - check multicast per port flooding])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +    ovs-vsctl set bridge br0 \
> +    datapath_type=dummy \
> +    mcast_snooping_enable=true \
> +    other-config:mcast-snooping-disable-flood-unregistered=false

Nit:
Not a full review, but in case you're sending a new version for Eelco's
comments, please, add more indentation to the 3 lines above, so they
are not on the same level with ovs-vsctl.

Best regards, Ilya Maximets.

On Thu, Nov 16, 2023 at 12:32 PM Ilya Maximets <i.maximets@ovn.org> wrote:
> > +AT_CHECK([
> > +    ovs-vsctl set bridge br0 \
> > +    datapath_type=dummy \
> > +    mcast_snooping_enable=true \
> > +    other-config:mcast-snooping-disable-flood-unregistered=false
>
> Nit:
> Not a full review, but in case you're sending a new version for Eelco's
> comments, please, add more indentation to the 3 lines above, so they
> are not on the same level with ovs-vsctl.

I did not see this comment.
Well, I'll wait for a full review before sending a new revision...