Skip to contentSkip to conversation

selftests: bonding: Add more missing config options

PATCH Applied Benjamin Poirier 5 participants 11 messages Jan 16, 2024 – Jan 18, 2024

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 selftests: bonding: Add more missing config options

As a followup to commit 03fb8565c880 ("selftests: bonding: add missing build configs"), add more networking-specific config options which are needed for bonding tests.

For testing, I used the minimal config generated by virtme-ng and I added the options in the config file. All bonding tests passed.

Fixes bbb774d921e2 ("net: Add tests for bonding and team address list management") # for ipv6
Fixes 6cbe791c0f4e ("kselftest: bonding: add num_grat_arp test") # for tc options
Fixes 222c94ec0ad4 ("selftests: bonding: add tests for ether type changes") # for nlmon
---
 tools/testing/selftests/drivers/net/bonding/config | 5 +++++
 1 file changed, 5 insertions(+)
tools/testing/selftests/drivers/net/bonding/config +5 -0
index f85b16fc5128..899d7fb6ea8e 100644
@@ -1,5 +1,10 @@
 CONFIG_BONDING=y
 CONFIG_BRIDGE=y
 CONFIG_DUMMY=y
+CONFIG_IPV6=y
 CONFIG_MACVLAN=y
+CONFIG_NET_ACT_GACT=y
+CONFIG_NET_CLS_FLOWER=y
+CONFIG_NET_SCH_INGRESS=y
+CONFIG_NLMON=y
 CONFIG_VETH=y
Signature · 1 line
2.43.0
maintainer
#1 lore raw
Quoted 4 lines · On Tue, 16 Jan 2024 10:49:26 -0500 Benjamin Poirier wrote:

As a followup to commit 03fb8565c880 ("selftests: bonding: add missing build configs"), add more networking-specific config options which are needed for bonding tests.

Quoted 8 lines

For testing, I used the minimal config generated by virtme-ng and I added the options in the config file. All bonding tests passed.

Fixes bbb774d921e2 ("net: Add tests for bonding and team address list management") # for ipv6
Fixes 6cbe791c0f4e ("kselftest: bonding: add num_grat_arp test") # for tc options
Fixes 222c94ec0ad4 ("selftests: bonding: add tests for ether type changes") # for nlmon

With this applied the only remaining bonding test which fails in our CI is bond-options [1], good progress! :) Looks like it doesn't finish in time:

not ok 7 selftests: drivers/net/bonding: bond_options.sh # TIMEOUT 120 seconds

The tests run in a VM without HW virtualization support. Any opinions about bumping the timeout for bonding? If we enable KASAN etc. things will get even slower.

[1]
https://netdev-2.bots.linux.dev/vmksft-bonding/results/423800/7-bond-options-sh

#2 lore raw
Quoted 18 lines · Jakub Kicinski
Quoted 4 lines · On Tue, 16 Jan 2024 10:49:26 -0500 Benjamin Poirier wrote:

As a followup to commit 03fb8565c880 ("selftests: bonding: add missing build configs"), add more networking-specific config options which are needed for bonding tests.

Quoted 8 lines

For testing, I used the minimal config generated by virtme-ng and I added the options in the config file. All bonding tests passed.

Fixes bbb774d921e2 ("net: Add tests for bonding and team address list management") # for ipv6
Fixes 6cbe791c0f4e ("kselftest: bonding: add num_grat_arp test") # for tc options
Fixes 222c94ec0ad4 ("selftests: bonding: add tests for ether type changes") # for nlmon

With this applied the only remaining bonding test which fails in our CI is bond-options [1], good progress! :) Looks like it doesn't finish in time:

Quoted 5 lines

not ok 7 selftests: drivers/net/bonding: bond_options.sh # TIMEOUT 120 seconds

The tests run in a VM without HW virtualization support. Any opinions about bumping the timeout for bonding? If we enable KASAN etc. things will get even slower.

Reading the grat_arp test, it looks like it has long sleep times
built into it:

garp_test()
{
[...]

	exp_num=$(echo "${param}" | cut -f6 -d ' ')
	sleep $((exp_num + 2))

num_grat_arp()
{
local val
for val in 10 20 30 50; do
garp_test "mode active-backup miimon 100 num_grat_arp $val peer_notify_delay 1000"

If I'm reading it right, this will sleep for 12, 22, 32 and 52
seconds for the passes through the loop in num_grat_arp(), so that would be 118 seconds just for that.

	-J
Quoted 2 lines

[1]
https://netdev-2.bots.linux.dev/vmksft-bonding/results/423800/7-bond-options-sh

---
-Jay Vosburgh, [email protected]

maintainer
#3 lore raw
Quoted 3 lines · On Tue, 16 Jan 2024 11:03:30 -0800 Jay Vosburgh wrote:

If I'm reading it right, this will sleep for 12, 22, 32 and 52
seconds for the passes through the loop in num_grat_arp(), so that would be 118 seconds just for that.

Hah, that's really tight if the default timeout is 120. Let me send a patch to bump it to 300..

#4 lore raw
Quoted 7 lines · Jakub Kicinski
Quoted 3 lines · On Tue, 16 Jan 2024 11:03:30 -0800 Jay Vosburgh wrote:

If I'm reading it right, this will sleep for 12, 22, 32 and 52
seconds for the passes through the loop in num_grat_arp(), so that would be 118 seconds just for that.

Hah, that's really tight if the default timeout is 120. Let me send a patch to bump it to 300..

I also ran into that timeout when using make run_tests so I executed the test script directly:

# time ./bond_options.sh
TEST: prio (active-backup miimon primary_reselect 0)                [ OK ]
TEST: prio (active-backup miimon primary_reselect 1)                [ OK ]
TEST: prio (active-backup miimon primary_reselect 2)                [ OK ]
TEST: prio (active-backup arp_ip_target primary_reselect 0)         [ OK ]
TEST: prio (active-backup arp_ip_target primary_reselect 1)         [ OK ]
TEST: prio (active-backup arp_ip_target primary_reselect 2)         [ OK ]
TEST: prio (active-backup ns_ip6_target primary_reselect 0)         [ OK ]
TEST: prio (active-backup ns_ip6_target primary_reselect 1)         [ OK ]
TEST: prio (active-backup ns_ip6_target primary_reselect 2)         [ OK ]
TEST: prio (balance-tlb miimon primary_reselect 0)                  [ OK ]
TEST: prio (balance-tlb miimon primary_reselect 1)                  [ OK ]
TEST: prio (balance-tlb miimon primary_reselect 2)                  [ OK ]
TEST: prio (balance-tlb arp_ip_target primary_reselect 0)           [ OK ]
TEST: prio (balance-tlb arp_ip_target primary_reselect 1)           [ OK ]
TEST: prio (balance-tlb arp_ip_target primary_reselect 2)           [ OK ]
TEST: prio (balance-tlb ns_ip6_target primary_reselect 0)           [ OK ]
TEST: prio (balance-tlb ns_ip6_target primary_reselect 1)           [ OK ]
TEST: prio (balance-tlb ns_ip6_target primary_reselect 2)           [ OK ]
TEST: prio (balance-alb miimon primary_reselect 0)                  [ OK ]
TEST: prio (balance-alb miimon primary_reselect 1)                  [ OK ]
TEST: prio (balance-alb miimon primary_reselect 2)                  [ OK ]
TEST: prio (balance-alb arp_ip_target primary_reselect 0)           [ OK ]
TEST: prio (balance-alb arp_ip_target primary_reselect 1)           [ OK ]
TEST: prio (balance-alb arp_ip_target primary_reselect 2)           [ OK ]
TEST: prio (balance-alb ns_ip6_target primary_reselect 0)           [ OK ]
TEST: prio (balance-alb ns_ip6_target primary_reselect 1)           [ OK ]
TEST: prio (balance-alb ns_ip6_target primary_reselect 2)           [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 0)     [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 1)     [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 2)     [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 3)     [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 4)     [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 5)     [ OK ]
TEST: arp_validate (active-backup arp_ip_target arp_validate 6)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 0)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 1)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 2)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 3)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 4)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 5)     [ OK ]
TEST: arp_validate (active-backup ns_ip6_target arp_validate 6)     [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 10)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 20)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 30)           [ OK ]
TEST: num_grat_arp (active-backup miimon num_grat_arp 50)           [ OK ]
real    13m35.065s
user    0m1.657s
sys     0m27.918s

The test is not cpu bound; as Jay pointed out, it spends most of its time sleeping.

maintainer
#5 lore raw
Quoted 6 lines · On Tue, 16 Jan 2024 14:21:51 -0500 Benjamin Poirier wrote:
real    13m35.065s
user    0m1.657s
sys     0m27.918s

