* [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting
@ 2024-01-24 9:58 Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions Hangbin Liu
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Hangbin Liu @ 2024-01-24 9:58 UTC (permalink / raw)
To: netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li, Hangbin Liu
There are a lot waitings in bonding tests use sleep. Let's replace them with
busywait or slowwait(added in the first patch). This could save much test
time. e.g.
bond-break-lacpdu-tx.sh
before: 0m16.346s
after: 0m2.424s
bond_options.sh
before: 9m25.299s
after: 5m27.439s
bond-lladdr-target.sh
before: 0m7.090s
after: 0m6.148s
bond_macvlan.sh
before: 0m44.999s
after: 0m23.468s
In total, we could save about 270 seconds.
Hangbin Liu (4):
selftests/net/forwarding: add slowwait functions
selftests: bonding: use tc filter to check if LACP was sent
selftests: bonding: reduce garp_test/arp_validate test time
selftests: bonding: use busy/slowwait instead of hard code sleep
.../net/bonding/bond-break-lacpdu-tx.sh | 18 +++++-----
.../drivers/net/bonding/bond-lladdr-target.sh | 21 +++++++++--
.../drivers/net/bonding/bond_macvlan.sh | 5 ++-
.../drivers/net/bonding/bond_options.sh | 22 +++++++++---
.../drivers/net/bonding/bond_topo_2d1c.sh | 6 ++--
tools/testing/selftests/net/forwarding/lib.sh | 36 +++++++++++++++++++
6 files changed, 85 insertions(+), 23 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions
2024-01-24 9:58 [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Hangbin Liu
@ 2024-01-24 9:58 ` Hangbin Liu
2024-01-24 13:25 ` Przemek Kitszel
2024-01-24 9:58 ` [PATCH net-next 2/4] selftests: bonding: use tc filter to check if LACP was sent Hangbin Liu
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Hangbin Liu @ 2024-01-24 9:58 UTC (permalink / raw)
To: netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li, Hangbin Liu
Add slowwait functions to wait for some operations that may need a long time
to finish. The busywait executes the cmd too fast, which is kind of wasting
cpu in this scenario. At the same time, if shell debugging is enabled with
`set -x`. the busywait will output too much logs.
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
tools/testing/selftests/net/forwarding/lib.sh | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 8a61464ab6eb..07faedc2071b 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -41,6 +41,7 @@ fi
# Kselftest framework requirement - SKIP code is 4.
ksft_skip=4
+# timeout in milliseconds
busywait()
{
local timeout=$1; shift
@@ -64,6 +65,32 @@ busywait()
done
}
+# timeout in seconds
+slowwait()
+{
+ local timeout=$1; shift
+
+ local start_time="$(date -u +%s)"
+ while true
+ do
+ local out
+ out=$("$@")
+ local ret=$?
+ if ((!ret)); then
+ echo -n "$out"
+ return 0
+ fi
+
+ local current_time="$(date -u +%s)"
+ if ((current_time - start_time > timeout)); then
+ echo -n "$out"
+ return 1
+ fi
+
+ sleep 1
+ done
+}
+
##############################################################################
# Sanity checks
@@ -505,6 +532,15 @@ busywait_for_counter()
busywait "$timeout" until_counter_is ">= $((base + delta))" "$@"
}
+slowwait_for_counter()
+{
+ local timeout=$1; shift
+ local delta=$1; shift
+
+ local base=$("$@")
+ slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
+}
+
setup_wait_dev()
{
local dev=$1; shift
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 2/4] selftests: bonding: use tc filter to check if LACP was sent
2024-01-24 9:58 [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions Hangbin Liu
@ 2024-01-24 9:58 ` Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time Hangbin Liu
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Hangbin Liu @ 2024-01-24 9:58 UTC (permalink / raw)
To: netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li, Hangbin Liu
Use tc filter to check if LACP was sent, which is accurate and save
more time.
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
.../net/bonding/bond-break-lacpdu-tx.sh | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh b/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh
index 6358df5752f9..5087291d15ce 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh
@@ -20,21 +20,22 @@
# +------+ +------+
#
# We use veths instead of physical interfaces
+REQUIRE_MZ=no
+NUM_NETIFS=0
+lib_dir=$(dirname "$0")
+source "$lib_dir"/net_forwarding_lib.sh
set -e
-tmp=$(mktemp -q dump.XXXXXX)
cleanup() {
ip link del fab-br0 >/dev/null 2>&1 || :
ip link del fbond >/dev/null 2>&1 || :
ip link del veth1-bond >/dev/null 2>&1 || :
ip link del veth2-bond >/dev/null 2>&1 || :
modprobe -r bonding >/dev/null 2>&1 || :
- rm -f -- ${tmp}
}
trap cleanup 0 1 2
cleanup
-sleep 1
# create the bridge
ip link add fab-br0 address 52:54:00:3B:7C:A6 mtu 1500 type bridge \
@@ -67,13 +68,12 @@ ip link set fab-br0 up
ip link set fbond up
ip addr add dev fab-br0 10.0.0.3
-tcpdump -n -i veth1-end -e ether proto 0x8809 >${tmp} 2>&1 &
-sleep 15
-pkill tcpdump >/dev/null 2>&1
rc=0
-num=$(grep "packets captured" ${tmp} | awk '{print $1}')
-if test "$num" -gt 0; then
- echo "PASS, captured ${num}"
+tc qdisc add dev veth1-end clsact
+tc filter add dev veth1-end ingress protocol 0x8809 pref 1 handle 101 flower skip_hw action pass
+if slowwait_for_counter 15 2 \
+ tc_rule_handle_stats_get "dev veth1-end ingress" 101 ".packets" "" &> /dev/null; then
+ echo "PASS, captured 2"
else
echo "FAIL"
rc=1
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time
2024-01-24 9:58 [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 2/4] selftests: bonding: use tc filter to check if LACP was sent Hangbin Liu
@ 2024-01-24 9:58 ` Hangbin Liu
2024-01-26 9:57 ` Paolo Abeni
2024-01-24 9:58 ` [PATCH net-next 4/4] selftests: bonding: use busy/slowwait instead of hard code sleep Hangbin Liu
2024-01-24 13:26 ` [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Przemek Kitszel
4 siblings, 1 reply; 11+ messages in thread
From: Hangbin Liu @ 2024-01-24 9:58 UTC (permalink / raw)
To: netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li, Hangbin Liu
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, the 50 num_grat_arp testing could be removed.
The arp_validate_test also need to check the mii_status, which sleep
too long. Use slowwait to save some time.
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
.../drivers/net/bonding/bond_options.sh | 22 ++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
index c54d1697f439..648006763b1b 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
@@ -199,6 +199,15 @@ prio()
done
}
+wait_mii_up()
+{
+ for i in $(seq 0 2); do
+ mii_status=$(cmd_jq "ip -n ${s_ns} -j -d link show eth$i" ".[].linkinfo.info_slave_data.mii_status")
+ [ ${mii_status} != "UP" ] && return 1
+ done
+ return 0
+}
+
arp_validate_test()
{
local param="$1"
@@ -211,7 +220,7 @@ arp_validate_test()
[ $RET -ne 0 ] && log_test "arp_validate" "$retmsg"
# wait for a while to make sure the mii status stable
- sleep 5
+ slowwait 5 wait_mii_up
for i in $(seq 0 2); do
mii_status=$(cmd_jq "ip -n ${s_ns} -j -d link show eth$i" ".[].linkinfo.info_slave_data.mii_status")
if [ ${mii_status} != "UP" ]; then
@@ -276,10 +285,13 @@ garp_test()
active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
ip -n ${s_ns} link set ${active_slave} down
- exp_num=$(echo "${param}" | cut -f6 -d ' ')
- sleep $((exp_num + 2))
+ # wait for active link change
+ sleep 1
+ exp_num=$(echo "${param}" | cut -f6 -d ' ')
active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
+ slowwait_for_counter $((exp_num + 5)) $exp_num \
+ tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}"
# check result
real_num=$(tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}")
@@ -296,8 +308,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
}
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 4/4] selftests: bonding: use busy/slowwait instead of hard code sleep
2024-01-24 9:58 [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Hangbin Liu
` (2 preceding siblings ...)
2024-01-24 9:58 ` [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time Hangbin Liu
@ 2024-01-24 9:58 ` Hangbin Liu
2024-01-24 13:26 ` [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Przemek Kitszel
4 siblings, 0 replies; 11+ messages in thread
From: Hangbin Liu @ 2024-01-24 9:58 UTC (permalink / raw)
To: netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li, Hangbin Liu
Use busywait or slowwait instead of hard code sleep. When using busywait
to check the link connection, I will set ping timeout to 0.1, which
could make busywait not too busy.
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
.../drivers/net/bonding/bond-lladdr-target.sh | 21 ++++++++++++++++---
.../drivers/net/bonding/bond_macvlan.sh | 5 ++---
.../drivers/net/bonding/bond_topo_2d1c.sh | 6 +++---
3 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/bonding/bond-lladdr-target.sh b/tools/testing/selftests/drivers/net/bonding/bond-lladdr-target.sh
index 89af402fabbe..b7b60e767daa 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond-lladdr-target.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond-lladdr-target.sh
@@ -17,6 +17,11 @@
# +----------------+
#
# We use veths instead of physical interfaces
+REQUIRE_MZ=no
+NUM_NETIFS=0
+lib_dir=$(dirname "$0")
+source "$lib_dir"/net_forwarding_lib.sh
+
sw="sw-$(mktemp -u XXXXXX)"
host="ns-$(mktemp -u XXXXXX)"
@@ -26,6 +31,16 @@ cleanup()
ip netns del $host
}
+wait_lladdr_dad()
+{
+ $@ | grep fe80 | grep -qv tentative
+}
+
+wait_bond_up()
+{
+ $@ | grep -q 'state UP'
+}
+
trap cleanup 0 1 2
ip netns add $sw
@@ -37,8 +52,8 @@ ip -n $host link add veth1 type veth peer name veth1 netns $sw
ip -n $sw link add br0 type bridge
ip -n $sw link set br0 up
sw_lladdr=$(ip -n $sw addr show br0 | awk '/fe80/{print $2}' | cut -d'/' -f1)
-# sleep some time to make sure bridge lladdr pass DAD
-sleep 2
+# wait some time to make sure bridge lladdr pass DAD
+slowwait 2 wait_lladdr_dad ip -n $sw addr show br0
ip -n $host link add bond0 type bond mode 1 ns_ip6_target ${sw_lladdr} \
arp_validate 3 arp_interval 1000
@@ -53,7 +68,7 @@ ip -n $sw link set veth1 master br0
ip -n $sw link set veth0 up
ip -n $sw link set veth1 up
-sleep 5
+slowwait 5 wait_bond_up ip -n $host link show bond0
rc=0
if ip -n $host link show bond0 | grep -q LOWER_UP; then
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_macvlan.sh b/tools/testing/selftests/drivers/net/bonding/bond_macvlan.sh
index b609fb6231f4..4fddb28a0715 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_macvlan.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_macvlan.sh
@@ -58,7 +58,7 @@ macvlan_over_bond()
ip -n ${m2_ns} addr add ${m2_ip4}/24 dev macv0
ip -n ${m2_ns} addr add ${m2_ip6}/24 dev macv0
- sleep 2
+ busywait 2000 ip netns exec ${c_ns} ping ${s_ip4} -c 1 -W 0.1 &> /dev/null
check_connection "${c_ns}" "${s_ip4}" "IPv4: client->server"
check_connection "${c_ns}" "${s_ip6}" "IPv6: client->server"
@@ -69,8 +69,7 @@ macvlan_over_bond()
check_connection "${m1_ns}" "${m2_ip4}" "IPv4: macvlan_1->macvlan_2"
check_connection "${m1_ns}" "${m2_ip6}" "IPv6: macvlan_1->macvlan_2"
-
- sleep 5
+ busywait 5000 ip netns exec ${s_ns} ping ${c_ip4} -c 1 -W 0.1 &> /dev/null
check_connection "${s_ns}" "${c_ip4}" "IPv4: server->client"
check_connection "${s_ns}" "${c_ip6}" "IPv6: server->client"
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_topo_2d1c.sh b/tools/testing/selftests/drivers/net/bonding/bond_topo_2d1c.sh
index a509ef949dcf..7c3f15bc6a9b 100644
--- a/tools/testing/selftests/drivers/net/bonding/bond_topo_2d1c.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_topo_2d1c.sh
@@ -73,7 +73,6 @@ server_create()
ip -n ${s_ns} link set bond0 up
ip -n ${s_ns} addr add ${s_ip4}/24 dev bond0
ip -n ${s_ns} addr add ${s_ip6}/24 dev bond0
- sleep 2
}
# Reset bond with new mode and options
@@ -96,7 +95,8 @@ bond_reset()
ip -n ${s_ns} link set bond0 up
ip -n ${s_ns} addr add ${s_ip4}/24 dev bond0
ip -n ${s_ns} addr add ${s_ip6}/24 dev bond0
- sleep 2
+ # Wait for IPv6 address ready as it needs DAD
+ busywait 5000 ip netns exec ${s_ns} ping ${c_ip6} -c 1 -W 0.1 &> /dev/null
}
server_destroy()
@@ -150,7 +150,7 @@ bond_check_connection()
{
local msg=${1:-"check connection"}
- sleep 2
+ busywait 2000 ip netns exec ${s_ns} ping ${c_ip4} -c 1 -W 0.1 &> /dev/null
ip netns exec ${s_ns} ping ${c_ip4} -c5 -i 0.1 &>/dev/null
check_err $? "${msg}: ping failed"
ip netns exec ${s_ns} ping6 ${c_ip6} -c5 -i 0.1 &>/dev/null
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions
2024-01-24 9:58 ` [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions Hangbin Liu
@ 2024-01-24 13:25 ` Przemek Kitszel
2024-01-26 9:22 ` Hangbin Liu
0 siblings, 1 reply; 11+ messages in thread
From: Przemek Kitszel @ 2024-01-24 13:25 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li
On 1/24/24 10:58, Hangbin Liu wrote:
> Add slowwait functions to wait for some operations that may need a long time
> to finish. The busywait executes the cmd too fast, which is kind of wasting
> cpu in this scenario. At the same time, if shell debugging is enabled with
> `set -x`. the busywait will output too much logs.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> tools/testing/selftests/net/forwarding/lib.sh | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 8a61464ab6eb..07faedc2071b 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -41,6 +41,7 @@ fi
> # Kselftest framework requirement - SKIP code is 4.
> ksft_skip=4
>
> +# timeout in milliseconds
> busywait()
> {
> local timeout=$1; shift
> @@ -64,6 +65,32 @@ busywait()
> done
> }
>
> +# timeout in seconds
> +slowwait()
> +{
> + local timeout=$1; shift
> +
> + local start_time="$(date -u +%s)"
> + while true
> + do
> + local out
> + out=$("$@")
> + local ret=$?
> + if ((!ret)); then
it would be nice to have some exit code used (or just reserved) for
"operation failed, no need to wait, fail the test please"
similar to the xargs, eg:
126 if the command cannot be run
> + echo -n "$out"
> + return 0
> + fi
> +
> + local current_time="$(date -u +%s)"
> + if ((current_time - start_time > timeout)); then
> + echo -n "$out"
> + return 1
> + fi
> +
> + sleep 1
I see that `sleep 1` is simplest correct impl, but it's tempting to
suggest exponential back-off, perhaps with saturation at 15
(but then you will have to cap last sleep to don't exceed timeout by
more than 1).
> + done
> +}
> +
> ##############################################################################
> # Sanity checks
>
> @@ -505,6 +532,15 @@ busywait_for_counter()
> busywait "$timeout" until_counter_is ">= $((base + delta))" "$@"
> }
>
> +slowwait_for_counter()
> +{
> + local timeout=$1; shift
> + local delta=$1; shift
> +
> + local base=$("$@")
> + slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
> +}
> +
> setup_wait_dev()
> {
> local dev=$1; shift
just nitpicks so I will provide my RB in case you want to ignore ;)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting
2024-01-24 9:58 [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Hangbin Liu
` (3 preceding siblings ...)
2024-01-24 9:58 ` [PATCH net-next 4/4] selftests: bonding: use busy/slowwait instead of hard code sleep Hangbin Liu
@ 2024-01-24 13:26 ` Przemek Kitszel
4 siblings, 0 replies; 11+ messages in thread
From: Przemek Kitszel @ 2024-01-24 13:26 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Liang Li
On 1/24/24 10:58, Hangbin Liu wrote:
> There are a lot waitings in bonding tests use sleep. Let's replace them with
> busywait or slowwait(added in the first patch). This could save much test
> time. e.g.
>
> bond-break-lacpdu-tx.sh
> before: 0m16.346s
> after: 0m2.424s
>
> bond_options.sh
> before: 9m25.299s
> after: 5m27.439s
>
> bond-lladdr-target.sh
> before: 0m7.090s
> after: 0m6.148s
>
> bond_macvlan.sh
> before: 0m44.999s
> after: 0m23.468s
>
> In total, we could save about 270 seconds.
>
> Hangbin Liu (4):
> selftests/net/forwarding: add slowwait functions
> selftests: bonding: use tc filter to check if LACP was sent
> selftests: bonding: reduce garp_test/arp_validate test time
> selftests: bonding: use busy/slowwait instead of hard code sleep
>
> .../net/bonding/bond-break-lacpdu-tx.sh | 18 +++++-----
> .../drivers/net/bonding/bond-lladdr-target.sh | 21 +++++++++--
> .../drivers/net/bonding/bond_macvlan.sh | 5 ++-
> .../drivers/net/bonding/bond_options.sh | 22 +++++++++---
> .../drivers/net/bonding/bond_topo_2d1c.sh | 6 ++--
> tools/testing/selftests/net/forwarding/lib.sh | 36 +++++++++++++++++++
> 6 files changed, 85 insertions(+), 23 deletions(-)
>
for the series:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions
2024-01-24 13:25 ` Przemek Kitszel
@ 2024-01-26 9:22 ` Hangbin Liu
2024-01-26 9:53 ` Paolo Abeni
0 siblings, 1 reply; 11+ messages in thread
From: Hangbin Liu @ 2024-01-26 9:22 UTC (permalink / raw)
To: Przemek Kitszel
Cc: netdev, Jay Vosburgh, David S . Miller, Jakub Kicinski,
Paolo Abeni, Eric Dumazet, Liang Li
Hi Przemek,
Thanks for your review.
On Wed, Jan 24, 2024 at 02:25:57PM +0100, Przemek Kitszel wrote:
> > +# timeout in seconds
> > +slowwait()
> > +{
> > + local timeout=$1; shift
> > +
> > + local start_time="$(date -u +%s)"
> > + while true
> > + do
> > + local out
> > + out=$("$@")
> > + local ret=$?
> > + if ((!ret)); then
>
> it would be nice to have some exit code used (or just reserved) for
> "operation failed, no need to wait, fail the test please"
> similar to the xargs, eg:
> 126 if the command cannot be run
Return directly instead of wait may confuse the caller. Maybe we can
add a parameter and let user decide whether to wait if return some value.
e.g.
slowwait nowait 126 $timeout $commands
>
> > + echo -n "$out"
> > + return 0
> > + fi
> > +
> > + local current_time="$(date -u +%s)"
> > + if ((current_time - start_time > timeout)); then
> > + echo -n "$out"
> > + return 1
> > + fi
> > +
> > + sleep 1
>
> I see that `sleep 1` is simplest correct impl, but it's tempting to
> suggest exponential back-off, perhaps with saturation at 15
>
> (but then you will have to cap last sleep to don't exceed timeout by
> more than 1).
Do you mean sleep longer when cmd exec failed? I'm not sure if it's a good
idea as the caller still wants to return ASAP when cmd exec succeeds.
Thanks
Hangbin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions
2024-01-26 9:22 ` Hangbin Liu
@ 2024-01-26 9:53 ` Paolo Abeni
0 siblings, 0 replies; 11+ messages in thread
From: Paolo Abeni @ 2024-01-26 9:53 UTC (permalink / raw)
To: Hangbin Liu, Przemek Kitszel
Cc: netdev, Jay Vosburgh, David S . Miller, Jakub Kicinski,
Eric Dumazet, Liang Li
On Fri, 2024-01-26 at 17:22 +0800, Hangbin Liu wrote:
> On Wed, Jan 24, 2024 at 02:25:57PM +0100, Przemek Kitszel wrote:
> >
> > > + echo -n "$out"
> > > + return 0
> > > + fi
> > > +
> > > + local current_time="$(date -u +%s)"
> > > + if ((current_time - start_time > timeout)); then
> > > + echo -n "$out"
> > > + return 1
> > > + fi
> > > +
> > > + sleep 1
> >
> > I see that `sleep 1` is simplest correct impl, but it's tempting to
> > suggest exponential back-off, perhaps with saturation at 15
> >
> > (but then you will have to cap last sleep to don't exceed timeout by
> > more than 1).
>
> Do you mean sleep longer when cmd exec failed? I'm not sure if it's a good
> idea as the caller still wants to return ASAP when cmd exec succeeds.
I think exponential backoff is not needed here: we don't care about
minimizing the CPU usage overhead, and there should not be 'collision'
issues.
Instead I *think* you could use a smaller sleep, e.g.
sleep 0.1
and hopefully reduce the latency even further.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time
2024-01-24 9:58 ` [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time Hangbin Liu
@ 2024-01-26 9:57 ` Paolo Abeni
2024-01-26 12:52 ` Hangbin Liu
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2024-01-26 9:57 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Jay Vosburgh, David S . Miller, Jakub Kicinski, Eric Dumazet, Liang Li
On Wed, 2024-01-24 at 17:58 +0800, Hangbin Liu wrote:
> @@ -276,10 +285,13 @@ garp_test()
> active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
> ip -n ${s_ns} link set ${active_slave} down
>
> - exp_num=$(echo "${param}" | cut -f6 -d ' ')
> - sleep $((exp_num + 2))
> + # wait for active link change
> + sleep 1
If 'slowwait' would loop around a sub-second sleep, I guess you could
use 'slowwait' here, too.
>
> + exp_num=$(echo "${param}" | cut -f6 -d ' ')
> active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
> + slowwait_for_counter $((exp_num + 5)) $exp_num \
> + tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}"
>
> # check result
> real_num=$(tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}")
> @@ -296,8 +308,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"
Can we reduce 'peer_notify_delay' even further, say to '250' and
preserve the test effectiveness?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time
2024-01-26 9:57 ` Paolo Abeni
@ 2024-01-26 12:52 ` Hangbin Liu
0 siblings, 0 replies; 11+ messages in thread
From: Hangbin Liu @ 2024-01-26 12:52 UTC (permalink / raw)
To: Paolo Abeni, Jay Vosburgh
Cc: netdev, David S . Miller, Jakub Kicinski, Eric Dumazet, Liang Li
On Fri, Jan 26, 2024 at 10:57:31AM +0100, Paolo Abeni wrote:
> On Wed, 2024-01-24 at 17:58 +0800, Hangbin Liu wrote:
> > @@ -276,10 +285,13 @@ garp_test()
> > active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
> > ip -n ${s_ns} link set ${active_slave} down
> >
> > - exp_num=$(echo "${param}" | cut -f6 -d ' ')
> > - sleep $((exp_num + 2))
> > + # wait for active link change
> > + sleep 1
>
> If 'slowwait' would loop around a sub-second sleep, I guess you could
> use 'slowwait' here, too.
OK, let me change the slowwait to sleep 0.1s.
>
> >
> > + exp_num=$(echo "${param}" | cut -f6 -d ' ')
> > active_slave=$(cmd_jq "ip -n ${s_ns} -d -j link show bond0" ".[].linkinfo.info_data.active_slave")
> > + slowwait_for_counter $((exp_num + 5)) $exp_num \
> > + tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}"
> >
> > # check result
> > real_num=$(tc_rule_handle_stats_get "dev s${active_slave#eth} ingress" 101 ".packets" "-n ${g_ns}")
> > @@ -296,8 +308,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"
>
> Can we reduce 'peer_notify_delay' even further, say to '250' and
> preserve the test effectiveness?
Hmm, maybe we can set miimon to 10. Then we can reduce peer_notify_delay to 100.
Jay, do you think if there is any side efect to set miimon to 10?
Thanks
Hangbin
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-01-26 12:53 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-24 9:58 [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions Hangbin Liu
2024-01-24 13:25 ` Przemek Kitszel
2024-01-26 9:22 ` Hangbin Liu
2024-01-26 9:53 ` Paolo Abeni
2024-01-24 9:58 ` [PATCH net-next 2/4] selftests: bonding: use tc filter to check if LACP was sent Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 3/4] selftests: bonding: reduce garp_test/arp_validate test time Hangbin Liu
2024-01-26 9:57 ` Paolo Abeni
2024-01-26 12:52 ` Hangbin Liu
2024-01-24 9:58 ` [PATCH net-next 4/4] selftests: bonding: use busy/slowwait instead of hard code sleep Hangbin Liu
2024-01-24 13:26 ` [PATCH net-next 0/4] selftests: bonding: use busy/slowwait when waiting Przemek Kitszel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.