diff mbox series

[v2,net-next,6/6] selftests: forwarding: add dynamic FDB test

Message ID 20230318141010.513424-7-netdev@kapio-technology.com (mailing list archive)
State New, archived
Headers show
Series ATU and FDB synchronization on locked ports | expand

Commit Message

Hans Schultz March 18, 2023, 2:10 p.m. UTC
Test FDB ageing of user entry created by

bridge fdb replace ADDR dev <DEV> master dynamic

Use LOW_AGEING_TIME variable in forwarding.config to set a low ageing time.
Beware, DSA might not accept the ageing time you want. Check the
age_time_coeff value for your driver.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 .../net/forwarding/bridge_locked_port.sh      | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Ido Schimmel March 20, 2023, 8:44 a.m. UTC | #1
On Sat, Mar 18, 2023 at 03:10:10PM +0100, Hans J. Schultz wrote:
> +# Test of dynamic FDB entries.
> +locked_port_dyn_fdb()
> +{
> +	local mac=00:01:02:03:04:05
> +	local ageing_time
> +
> +	RET=0
> +	ageing_time=$(bridge_ageing_time_get br0)
> +	tc qdisc add dev $swp2 clsact
> +	ip link set dev br0 type bridge ageing_time $LOW_AGEING_TIME
> +	bridge link set dev $swp1 learning on locked on
> +
> +	bridge fdb replace $mac dev $swp1 master dynamic
> +	tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
> +		dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
> +
> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q

Packets should be injected via $h1, not $swp1. See other test cases in
this file.

> +	tc_check_packets "dev $swp2 egress" 1 1
> +	check_err $? "Packet not seen on egress after adding dynamic FDB"

Does this actually work? The packet is transmitted via $swp1, I don't
understand how it can arrive at the egress of $swp2.

> +
> +	sleep $((LOW_AGEING_TIME / 100 + 10))
> +
> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> +	tc_check_packets "dev $swp2 egress" 1 1
> +	check_fail $? "Dynamic FDB entry did not age out"

Shouldn't this be check_err()? After the FDB entry was aged you want to
make sure that packets received via $swp1 with SMAC being $mac are no
longer forwarded by the bridge.

Also, I suggest executing 'bridge fdb get' to make sure the entry is no
longer present in the bridge FDB.

> +
> +	ip link set dev br0 type bridge ageing_time $ageing_time
> +	bridge link set dev $swp1 learning off locked off
> +	tc qdisc del dev $swp2 clsact
> +
> +	log_test "Locked port dyn FDB"
> +}
> +
>  trap cleanup EXIT
>  
>  setup_prepare
> -- 
> 2.34.1
>
Hans Schultz March 26, 2023, 3:41 p.m. UTC | #2
On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
>> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
>> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
>> +	tc_check_packets "dev $swp2 egress" 1 1
>> +	check_fail $? "Dynamic FDB entry did not age out"
>
> Shouldn't this be check_err()? After the FDB entry was aged you want to
> make sure that packets received via $swp1 with SMAC being $mac are no
> longer forwarded by the bridge.

I was thinking that check_fail() will pass when tc_check_packets() does
not see any packets, thus the test passing here when no packets are forwarded?

>
> Also, I suggest executing 'bridge fdb get' to make sure the entry is no
> longer present in the bridge FDB.
>
Ido Schimmel March 28, 2023, 4:40 p.m. UTC | #3
On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> >> +	tc_check_packets "dev $swp2 egress" 1 1
> >> +	check_fail $? "Dynamic FDB entry did not age out"
> >
> > Shouldn't this be check_err()? After the FDB entry was aged you want to
> > make sure that packets received via $swp1 with SMAC being $mac are no
> > longer forwarded by the bridge.
> 
> I was thinking that check_fail() will pass when tc_check_packets() does
> not see any packets, thus the test passing here when no packets are forwarded?

