diff mbox series

[mptcp-next] selftests/mptcp: Better delay & reordering configuration

Message ID 20200727234759.25705-1-cpaasch@apple.com
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series [mptcp-next] selftests/mptcp: Better delay & reordering configuration | expand

Commit Message

Christoph Paasch July 27, 2020, 11:47 p.m. UTC
The delay was intended to be configured to "simulate" a high(er) BDP
link. As such, it needs to be set as part of the loss-configuration and
not as part of the netem reordering configuration.

The reordering-config also requires a delay but that delay is the
reordering-extend. So, a good approach is to set the reordering-extend
as a function of the configured latency. E.g., 25% of the overall
latency.

Finally, the intention of tc_reorder was that when it is unset, the test
picks a random configuration. However, currently it is always initialized
and thus the random config won't be picked up.

Github-issue: https://github.com/multipath-tcp/mptcp_net-next/issues/6

Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---

Notes:
    Admittedly, two changes here in this patch (delay-fix and unitializing
    tc_reorder). If you want, I can split them but I thought that's overkill for
    a selftest-patch.

 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Matthieu Baerts Aug. 4, 2020, 4:16 p.m. UTC | #1
Hi Christoph,

Thank you for sharing this patch!

On 28/07/2020 01:47, Christoph Paasch wrote:
> The delay was intended to be configured to "simulate" a high(er) BDP
> link. As such, it needs to be set as part of the loss-configuration and
> not as part of the netem reordering configuration.
> 
> The reordering-config also requires a delay but that delay is the
> reordering-extend. So, a good approach is to set the reordering-extend
> as a function of the configured latency. E.g., 25% of the overall
> latency.
> 
> Finally, the intention of tc_reorder was that when it is unset, the test
> picks a random configuration. However, currently it is always initialized
> and thus the random config won't be picked up.
> 
> Github-issue: https://github.com/multipath-tcp/mptcp_net-next/issues/6

You can use the "Closes:" tag if you want.
Or another one from:

https://docs.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords

> Signed-off-by: Christoph Paasch <cpaasch@apple.com>
> ---
> 
> Notes:
>      Admittedly, two changes here in this patch (delay-fix and unitializing
>      tc_reorder). If you want, I can split them but I thought that's overkill for
>      a selftest-patch.

I am fine with that.