The test is not cpu bound; as Jay pointed out, it spends most of its time sleeping.

Ugh, so it does multiple iterations of 118 sec?

Could you send a patch to bump the timeout to 900 or 1200 in this case?

#6 lore raw
Quoted 11 lines · Jakub Kicinski
Quoted 6 lines · On Tue, 16 Jan 2024 14:21:51 -0500 Benjamin Poirier wrote:
real    13m35.065s
user    0m1.657s
sys     0m27.918s

The test is not cpu bound; as Jay pointed out, it spends most of its time sleeping.

Ugh, so it does multiple iterations of 118 sec?

Could you send a patch to bump the timeout to 900 or 1200 in this case?

	We could also lower the interval or number of notifications;

right now "peer_notif_delay 1000" puts 1 second between the ARPs in the num_grat_arp() test. I'm not sure why that value was chosen, but the peer_notify_delay is rounded to units of the miimon interval, and in this test miimon is 100 msec.

I haven't tested this at all, but something like

tools/testing/selftests/drivers/net/bonding/bond_options.sh +2 -2
index c54d1697f439..95eb77aebc3c 100755
@@ -277,7 +277,7 @@ garp_test()
 	ip -n ${s_ns} link set ${active_slave} down
 
 	exp_num=$(echo "${param}" | cut -f6 -d ' ')
-	sleep $((exp_num + 2))
+	sleep $((exp_num / 5 + 2))
 
 	active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
 
@@ -297,7 +297,7 @@ num_grat_arp()
 {
 	local val
 	for val in 10 20 30 50; do
-		garp_test "mode active-backup miimon 100 num_grat_arp $val peer_notify_delay 1000"
+		garp_test "mode active-backup miimon 100 num_grat_arp $val peer_notify_delay 200"
 		log_test "num_grat_arp" "active-backup miimon num_grat_arp $val"
 	done
 }

could substantially reduce the time to run the test. It's kind
of icky with magic numbers, but that could be cleaned up.

	-J

---
-Jay Vosburgh, [email protected]

#7 lore raw
In reply to Jakub Kicinski
Quoted 9 lines · Jakub Kicinski
Quoted 6 lines · On Tue, 16 Jan 2024 14:21:51 -0500 Benjamin Poirier wrote:
real    13m35.065s
user    0m1.657s
sys     0m27.918s

The test is not cpu bound; as Jay pointed out, it spends most of its time sleeping.

Ugh, so it does multiple iterations of 118 sec?

There are other test functions in the script which include a lot of sleeping.

Quoted 1 line

Could you send a patch to bump the timeout to 900 or 1200 in this case?

Sure but I'd like to give a chance for Hangbin to reply first. Would the test be just as good if it was shortened by removing some cases or reducing the time intervals? Or is increasing the timeout the best approach?

#8 lore raw
Quoted 6 lines · Benjamin Poirier
Quoted 5 lines · Jakub Kicinski
Quoted 4 lines · On Tue, 16 Jan 2024 14:21:51 -0500 Benjamin Poirier wrote:
real    13m35.065s
user    0m1.657s
sys     0m27.918s
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

The test is not cpu bound; as Jay pointed out, it spends most of its time sleeping.

Ugh, so it does multiple iterations of 118 sec?

There are other test functions in the script which include a lot of sleeping.

The arp_validate_test need to check the mii_status, which sleep too much time. Maybe we can use busywait to save more time.

Quoted 7 lines
Quoted 2 lines

Could you send a patch to bump the timeout to 900 or 1200 in this case?

Sure but I'd like to give a chance for Hangbin to reply first. Would the test be just as good if it was shortened by removing some cases or reducing the time intervals? Or is increasing the timeout the best approach?

The purpose of grat_arp is testing commit 9949e2efb54e ("bonding: fix send_peer_notif overflow"). As the send_peer_notif was defined to u8, to overflow it, we need to

send_peer_notif = num_peer_notif * peer_notif_delay = num_grat_arp * peer_notify_delay / miimon > 255
(kernel) (kernel parameter) (user parameter)

e.g. 30 (num_grat_arp) * 1000 (peer_notify_delay) / 100 (miimon) > 255.

Which need 30s to complete sending garp messages. To save the testing time, the only way is reduce the miimon number. Something like
30 (num_grat_arp) * 500 (peer_notify_delay) / 50 (miimon) > 255.

To save more time, we can remove the 50 num_grat_arp testing. The patch would like