What do you mean by "I was *thinking*"? How is it possible that you are
submitting a selftest that you didn't bother running?!

I see you trimmed my earlier question: "Does this actually work?"

I tried it and it passed:

# ./bridge_locked_port.sh                         
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]            
TEST: Locked port MAB                                               [ OK ]            
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
TEST: Locked port MAB FDB flush                                     [ OK ]

And I couldn't understand how that's even possible. Then I realized that
the entire test is dead code because the patch is missing this
fundamental hunk:

```
diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index dbc7017fd45d..5bf6b2aa1098 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -9,6 +9,7 @@ ALL_TESTS="
        locked_port_mab_roam
        locked_port_mab_config
        locked_port_mab_flush
+       locked_port_dyn_fdb
 "
 
 NUM_NETIFS=4
```

Which tells me that you didn't even try running it once. Now the test
failed as I expected:

# ./bridge_locked_port.sh                         
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]
TEST: Locked port MAB                                               [ OK ]
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]            
TEST: Locked port MAB FDB flush                                     [ OK ]
TEST: Locked port dyn FDB                                           [FAIL]            
        Packet not seen on egress after adding dynamic FDB

Fixed by:

```
@@ -336,7 +337,7 @@ locked_port_dyn_fdb()
        tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
                dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
 
-       $MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
+       $MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
                -a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
        tc_check_packets "dev $swp2 egress" 1 1
        check_err $? "Packet not seen on egress after adding dynamic FDB"
```

Ran it again and it failed because of the second issue I pointed out:

# ./bridge_locked_port.sh 
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]
TEST: Locked port MAB                                               [ OK ]
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
TEST: Locked port MAB FDB flush                                     [ OK ]
TEST: Locked port dyn FDB                                           [FAIL]
        Dynamic FDB entry did not age out                                             

Fixed by:

```
@@ -346,7 +347,7 @@ locked_port_dyn_fdb()
        $MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
                -a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
        tc_check_packets "dev $swp2 egress" 1 1
-       check_fail $? "Dynamic FDB entry did not age out"
+       check_err $? "Dynamic FDB entry did not age out"
 
        ip link set dev br0 type bridge ageing_time $ageing_time
        bridge link set dev $swp1 learning off locked off
```

# ./bridge_locked_port.sh 
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]
TEST: Locked port MAB                                               [ OK ]
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
TEST: Locked port MAB FDB flush                                     [ OK ]
TEST: Locked port dyn FDB                                           [ OK ]

Sigh
Hans Schultz March 28, 2023, 7:30 p.m. UTC | #4
On Tue, Mar 28, 2023 at 19:40, Ido Schimmel <idosch@nvidia.com> wrote:
> On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
>> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
>> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
>> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
>> >> +	tc_check_packets "dev $swp2 egress" 1 1
>> >> +	check_fail $? "Dynamic FDB entry did not age out"
>> >
>> > Shouldn't this be check_err()? After the FDB entry was aged you want to
>> > make sure that packets received via $swp1 with SMAC being $mac are no
>> > longer forwarded by the bridge.
>> 
>> I was thinking that check_fail() will pass when tc_check_packets() does
>> not see any packets, thus the test passing here when no packets are forwarded?
>
> What do you mean by "I was *thinking*"? How is it possible that you are
> submitting a selftest that you didn't bother running?!
>

Sorry, but I have sent you several emails telling you about the problems
I have with running the selftests due to changes in the phy etc. Maybe
you have just not received all those emails?

Have you checked spamfilters?

