[PATCH mptcp-next 0/3] mptcp: fixes for adaptive ADD_ADDR retransmission timeout

[PATCH mptcp-next 0/3] mptcp: fixes for adaptive ADD_ADDR retransmission timeout

[Matthieu Baerts (NGI0)]

The recent patch "mptcp: make ADD_ADDR retransmission timeout adaptive"
seems causing some unstable tests, see the links below.

The modification in the selftest is reverted, and more tolerance is
added instead.

While at it, slow down some signalling tests.

Link: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17391054770
Link: https://netdev-3.bots.linux.dev/vmksft-mptcp-dbg/results/280241/1-mptcp-join-sh/stdout
Link: https://netdev-3.bots.linux.dev/vmksft-mptcp-dbg/results/279881/1-mptcp-join-sh/stdout
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (3):
      Squash to "selftests: mptcp: remove add_addr_timeout settings"
      selftests: mptcp: join: tolerate more ADD_ADDR
      selftests: mptcp: join: allow more time to send ADD_ADDR

 tools/testing/selftests/net/mptcp/mptcp_join.sh | 30 ++++++++++++-------------
 1 file changed, 14 insertions(+), 16 deletions(-)
---
base-commit: 8984ec428a58d1d82ba89db40e462ad1e8dd317e
change-id: 20250902-sft-mptcp-join-add_addr-no-retrans-345b5db46fac

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next 1/3] Squash to "selftests: mptcp: remove add_addr_timeout settings"

[Matthieu Baerts (NGI0)]

This reverts the patch: when add_addr_timeout is set to 1, chk_add_nr()
and chk_add_tx_nr() are more tolerant with retransmissions, see commit
6ef84b1517e0 ("selftests: mptcp: more robust signal race test").

But the main reason is that this revert breaks the selftests running on
old kernels.

It is then important to keep this. Plus it still acts as a maximum, just
in case everything is very slow, we still have retransmissions in time,
especially with the new exponential backoff.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index a97b568104bc284f050b2f0e09fe3fdd3341c5cb..2f046167a0b6cc6fb5531a033d8d95c9ea399cf9 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -358,6 +358,8 @@ reset_with_add_addr_timeout()
 		tables="${ip6tables}"
 	fi
 
+	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
+
 	if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \
 			-m tcp --tcp-option 30 \
 			-m bpf --bytecode \
@@ -2303,6 +2305,7 @@ signal_address_tests()
 		pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
 
 		# the peer could possibly miss some addr notification, allow retransmission
+		ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 

-- 
2.51.0

[PATCH mptcp-next 2/3] selftests: mptcp: join: tolerate more ADD_ADDR

[Matthieu Baerts (NGI0)]

ADD_ADDR can be retransmitted, and with, the parent commit, these
retransmissions can be sent quicker: from 2 minutes to less than one
second.

To avoid false positives where retransmitted ADD_ADDR causes higher
counters than expected, it is required to be more tolerant. Errors are
now only reported when fewer ADD_ADDRs have been sent/received.

An alternative could be to disable the ADD_ADDR retransmissions by
default, but that's changing the default kernel behaviour. Plus,
ADD_ADDR retransmissions can be required for some tests. To avoid adding
exceptions to a few tests, it seems better to increase the tolerance.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 2f046167a0b6cc6fb5531a033d8d95c9ea399cf9..1028f3f931612d98e828707a9f55caa69be4a6ca 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -358,6 +358,7 @@ reset_with_add_addr_timeout()
 		tables="${ip6tables}"
 	fi
 
+	# set a maximum, to avoid too long timeout with exponential backoff
 	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
 
 	if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \
@@ -1669,7 +1670,6 @@ chk_add_nr()
 	local tx=""
 	local rx=""
 	local count
-	local timeout
 
 	if [[ $ns_invert = "invert" ]]; then
 		ns_tx=$ns2
@@ -1678,16 +1678,13 @@ chk_add_nr()
 		rx=" server"
 	fi
 
-	timeout=$(ip netns exec ${ns_tx} sysctl -n net.mptcp.add_addr_timeout)
-
 	print_check "add addr rx${rx}"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
 	if [ -z "$count" ]; then
 		print_skip