tools/testing/selftests/drivers/net/bonding/bond_options.sh +3 -3
index c54d1697f439..20c4d862c436 100755
@@ -277,7 +277,7 @@ garp_test()
        ip -n ${s_ns} link set ${active_slave} down
 
        exp_num=$(echo "${param}" | cut -f6 -d ' ')
-       sleep $((exp_num + 2))
+       sleep $((exp_num / 2 + 2))
 
        active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
 
@@ -296,8 +296,8 @@ garp_test()
 num_grat_arp()
 {
        local val
-       for val in 10 20 30 50; do
-               garp_test "mode active-backup miimon 100 num_grat_arp $val peer_notify_delay 1000"
+       for val in 10 20 30; do
+               garp_test "mode active-backup miimon 50 num_grat_arp $val peer_notify_delay 500"
                log_test "num_grat_arp" "active-backup miimon num_grat_arp $val"
        done
 }

With this we can save 100s.

Thanks
Hangbin

#9 lore raw
Quoted 67 lines · Hangbin Liu
Quoted 6 lines · Benjamin Poirier
Quoted 5 lines · Jakub Kicinski
Quoted 4 lines · On Tue, 16 Jan 2024 14:21:51 -0500 Benjamin Poirier wrote:
real    13m35.065s
user    0m1.657s
sys     0m27.918s
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

The test is not cpu bound; as Jay pointed out, it spends most of its time sleeping.

Ugh, so it does multiple iterations of 118 sec?

There are other test functions in the script which include a lot of sleeping.

The arp_validate_test need to check the mii_status, which sleep too much time. Maybe we can use busywait to save more time.

Quoted 7 lines
Quoted 2 lines

Could you send a patch to bump the timeout to 900 or 1200 in this case?

Sure but I'd like to give a chance for Hangbin to reply first. Would the test be just as good if it was shortened by removing some cases or reducing the time intervals? Or is increasing the timeout the best approach?

The purpose of grat_arp is testing commit 9949e2efb54e ("bonding: fix send_peer_notif overflow"). As the send_peer_notif was defined to u8, to overflow it, we need to

send_peer_notif = num_peer_notif * peer_notif_delay = num_grat_arp * peer_notify_delay / miimon > 255
(kernel) (kernel parameter) (user parameter)

e.g. 30 (num_grat_arp) * 1000 (peer_notify_delay) / 100 (miimon) > 255.

Which need 30s to complete sending garp messages. To save the testing time, the only way is reduce the miimon number. Something like
30 (num_grat_arp) * 500 (peer_notify_delay) / 50 (miimon) > 255.

To save more time, we can remove the 50 num_grat_arp testing. The patch would like

tools/testing/selftests/drivers/net/bonding/bond_options.sh +3 -3
index c54d1697f439..20c4d862c436 100755
@@ -277,7 +277,7 @@ garp_test()
        ip -n ${s_ns} link set ${active_slave} down
 
        exp_num=$(echo "${param}" | cut -f6 -d ' ')
-       sleep $((exp_num + 2))
+       sleep $((exp_num / 2 + 2))
 
        active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
 
@@ -296,8 +296,8 @@ garp_test()
 num_grat_arp()
 {
        local val
-       for val in 10 20 30 50; do
-               garp_test "mode active-backup miimon 100 num_grat_arp $val peer_notify_delay 1000"
+       for val in 10 20 30; do
+               garp_test "mode active-backup miimon 50 num_grat_arp $val peer_notify_delay 500"
                log_test "num_grat_arp" "active-backup miimon num_grat_arp $val"
        done
 }
Quoted 2 lines

With this we can save 100s.

Thanks for looking into it. This change got the runtime down from 13m35s to 12m7s on the same system I used to test yesterday. That's a start but since it's still well above the current timeout of 120s, I sent a patch to increase the timeout to 1200s.

#10 lore raw
In reply to Benjamin Poirier
This patch was applied to netdev/net.git (main) by Paolo Abeni <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

Quoted 8 lines · On Tue, 16 Jan 2024 10:49:26 -0500 you wrote:

As a followup to commit 03fb8565c880 ("selftests: bonding: add missing build configs"), add more networking-specific config options which are needed for bonding tests.

For testing, I used the minimal config generated by virtme-ng and I added the options in the config file. All bonding tests passed.

[...]

Here is the summary with links:
- [net] selftests: bonding: Add more missing config options

    https://git.kernel.org/netdev/net/c/dd2d40acdbb2

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html