With the kernels now, I cannot even test with the software bridge and
selftests as the compile fails - probably due to changes in uapi headers
compared to what the packages my system uses expects.
Ido Schimmel March 30, 2023, 6:37 a.m. UTC | #5
On Tue, Mar 28, 2023 at 09:30:08PM +0200, Hans Schultz wrote:
> On Tue, Mar 28, 2023 at 19:40, Ido Schimmel <idosch@nvidia.com> wrote:
> > On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
> >> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
> >> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> >> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> >> >> +	tc_check_packets "dev $swp2 egress" 1 1
> >> >> +	check_fail $? "Dynamic FDB entry did not age out"
> >> >
> >> > Shouldn't this be check_err()? After the FDB entry was aged you want to
> >> > make sure that packets received via $swp1 with SMAC being $mac are no
> >> > longer forwarded by the bridge.
> >> 
> >> I was thinking that check_fail() will pass when tc_check_packets() does
> >> not see any packets, thus the test passing here when no packets are forwarded?
> >
> > What do you mean by "I was *thinking*"? How is it possible that you are
> > submitting a selftest that you didn't bother running?!
> >
> 
> Sorry, but I have sent you several emails telling you about the problems
> I have with running the selftests due to changes in the phy etc. Maybe
> you have just not received all those emails?
> 
> Have you checked spamfilters?
> 
> With the kernels now, I cannot even test with the software bridge and
> selftests as the compile fails - probably due to changes in uapi headers
> compared to what the packages my system uses expects.

My spam filters are fine. I saw your emails where you basically said
that you are too lazy to setup a VM to test your patches and that your
time is more valuable than mine, which is why I should be testing them.
Stop making your problems our problems. It's hardly the first time. If
you are unable to test your patches, then invest the time in fixing your
setup instead of submitting completely broken patches and making it our
problem to test and fix them. I refuse to invest time in reviewing /
testing / reworking your submissions as long as you insist on doing less
than the bare minimum.

Good luck
Hans Schultz March 30, 2023, 10:29 a.m. UTC | #6
On Thu, Mar 30, 2023 at 09:37, Ido Schimmel <idosch@nvidia.com> wrote:
> On Tue, Mar 28, 2023 at 09:30:08PM +0200, Hans Schultz wrote:
>> 
>> Sorry, but I have sent you several emails telling you about the problems
>> I have with running the selftests due to changes in the phy etc. Maybe
>> you have just not received all those emails?
>> 
>> Have you checked spamfilters?
>> 
>> With the kernels now, I cannot even test with the software bridge and
>> selftests as the compile fails - probably due to changes in uapi headers
>> compared to what the packages my system uses expects.
>
> My spam filters are fine. I saw your emails where you basically said
> that you are too lazy to setup a VM to test your patches and that your
> time is more valuable than mine, which is why I should be testing them.
> Stop making your problems our problems. It's hardly the first time. If
> you are unable to test your patches, then invest the time in fixing your
> setup instead of submitting completely broken patches and making it our
> problem to test and fix them. I refuse to invest time in reviewing /
> testing / reworking your submissions as long as you insist on doing less
> than the bare minimum.
>
> Good luck

I never said or indicated that my time is more valuable than yours. I
have a VM to run these things that some have spent countless hours to
develop with the right tools etc installed and set up. Fixing that
system will take quite many hours for me, so I am asking for some simple
assistance from someone who already has a system running supporting the
newest kernel.

Alternatively if there is an open sourced system available that would be
great.