-	# if the test configured a short timeout tolerate greater then expected
-	# add addrs options, due to retransmissions
-	elif [ "$count" != "$add_nr" ] && { [ "$timeout" -gt 1 ] || [ "$count" -lt "$add_nr" ]; }; then
-		fail_test "got $count ADD_ADDR[s] expected $add_nr"
+	# Tolerate more ADD_ADDR then expected, due to retransmissions
+	elif [ "$count" -lt "$add_nr" ]; then
+		fail_test "got $count ADD_ADDR[s] expected $add_nr or more"
 	else
 		print_ok
 	fi
@@ -1774,19 +1771,15 @@ chk_add_tx_nr()
 {
 	local add_tx_nr=$1
 	local echo_tx_nr=$2
-	local timeout
 	local count
 
-	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
-
 	print_check "add addr tx"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTx")
 	if [ -z "$count" ]; then
 		print_skip
-	# if the test configured a short timeout tolerate greater then expected
-	# add addrs options, due to retransmissions
-	elif [ "$count" != "$add_tx_nr" ] && { [ "$timeout" -gt 1 ] || [ "$count" -lt "$add_tx_nr" ]; }; then
-		fail_test "got $count ADD_ADDR[s] TX, expected $add_tx_nr"
+	# Tolerate more ADD_ADDR then expected, due to retransmissions
+	elif [ "$count" -lt "$add_tx_nr" ]; then
+		fail_test "got $count ADD_ADDR[s] TX, expected $add_tx_nr or more"
 	else
 		print_ok
 	fi

-- 
2.51.0

[PATCH mptcp-next 3/3] selftests: mptcp: join: allow more time to send ADD_ADDR

[Matthieu Baerts (NGI0)]

When many ADD_ADDR need to be sent, it can take some time to send each
of them, and create new subflows. Some CIs seem to occasionally have
issues with these tests, especially with "debug" kernels.

Two subtests will now run for a slightly longer time: the last two where
3 or more ADD_ADDR are sent during the test.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 1028f3f931612d98e828707a9f55caa69be4a6ca..9c1ab36110850dcc35bb77e2d5894d8b1dd496a0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2266,7 +2266,8 @@ signal_address_tests()
 		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
 		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal
 		pm_nl_set_limits $ns2 3 3
-		run_tests $ns1 $ns2 10.0.1.1
+		speed=slow \
+			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 3 3 3
 		chk_add_nr 3 3
 	fi
@@ -2278,7 +2279,8 @@ signal_address_tests()
 		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
 		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal
 		pm_nl_set_limits $ns2 3 3
-		run_tests $ns1 $ns2 10.0.1.1
+		speed=slow \
+			run_tests $ns1 $ns2 10.0.1.1
 		join_syn_tx=3 \
 			chk_join_nr 1 1 1
 		chk_add_nr 3 3

-- 
2.51.0

Re: [PATCH mptcp-next 0/3] mptcp: fixes for adaptive ADD_ADDR retransmission timeout

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Unstable: 3 failed test(s): packetdrill_fastopen packetdrill_mp_capable packetdrill_syscalls 🔴
- KVM Validation: btf-normal (only bpftest_all): Unstable: 1 failed test(s): bpftest_test_progs_mptcp 🔴
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17413711600

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/5b20b8115e21
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=998127


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next 1/3] Squash to "selftests: mptcp: remove add_addr_timeout settings"

[Geliang Tang]

Hi Matt,

On Tue, 2025-09-02 at 21:01 +0200, Matthieu Baerts (NGI0) wrote:
> This reverts the patch: when add_addr_timeout is set to 1,
> chk_add_nr()
> and chk_add_tx_nr() are more tolerant with retransmissions, see
> commit
> 6ef84b1517e0 ("selftests: mptcp: more robust signal race test").
> 
> But the main reason is that this revert breaks the selftests running
> on
> old kernels.
> 
> It is then important to keep this. Plus it still acts as a maximum,
> just
> in case everything is very slow, we still have retransmissions in
> time,
> especially with the new exponential backoff.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