>   tools/testing/selftests/net/mptcp/mptcp_connect.sh | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 6260520674d0..d29d189d1ae5 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -16,7 +16,6 @@ ipv6=true
>   ethtool_random_on=true
>   tc_delay="$((RANDOM%400))"
>   tc_loss=$((RANDOM%101))
> -tc_reorder=""
>   testmode=""
>   sndbuf=0
>   rcvbuf=0
> @@ -631,22 +630,24 @@ for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
>   	do_ping "$ns4" $sender dead:beef:3::1
>   done
>   
> -[ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss
> +[ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms

It looks like it is too slow when we use this tc_delay for each packets, 
it is easy to timeout:

============
00:28:49.725 # selftests: net/mptcp: mptcp_connect.sh
00:28:50.693 [   63.023982] ip (516) used greatest stack depth: 22984 
bytes left
00:28:51.399 [   63.730210] IPv6: ADDRCONF(NETDEV_CHANGE): ns1eth2: link 
becomes ready
00:28:51.872 [   64.202628] IPv6: ADDRCONF(NETDEV_CHANGE): ns2eth1: link 
becomes ready
00:28:51.930 [   64.260700] IPv6: ADDRCONF(NETDEV_CHANGE): ns3eth2: link 
becomes ready
00:28:51.931 [   64.261982] IPv6: ADDRCONF(NETDEV_CHANGE): ns2eth3: link 
becomes ready
00:28:52.414 [   64.744960] IPv6: ADDRCONF(NETDEV_CHANGE): ns4eth3: link 
becomes ready
00:28:52.415 [   64.746169] IPv6: ADDRCONF(NETDEV_CHANGE): ns3eth4: link 
becomes ready
00:28:52.674 # INFO: set ns3-5f285c53-13cspj dev ns3eth2: ethtool -K 
gso off gro off
00:28:52.760 # INFO: set ns4-5f285c53-13cspj dev ns4eth3: ethtool -K tso off
00:28:53.183 # Created /tmp/tmp.giudffQ2E4 (size 7922716 
/tmp/tmp.giudffQ2E4) containing data sent by client
00:28:53.528 # Created /tmp/tmp.Tie6RowCZz (size 7493660 
/tmp/tmp.Tie6RowCZz) containing data sent by server
00:28:53.956 # New MPTCP socket can be blocked via sysctl		[ OK ]
00:28:54.164 # setsockopt(..., TCP_ULP, "mptcp", ...) blocked	[ OK ]
00:28:54.215 # INFO: validating network environment with pings
00:28:58.551 # INFO: Using loss of 0.57% delay 197 ms reorder 93% 64% on 
ns3eth4
00:29:00.071 # ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP	(duration 
323ms) [ OK ]
00:29:01.498 # ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP  	(duration 
295ms) [ OK ]
00:29:02.850 # ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP	(duration 
256ms) [ OK ]
00:29:04.294 # ns1 MPTCP -> ns1 (dead:beef:1::1:10003) MPTCP	(duration 
351ms) [ OK ]
00:29:05.697 # ns1 MPTCP -> ns1 (dead:beef:1::1:10004) TCP  	(duration 
257ms) [ OK ]
00:29:07.053 # ns1 TCP   -> ns1 (dead:beef:1::1:10005) MPTCP	(duration 
244ms) [ OK ]
00:29:08.924 # ns1 MPTCP -> ns2 (10.0.1.2:10006      ) MPTCP	(duration 
736ms) [ OK ]
00:29:10.537 # ns1 MPTCP -> ns2 (dead:beef:1::2:10007) MPTCP	(duration 
530ms) [ OK ]
00:29:12.570 # ns1 MPTCP -> ns2 (10.0.2.1:10008      ) MPTCP	(duration 
915ms) [ OK ]
00:29:14.483 # ns1 MPTCP -> ns2 (dead:beef:2::1:10009) MPTCP	(duration 
807ms) [ OK ]
00:29:39.992 # ns1 MPTCP -> ns3 (10.0.2.2:10010      ) MPTCP	(duration 
24444ms) [ OK ]
00:30:08.517 # ns1 MPTCP -> ns3 (dead:beef:2::2:10011) MPTCP	(duration 
27434ms) [ OK ]
00:30:39.626 # ns1 MPTCP -> ns3 (10.0.3.2:10012      ) MPTCP	(duration 
30024ms) [ OK ]
00:30:59.692 # ns1 MPTCP -> ns3 (dead:beef:3::2:10013) MPTCP	(duration 
18959ms) [ OK ]
00:31:35.088 # ns1 MPTCP -> ns4 (10.0.3.1:10014      ) MPTCP	(duration 
34303ms) [ OK ]
00:32:01.805 # ns1 MPTCP -> ns4 (dead:beef:3::1:10015) MPTCP	(duration 
25614ms) [ OK ]
00:32:03.701 # ns2 MPTCP -> ns1 (10.0.1.1:10016      ) MPTCP	(duration 
792ms) [ OK ]
00:32:05.375 # ns2 MPTCP -> ns1 (dead:beef:1::1:10017) MPTCP	(duration 
568ms) [ OK ]
00:32:24.546 # ns2 MPTCP -> ns3 (10.0.2.2:10018      ) MPTCP	(duration 
17980ms) [ OK ]
00:32:56.164 # ns2 MPTCP -> ns3 (dead:beef:2::2:10019) MPTCP	(duration 
30497ms) [ OK ]
00:33:00.952 # ns2 MPTCP -> ns3 (10.0.3.2:10020      ) MPTCP	(duration 
3684ms) [ OK ]
00:33:28.441 # ns2 MPTCP -> ns3 (dead:beef:3::2:10021) MPTCP	(duration 
26375ms) [ OK ]
00:34:00.064 # ns2 MPTCP -> ns4 (10.0.3.1:10022      ) MPTCP	(duration 
30538ms) [ OK ]
00:34:05.500 # ns2 MPTCP -> ns4 (dead:beef:3::1:10023) MPTCP	(duration 
4352ms) [ OK ]
00:34:27.354 # ns3 MPTCP -> ns1 (10.0.1.1:10024      ) MPTCP	(duration 
20759ms) [ OK ]
00:34:42.555 # ns3 MPTCP -> ns1 (dead:beef:1::1:10025) MPTCP	(duration 
14098ms) [ OK ]
00:34:54.836 # ns3 MPTCP -> ns2 (10.0.1.2:10026      ) MPTCP	(duration 
11197ms) [ OK ]
00:35:30.150 # ns3 MPTCP -> ns2 (dead:beef:1::2:10027) MPTCP	(duration 
34225ms) [ OK ]
00:36:07.746 # ns3 MPTCP -> ns2 (10.0.2.1:10028      ) MPTCP	(duration 
36505ms) [ OK ]
00:36:26.349 # ns3 MPTCP -> ns2 (dead:beef:2::1:10029) MPTCP	#
00:36:26.350 not ok 1 selftests: net/mptcp: mptcp_connect.sh # TIMEOUT
============