As this patch-set is for the community and some companies that would
like to use it and not for myself, I am asking for some help from the
community with a task that when someone has the system in place should
not take more than 15-20 minutes, maybe even less.
Nikolay Aleksandrov March 30, 2023, 10:45 a.m. UTC | #7
On 30/03/2023 13:29, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 09:37, Ido Schimmel <idosch@nvidia.com> wrote:
>> On Tue, Mar 28, 2023 at 09:30:08PM +0200, Hans Schultz wrote:
>>>
>>> Sorry, but I have sent you several emails telling you about the problems
>>> I have with running the selftests due to changes in the phy etc. Maybe
>>> you have just not received all those emails?
>>>
>>> Have you checked spamfilters?
>>>
>>> With the kernels now, I cannot even test with the software bridge and
>>> selftests as the compile fails - probably due to changes in uapi headers
>>> compared to what the packages my system uses expects.
>>
>> My spam filters are fine. I saw your emails where you basically said
>> that you are too lazy to setup a VM to test your patches and that your
>> time is more valuable than mine, which is why I should be testing them.
>> Stop making your problems our problems. It's hardly the first time. If
>> you are unable to test your patches, then invest the time in fixing your
>> setup instead of submitting completely broken patches and making it our
>> problem to test and fix them. I refuse to invest time in reviewing /
>> testing / reworking your submissions as long as you insist on doing less
>> than the bare minimum.
>>
>> Good luck
> 
> I never said or indicated that my time is more valuable than yours. I
> have a VM to run these things that some have spent countless hours to
> develop with the right tools etc installed and set up. Fixing that
> system will take quite many hours for me, so I am asking for some simple
> assistance from someone who already has a system running supporting the
> newest kernel.
> 
> Alternatively if there is an open sourced system available that would be
> great.
> 
> As this patch-set is for the community and some companies that would
> like to use it and not for myself, I am asking for some help from the
> community with a task that when someone has the system in place should
> not take more than 15-20 minutes, maybe even less.

I'm sorry but this is absolutely the wrong way to go about this. Your setup's
problems are yours to figure out and fix, if you are going to send *any* future
patches make absolutely sure they build, run and work as intended.
Please do not send any future patches without them being fully tested and, as
Ido mentioned, cover at least the bare minimum for a submission.

Thanks,
 Nik
Hans Schultz March 30, 2023, 7:07 p.m. UTC | #8
On Tue, Mar 28, 2023 at 19:40, Ido Schimmel <idosch@nvidia.com> wrote:
> On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
>> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
>> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
>> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
>> >> +	tc_check_packets "dev $swp2 egress" 1 1
>> >> +	check_fail $? "Dynamic FDB entry did not age out"
>> >
>> > Shouldn't this be check_err()? After the FDB entry was aged you want to
>> > make sure that packets received via $swp1 with SMAC being $mac are no
>> > longer forwarded by the bridge.
>> 
>> I was thinking that check_fail() will pass when tc_check_packets() does
>> not see any packets, thus the test passing here when no packets are forwarded?
>
> What do you mean by "I was *thinking*"? How is it possible that you are
> submitting a selftest that you didn't bother running?!
>
> I see you trimmed my earlier question: "Does this actually work?"
>
> I tried it and it passed:
>
> # ./bridge_locked_port.sh                         
> TEST: Locked port ipv4                                              [ OK ]
> TEST: Locked port ipv6                                              [ OK ]
> TEST: Locked port vlan                                              [ OK ]            
> TEST: Locked port MAB                                               [ OK ]            
> TEST: Locked port MAB roam                                          [ OK ]
> TEST: Locked port MAB configuration                                 [ OK ]
> TEST: Locked port MAB FDB flush                                     [ OK ]
>
> And I couldn't understand how that's even possible. Then I realized that
> the entire test is dead code because the patch is missing this
> fundamental hunk:
>
> ```
> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> index dbc7017fd45d..5bf6b2aa1098 100755
> --- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> @@ -9,6 +9,7 @@ ALL_TESTS="
>         locked_port_mab_roam
>         locked_port_mab_config
>         locked_port_mab_flush
> +       locked_port_dyn_fdb
>  "
>  
>  NUM_NETIFS=4
> ```
>
> Which tells me that you didn't even try running it once.

Not true, it reveals that I forgot to put it in the patch, that's all. As
I cannot run several of these tests because of memory constraints I link
the file to a copy in a rw area where I modify the list and just run one
of the subtests at a time. If I try to run the whole it always fails
after a couple of sub-tests with an error.