I agree.

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index
> a97b568104bc284f050b2f0e09fe3fdd3341c5cb..2f046167a0b6cc6fb5531a033d8
> d95c9ea399cf9 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -358,6 +358,8 @@ reset_with_add_addr_timeout()
>  		tables="${ip6tables}"
>  	fi
>  
> +	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
> +
>  	if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \
>  			-m tcp --tcp-option 30 \
>  			-m bpf --bytecode \
> @@ -2303,6 +2305,7 @@ signal_address_tests()
>  		pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
>  
>  		# the peer could possibly miss some addr
> notification, allow retransmission
> +		ip netns exec $ns1 sysctl -q
> net.mptcp.add_addr_timeout=1
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  

Re: [PATCH mptcp-next 3/3] selftests: mptcp: join: allow more time to send ADD_ADDR

[Geliang Tang]

Hi Matt,

On Tue, 2025-09-02 at 21:01 +0200, Matthieu Baerts (NGI0) wrote:
> When many ADD_ADDR need to be sent, it can take some time to send
> each
> of them, and create new subflows. Some CIs seem to occasionally have
> issues with these tests, especially with "debug" kernels.
> 
> Two subtests will now run for a slightly longer time: the last two
> where
> 3 or more ADD_ADDR are sent during the test.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

LGTM!

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index
> 1028f3f931612d98e828707a9f55caa69be4a6ca..9c1ab36110850dcc35bb77e2d58
> 94d8b1dd496a0 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2266,7 +2266,8 @@ signal_address_tests()
>  		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
>  		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal
>  		pm_nl_set_limits $ns2 3 3
> -		run_tests $ns1 $ns2 10.0.1.1
> +		speed=slow \
> +			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 3 3 3
>  		chk_add_nr 3 3
>  	fi
> @@ -2278,7 +2279,8 @@ signal_address_tests()
>  		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
>  		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal
>  		pm_nl_set_limits $ns2 3 3
> -		run_tests $ns1 $ns2 10.0.1.1
> +		speed=slow \
> +			run_tests $ns1 $ns2 10.0.1.1
>  		join_syn_tx=3 \
>  			chk_join_nr 1 1 1
>  		chk_add_nr 3 3

Re: [PATCH mptcp-next 2/3] selftests: mptcp: join: tolerate more ADD_ADDR

[Geliang Tang]

Hi Matt,

On Tue, 2025-09-02 at 21:01 +0200, Matthieu Baerts (NGI0) wrote:
> ADD_ADDR can be retransmitted, and with, the parent commit, these
> retransmissions can be sent quicker: from 2 minutes to less than one
> second.
> 
> To avoid false positives where retransmitted ADD_ADDR causes higher
> counters than expected, it is required to be more tolerant. Errors
> are
> now only reported when fewer ADD_ADDRs have been sent/received.
> 
> An alternative could be to disable the ADD_ADDR retransmissions by
> default, but that's changing the default kernel behaviour. Plus,
> ADD_ADDR retransmissions can be required for some tests. To avoid
> adding
> exceptions to a few tests, it seems better to increase the tolerance.

I think adding "tolerate more ADD_ADDR" to the two general helpers
chk_add_nr() and chk_add_tx_nr() is not very accurate, because we don't
know which test sends/receives more ADD_ADDR. Because chk_add_nr() and
chk_add_tx_nr() of most tests are normal, only a few tests will be
affected and need to add "tolerate".

I think it can be handled like chk_csum_nr(), accepting a number with
"+", such as "+2" to allow more than two ADD_ADDR. In this way, we only
need to add this "+" to a few failed tests, so the control is more
accurate. What do you think?

Thanks,
-Geliang

> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 21 +++++++--------
> ------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index
> 2f046167a0b6cc6fb5531a033d8d95c9ea399cf9..1028f3f931612d98e828707a9f5
> 5caa69be4a6ca 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -358,6 +358,7 @@ reset_with_add_addr_timeout()
>  		tables="${ip6tables}"
>  	fi
>  
> +	# set a maximum, to avoid too long timeout with exponential
> backoff
>  	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
>  
>  	if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \
> @@ -1669,7 +1670,6 @@ chk_add_nr()
>  	local tx=""
>  	local rx=""
>  	local count
> -	local timeout
>  
>  	if [[ $ns_invert = "invert" ]]; then
>  		ns_tx=$ns2
> @@ -1678,16 +1678,13 @@ chk_add_nr()
>  		rx=" server"
>  	fi
>  
> -	timeout=$(ip netns exec ${ns_tx} sysctl -n
> net.mptcp.add_addr_timeout)
> -
>  	print_check "add addr rx${rx}"
>  	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
>  	if [ -z "$count" ]; then
>  		print_skip
> -	# if the test configured a short timeout tolerate greater
> then expected
> -	# add addrs options, due to retransmissions
> -	elif [ "$count" != "$add_nr" ] && { [ "$timeout" -gt 1 ] ||
> [ "$count" -lt "$add_nr" ]; }; then
> -		fail_test "got $count ADD_ADDR[s] expected $add_nr"
> +	# Tolerate more ADD_ADDR then expected, due to
> retransmissions
> +	elif [ "$count" -lt "$add_nr" ]; then
> +		fail_test "got $count ADD_ADDR[s] expected $add_nr
> or more"
>  	else
>  		print_ok
>  	fi
> @@ -1774,19 +1771,15 @@ chk_add_tx_nr()
>  {
>  	local add_tx_nr=$1
>  	local echo_tx_nr=$2
> -	local timeout
>  	local count
>  
> -	timeout=$(ip netns exec $ns1 sysctl -n
> net.mptcp.add_addr_timeout)
> -
>  	print_check "add addr tx"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> -	# if the test configured a short timeout tolerate greater
> then expected
> -	# add addrs options, due to retransmissions
> -	elif [ "$count" != "$add_tx_nr" ] && { [ "$timeout" -gt 1 ]
> || [ "$count" -lt "$add_tx_nr" ]; }; then
> -		fail_test "got $count ADD_ADDR[s] TX, expected
> $add_tx_nr"
> +	# Tolerate more ADD_ADDR then expected, due to
> retransmissions
> +	elif [ "$count" -lt "$add_tx_nr" ]; then
> +		fail_test "got $count ADD_ADDR[s] TX, expected
> $add_tx_nr or more"
>  	else
>  		print_ok
>  	fi

Re: [PATCH mptcp-next 2/3] selftests: mptcp: join: tolerate more ADD_ADDR

[Matthieu Baerts]

Hi Geliang,

On 04/09/2025 11:26, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2025-09-02 at 21:01 +0200, Matthieu Baerts (NGI0) wrote:
>> ADD_ADDR can be retransmitted, and with, the parent commit, these
>> retransmissions can be sent quicker: from 2 minutes to less than one
>> second.
>>
>> To avoid false positives where retransmitted ADD_ADDR causes higher
>> counters than expected, it is required to be more tolerant. Errors
>> are
>> now only reported when fewer ADD_ADDRs have been sent/received.
>>
>> An alternative could be to disable the ADD_ADDR retransmissions by
>> default, but that's changing the default kernel behaviour. Plus,
>> ADD_ADDR retransmissions can be required for some tests. To avoid
>> adding
>> exceptions to a few tests, it seems better to increase the tolerance.
> 
> I think adding "tolerate more ADD_ADDR" to the two general helpers
> chk_add_nr() and chk_add_tx_nr() is not very accurate, because we don't
> know which test sends/receives more ADD_ADDR. Because chk_add_nr() and
> chk_add_tx_nr() of most tests are normal, only a few tests will be
> affected and need to add "tolerate".
> 
> I think it can be handled like chk_csum_nr(), accepting a number with
> "+", such as "+2" to allow more than two ADD_ADDR. In this way, we only
> need to add this "+" to a few failed tests, so the control is more
> accurate. What do you think?

I initially did something similar: marking the tests where we can be
more tolerant, but the modifications were quite important. But that was
not it: before, there were no retransmissions (only after 2 minutes, so
after the end of each subtest), and we had the tolerance only when
setting the timeout to 1 second. Now, we always have retransmissions
(and we don't control the timeout any more), so to be aligned with what
we had before, we should probably always have this tolerance. We might
have unexpected retransmissions because suddenly, the host got busy, and
the ADD_ADDR retransmission timer fires. We don't want to be impacted by
these unexpected events. I think that's valid for all tests were we
check the ADD_ADDR.

Maybe what is missing is an exception for the expected counter 0: if we
expect no ADD_ADDR, we should fail if some are seen. But this issue was
already there, we would need another patch for that, probably. (I can look).

Also, what would be certainly better here, is to add a new MIB counters
for the retransmissions, so we can check the actual number instead of
having the tolerance. Again, that's orthogonal to this fix I think, so
this can be done later on.

So in short:
- this patch simply align with what was already there
- we should add an exception: no tolerance when expecting no ADD_ADDR
- we might add a new MIB counter (sent retransmitted ADD_ADDR) to avoid
the tolerance (on newer kernels only: when this counter is available)

WDYT?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next 2/3] selftests: mptcp: join: tolerate more ADD_ADDR

[Matthieu Baerts]

Hi Geliang,

On 04/09/2025 12:47, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/09/2025 11:26, Geliang Tang wrote:
>> Hi Matt,
>>
>> On Tue, 2025-09-02 at 21:01 +0200, Matthieu Baerts (NGI0) wrote:
>>> ADD_ADDR can be retransmitted, and with, the parent commit, these
>>> retransmissions can be sent quicker: from 2 minutes to less than one
>>> second.
>>>
>>> To avoid false positives where retransmitted ADD_ADDR causes higher
>>> counters than expected, it is required to be more tolerant. Errors
>>> are
>>> now only reported when fewer ADD_ADDRs have been sent/received.
>>>
>>> An alternative could be to disable the ADD_ADDR retransmissions by
>>> default, but that's changing the default kernel behaviour. Plus,
>>> ADD_ADDR retransmissions can be required for some tests. To avoid
>>> adding
>>> exceptions to a few tests, it seems better to increase the tolerance.
>>
>> I think adding "tolerate more ADD_ADDR" to the two general helpers
>> chk_add_nr() and chk_add_tx_nr() is not very accurate, because we don't
>> know which test sends/receives more ADD_ADDR. Because chk_add_nr() and
>> chk_add_tx_nr() of most tests are normal, only a few tests will be
>> affected and need to add "tolerate".
>>
>> I think it can be handled like chk_csum_nr(), accepting a number with
>> "+", such as "+2" to allow more than two ADD_ADDR. In this way, we only
>> need to add this "+" to a few failed tests, so the control is more
>> accurate. What do you think?
> 
> I initially did something similar: marking the tests where we can be
> more tolerant, but the modifications were quite important. But that was
> not it: before, there were no retransmissions (only after 2 minutes, so
> after the end of each subtest), and we had the tolerance only when
> setting the timeout to 1 second. Now, we always have retransmissions
> (and we don't control the timeout any more), so to be aligned with what
> we had before, we should probably always have this tolerance. We might
> have unexpected retransmissions because suddenly, the host got busy, and
> the ADD_ADDR retransmission timer fires. We don't want to be impacted by
> these unexpected events. I think that's valid for all tests were we
> check the ADD_ADDR.
> 
> Maybe what is missing is an exception for the expected counter 0: if we
> expect no ADD_ADDR, we should fail if some are seen. But this issue was
> already there, we would need another patch for that, probably. (I can look).

To be more correct here: the issue was already there before, but we
couldn't hit it because "net.mptcp.add_addr_timeout" was only set to 1
(check with tolerance) when we expected to see ADD_ADDR. In other words,
no need to have an exception for 0 before this patch. I will then modify
this one.
> Also, what would be certainly better here, is to add a new MIB counters
> for the retransmissions, so we can check the actual number instead of
> having the tolerance. Again, that's orthogonal to this fix I think, so
> this can be done later on.

(Note that this might be nice, but I'm currently not planning to add this)

> 
> So in short:
> - this patch simply align with what was already there
> - we should add an exception: no tolerance when expecting no ADD_ADDR
> - we might add a new MIB counter (sent retransmitted ADD_ADDR) to avoid
> the tolerance (on newer kernels only: when this counter is available)
> 
> WDYT?
> 
> Cheers,
> Matt

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.