Should we have a max of 50ms or less?

(+ ${reorder_delay} of max 50ms as well?)


>   echo -n "INFO: Using loss of $tc_loss "
>   test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
>   
> +reorder_delay=`expr $tc_delay / 4`

It's a bit old school to use "`" :-D
No but it is usually recommended to use "$(...)" because it is more 
visible and can be nested.

Also only "$(...)" are used in this file.

And maybe even better to use this (if we still need to compute a value, 
see my comment above):

   reorder_delay=$((tc_delay / 4))

like we do just below with "reorder":

> +
>   if [ -z "${tc_reorder}" ]; then
>   	reorder1=$((RANDOM%10))
>   	reorder1=$((100 - reorder1))
>   	reorder2=$((RANDOM%100))
>   
> -	if [ $tc_delay -gt 0 ] && [ $reorder1 -lt 100 ] && [ $reorder2 -gt 0 ]; then
> +	if [ $reorder_delay -gt 0 ] && [ $reorder1 -lt 100 ] && [ $reorder2 -gt 0 ]; then
>   		tc_reorder="reorder ${reorder1}% ${reorder2}%"
>   		echo -n "$tc_reorder "
>   	fi
>   elif [ "$tc_reorder" = "0" ];then
>   	tc_reorder=""
> -elif [ "$tc_delay" -gt 0 ];then
> +elif [ "$reorder_delay" -gt 0 ];then
>   	# reordering requires some delay
>   	tc_reorder="reorder $tc_reorder"
>   	echo -n "$tc_reorder "

Can you print ${reorder_delay} as well please?

> @@ -654,7 +655,7 @@ fi
>   
>   echo "on ns3eth4"
>   
> -tc -net "$ns3" qdisc add dev ns3eth4 root netem delay ${tc_delay}ms $tc_reorder
> +tc -net "$ns3" qdisc add dev ns3eth4 root netem delay ${reorder_delay}ms $tc_reorder
>   
>   for sender in $ns1 $ns2 $ns3 $ns4;do
>   	run_tests_lo "$ns1" "$sender" 10.0.1.1 1
>

Note that I applied your patch in the export branch to do more tests 
with the CI but often, I was hitting the kselftest timeout. So I just 
reverted it.

I think it would be really good if these tests are quick to execute. 
Except if we think it is really important to do some tests with more 
than 50ms of delay? Maybe the proposed 50ms are already too high, 30ms 
max + reorder_delay could be OK no? If we need some tests with higher 
delay, maybe better to execute only a subset?

Cheers,
Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 6260520674d0..d29d189d1ae5 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -16,7 +16,6 @@  ipv6=true
 ethtool_random_on=true
 tc_delay="$((RANDOM%400))"
 tc_loss=$((RANDOM%101))
-tc_reorder=""
 testmode=""
 sndbuf=0
 rcvbuf=0
@@ -631,22 +630,24 @@  for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
 	do_ping "$ns4" $sender dead:beef:3::1
 done
 
-[ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss
+[ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
 echo -n "INFO: Using loss of $tc_loss "
 test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
 
+reorder_delay=`expr $tc_delay / 4`
+
 if [ -z "${tc_reorder}" ]; then
 	reorder1=$((RANDOM%10))
 	reorder1=$((100 - reorder1))
 	reorder2=$((RANDOM%100))
 
-	if [ $tc_delay -gt 0 ] && [ $reorder1 -lt 100 ] && [ $reorder2 -gt 0 ]; then
+	if [ $reorder_delay -gt 0 ] && [ $reorder1 -lt 100 ] && [ $reorder2 -gt 0 ]; then
 		tc_reorder="reorder ${reorder1}% ${reorder2}%"
 		echo -n "$tc_reorder "
 	fi
 elif [ "$tc_reorder" = "0" ];then
 	tc_reorder=""
-elif [ "$tc_delay" -gt 0 ];then
+elif [ "$reorder_delay" -gt 0 ];then
 	# reordering requires some delay
 	tc_reorder="reorder $tc_reorder"
 	echo -n "$tc_reorder "
@@ -654,7 +655,7 @@  fi
 
 echo "on ns3eth4"
 
-tc -net "$ns3" qdisc add dev ns3eth4 root netem delay ${tc_delay}ms $tc_reorder
+tc -net "$ns3" qdisc add dev ns3eth4 root netem delay ${reorder_delay}ms $tc_reorder
 
 for sender in $ns1 $ns2 $ns3 $ns4;do
 	run_tests_lo "$ns1" "$sender" 10.0.1.1 1