It seems to me that these scripts are quite memory consuming as they
accumulate memory consuption in relation to what is loaded along the
way. A major problem with my system.
Vladimir Oltean March 30, 2023, 7:27 p.m. UTC | #9
On Thu, Mar 30, 2023 at 09:07:53PM +0200, Hans Schultz wrote:
> Not true, it reveals that I forgot to put it in the patch, that's all. As
> I cannot run several of these tests because of memory constraints I link
> the file to a copy in a rw area where I modify the list and just run one
> of the subtests at a time. If I try to run the whole it always fails
> after a couple of sub-tests with an error.
> 
> It seems to me that these scripts are quite memory consuming as they
> accumulate memory consuption in relation to what is loaded along the
> way. A major problem with my system.

I'm sorry for perhaps asking something entirely obvious, but have you tried:

kernel-dir $ rsync -avr tools/testing/selftests/ root@$board:selftests/
board $ cd selftests/drivers/net/dsa/
board $ ./bridge_locked_port.sh lan0 lan1 lan2 lan3

?

This is how I always run them, and it worked fine with both Debian
(where it's easy to add missing packages to the rootfs) or with a more
embedded-oriented Buildroot.
Hans Schultz March 31, 2023, 7:43 a.m. UTC | #10
On Thu, Mar 30, 2023 at 22:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 09:07:53PM +0200, Hans Schultz wrote:
>> Not true, it reveals that I forgot to put it in the patch, that's all. As
>> I cannot run several of these tests because of memory constraints I link
>> the file to a copy in a rw area where I modify the list and just run one
>> of the subtests at a time. If I try to run the whole it always fails
>> after a couple of sub-tests with an error.
>> 
>> It seems to me that these scripts are quite memory consuming as they
>> accumulate memory consuption in relation to what is loaded along the
>> way. A major problem with my system.
>
> I'm sorry for perhaps asking something entirely obvious, but have you tried:
>
> kernel-dir $ rsync -avr tools/testing/selftests/ root@$board:selftests/
> board $ cd selftests/drivers/net/dsa/
> board $ ./bridge_locked_port.sh lan0 lan1 lan2 lan3
>
> ?
>
> This is how I always run them, and it worked fine with both Debian
> (where it's easy to add missing packages to the rootfs) or with a more
> embedded-oriented Buildroot.

I am not entirely clear of your idea. You need somehow to boot into a
system with the patched net-next kernel or you have a virtual machine
boot into a virtual OS. I guess it is the last option you refer to using
Debian?
Hans Schultz March 31, 2023, 8:06 a.m. UTC | #11
On Thu, Mar 30, 2023 at 22:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 09:07:53PM +0200, Hans Schultz wrote:
>> Not true, it reveals that I forgot to put it in the patch, that's all. As
>> I cannot run several of these tests because of memory constraints I link
>> the file to a copy in a rw area where I modify the list and just run one
>> of the subtests at a time. If I try to run the whole it always fails
>> after a couple of sub-tests with an error.
>> 
>> It seems to me that these scripts are quite memory consuming as they
>> accumulate memory consuption in relation to what is loaded along the
>> way. A major problem with my system.
>
> I'm sorry for perhaps asking something entirely obvious, but have you tried:
>
> kernel-dir $ rsync -avr tools/testing/selftests/ root@$board:selftests/
> board $ cd selftests/drivers/net/dsa/
> board $ ./bridge_locked_port.sh lan0 lan1 lan2 lan3
>
> ?
>
> This is how I always run them, and it worked fine with both Debian
> (where it's easy to add missing packages to the rootfs) or with a more
> embedded-oriented Buildroot.

The memory problems are of course on the embedded target. In that case I
think it would be a very good idea to do something to design the system
better, so that it frees memory between the subtests.

If all tests are always run on the bridge only, I think they don't make
much sense as these patchsets are directed towards switchcores.
Vladimir Oltean March 31, 2023, 8:58 a.m. UTC | #12
On Fri, Mar 31, 2023 at 09:43:34AM +0200, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 22:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> > This is how I always run them, and it worked fine with both Debian
> > (where it's easy to add missing packages to the rootfs) or with a more
> > embedded-oriented Buildroot.
> 
> I am not entirely clear of your idea. You need somehow to boot into a
> system with the patched net-next kernel

You have to do that anyway for any kind of kernel work, no?

> or you have a virtual machine boot into a virtual OS. I guess it is
> the last option you refer to using Debian?

You could do that too, but you don't have to. Debian, like many other
Linux distributions, supports a wide variety of CPU architectures; it
can be run on embedded systems just as well as on desktop PCs or VMs.
I didn't say you have to use Debian, though, I just said I ran the
selftests on a Debian-based rootfs and that it was easy to prepare the
environment there. The Debian rootfs and the selftests were deployed to
the target board with the DSA switch on it, in case that wasn't clear.
Vladimir Oltean March 31, 2023, 9:37 a.m. UTC | #13
On Fri, Mar 31, 2023 at 10:06:34AM +0200, Hans Schultz wrote:
> The memory problems are of course on the embedded target. In that case I
> think it would be a very good idea to do something to design the system
> better, so that it frees memory between the subtests.

People like Martin Blumenstingl have managed to deploy and run the
networking kselftests on OpenWRT, which typically runs on very
resource-constrained embedded devices.
https://lore.kernel.org/netdev/CAFBinCDX5XRyMyOd-+c_Zkn6dawtBpQ9DaPkA4FDC5agL-t8CA@mail.gmail.com/
https://lore.kernel.org/netdev/20220707135532.1783925-1-martin.blumenstingl@googlemail.com/

Considering that, you'll have to come with a much more concrete description
of why the system should be "designed better" and "free memory between
subtests" (what memory?!) before you could run it on your target system.

Either that, or at least take into serious consideration the fact that you
may be hung up on doing something which isn't necessary for the end goal.

I simply have no clue what you're talking about. It's as if we're talking
about completely different things.

> If all tests are always run on the bridge only, I think they don't make
> much sense as these patchsets are directed towards switchcores.

Is this supposed to mean something, or is it just a random thought you
had, that you believed it would be good to share with us?

The tools/testing/selftests/net/forwarding/lib.sh central framework has
the NETIF_TYPE and NETIF_CREATE variables, which indicate that by default,
veth interfaces are created. When running a bridge selftest with veth as
bridge ports, indeed software bridging should take place, and those
selftests should work fine. In Linux, the software behavior represents a
model for the offload behavior, since offloads are 100% transparent to
the user most of the time.

Below in lib.sh, there is a line which sources "$relative_path/forwarding.config",
a file which can contain customizations of the default variables used by
the framework. Even though it isn't strictly necessary to put the
customized bash variables in a forwarding.config file, it is more
convenient to do this than to specify them as environment variables.

If you "cd tools/testing/selftests/drivers/net/dsa/", you will find
precisely such a forwarding.config file there, which contains the line
"NETIF_CREATE=no", which means that when you run the symlinked sub-group
of forwarding tests relevant to DSA from this folder, the expectation is
that the bridge ports are not veth interfaces created for the test, but
rather, physical ports.

So, by running the command I posted in the earlier email, you actually
run it on the physical DSA user port interfaces, and it should pass
there too. This is based on the equivalency principle between the
software and the hardware data paths that I was talking about.

If you're actively and repeatedly making an effort to work with your eyes
closed, and then build strawmen around the fact that you don't see, then
you're not going to get very friendly reactions from people, me included,
who explain things to you that pertain to your due diligence. This is
because these people know the things that they're explaining to you out
of their own due diligence, and, as a result, are not easily fooled by
your childish excuses.
Hans Schultz March 31, 2023, 12:43 p.m. UTC | #14
On Fri, Mar 31, 2023 at 12:37, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> So, by running the command I posted in the earlier email, you actually
> run it on the physical DSA user port interfaces, and it should pass
> there too.

Okay, that sounds like a good idea which I have not done before. I am
seeing how I can install Debian in an Qemu or VMWare setup to be able to
test that way.

> This is based on the equivalency principle between the
> software and the hardware data paths that I was talking about.
>
> If you're actively and repeatedly making an effort to work with your eyes
> closed, and then build strawmen around the fact that you don't see, then
> you're not going to get very friendly reactions from people, me included,
> who explain things to you that pertain to your due diligence. This is
> because these people know the things that they're explaining to you out
> of their own due diligence, and, as a result, are not easily fooled by
> your childish excuses.

I am not coming with excuses here, and certainly not childish ones at
that either. I am just pointing out that on my device the tests don't
run well because of memory shortage and my reasoning why I think it is
so.
I will as long as the system is as it is with these selftests, just run
single subtests at a time on target, but if I have new phy problems like
the one you have seen I have had before, then testing on target becomes
off limits.
Vladimir Oltean April 6, 2023, 3:24 p.m. UTC | #15
On Fri, Mar 31, 2023 at 02:43:11PM +0200, Hans Schultz wrote:
> I will as long as the system is as it is with these selftests, just run
> single subtests at a time on target, but if I have new phy problems like
> the one you have seen I have had before, then testing on target becomes
> off limits.

Please open a dedicated communication channel (separate email thread on
netdev@vger.kernel.org) with the appropriate maintainers for the PHY
code that is failing for you in To:, and you will get the help that you
need to resolve that and to be able to test on the target board.
Hans Schultz April 6, 2023, 4:26 p.m. UTC | #16
On Thu, Apr 06, 2023 at 18:24, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Fri, Mar 31, 2023 at 02:43:11PM +0200, Hans Schultz wrote:
>> I will as long as the system is as it is with these selftests, just run
>> single subtests at a time on target, but if I have new phy problems like
>> the one you have seen I have had before, then testing on target becomes
>> off limits.
>
> Please open a dedicated communication channel (separate email thread on
> netdev@vger.kernel.org) with the appropriate maintainers for the PHY
> code that is failing for you in To:, and you will get the help that you
> need to resolve that and to be able to test on the target board.

The errors from the phy I saw in February. Maybe something was fixed in
the meantime as I did not see the same warning and exception last I
tried to run the newest kernel on target a little over a week ago.
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index dc92d32464f6..dbc7017fd45d 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -14,6 +14,7 @@  ALL_TESTS="
 NUM_NETIFS=4
 CHECK_TC="no"
 source lib.sh
+source tc_common.sh
 
 h1_create()
 {
@@ -319,6 +320,41 @@  locked_port_mab_flush()
 	log_test "Locked port MAB FDB flush"
 }
 
+# Test of dynamic FDB entries.
+locked_port_dyn_fdb()
+{
+	local mac=00:01:02:03:04:05
+	local ageing_time
+
+	RET=0
+	ageing_time=$(bridge_ageing_time_get br0)
+	tc qdisc add dev $swp2 clsact
+	ip link set dev br0 type bridge ageing_time $LOW_AGEING_TIME
+	bridge link set dev $swp1 learning on locked on
+
+	bridge fdb replace $mac dev $swp1 master dynamic
+	tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
+		dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
+
+	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
+		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
+	tc_check_packets "dev $swp2 egress" 1 1
+	check_err $? "Packet not seen on egress after adding dynamic FDB"
+
+	sleep $((LOW_AGEING_TIME / 100 + 10))
+
+	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
+		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
+	tc_check_packets "dev $swp2 egress" 1 1
+	check_fail $? "Dynamic FDB entry did not age out"
+
+	ip link set dev br0 type bridge ageing_time $ageing_time
+	bridge link set dev $swp1 learning off locked off
+	tc qdisc del dev $swp2 clsact
+
+	log_test "Locked port dyn FDB"
+}
+
 trap cleanup EXIT
 
 setup_prepare