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

[PATCH mptcp-next v2 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>
---
Changes in v2:
- Patch 2: tolerate more ADD_ADDR, except if no ADD_ADDR are expected.
- Add Geliang's RvB tags (thanks!)
- Link to v1: https://lore.kernel.org/r/20250902-sft-mptcp-join-add_addr-no-retrans-v1-0-956a315d6a5a@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 | 28 ++++++++++++-------------
 1 file changed, 14 insertions(+), 14 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 v2 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.

Reviewed-by: Geliang Tang <geliang@kernel.org>
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 v2 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, except
if no ADD_ADDR are expected.

Before the parent commit, the tolerance was present for each tests where
the ADD_ADDR could be retransmitted in a reasonable time (1 sec). Now
that all tests can have retransmitted ADD_ADDR, it is normal to apply
the same tolerance for all tests.

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 many tests, it seems better to increase the tolerance.

Later, we could add a new MIB counter to identify the ADD_ADDR
retransmissions, and remove the tolerance when this counter is
available.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 2f046167a0b6cc6fb5531a033d8d95c9ea399cf9..be49712f91155a1f14f6cc21776764d65a897c27 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,15 +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
+	# Tolerate more ADD_ADDR then expected (if any), due to retransmissions
+	elif [ "$count" -lt "$add_nr" ] ||
+	     { [ "$count" != "$add_nr" ] && [ "$add_nr" != 0 ]; }; then
 		fail_test "got $count ADD_ADDR[s] expected $add_nr"
 	else
 		print_ok
@@ -1774,18 +1772,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
+	# Tolerate more ADD_ADDR then expected (if any), due to retransmissions
+	elif [ "$count" -lt "$add_tx_nr" ] ||
+	     { [ "$count" != "$add_tx_nr" ] && [ "$add_tx_nr" != 0 ]; }; then
 		fail_test "got $count ADD_ADDR[s] TX, expected $add_tx_nr"
 	else
 		print_ok

-- 
2.51.0

[PATCH mptcp-next v2 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.

Reviewed-by: Geliang Tang <geliang@kernel.org>
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 be49712f91155a1f14f6cc21776764d65a897c27..dc1f7d5b7fbaf5aa71d95c0d3e0b418cf3916199 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2268,7 +2268,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
@@ -2280,7 +2281,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 v2 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: Unstable: 1 failed test(s): selftest_mptcp_connect 🔴
- KVM Validation: debug: Unstable: 3 failed test(s): packetdrill_add_addr selftest_mptcp_connect_checksum selftest_mptcp_join 🔴
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17463051487

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


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 v2 2/3] selftests: mptcp: join: tolerate more ADD_ADDR

[Matthieu Baerts]

On 04/09/2025 13:38, 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, except
> if no ADD_ADDR are expected.
> 
> Before the parent commit, the tolerance was present for each tests where
> the ADD_ADDR could be retransmitted in a reasonable time (1 sec). Now
> that all tests can have retransmitted ADD_ADDR, it is normal to apply
> the same tolerance for all tests.
> 
> 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 many tests, it seems better to increase the tolerance.
> 
> Later, we could add a new MIB counter to identify the ADD_ADDR
> retransmissions, and remove the tolerance when this counter is
> available.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 2f046167a0b6cc6fb5531a033d8d95c9ea399cf9..be49712f91155a1f14f6cc21776764d65a897c27 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,15 +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
> +	# Tolerate more ADD_ADDR then expected (if any), due to retransmissions
> +	elif [ "$count" -lt "$add_nr" ] ||
> +	     { [ "$count" != "$add_nr" ] && [ "$add_nr" != 0 ]; }; then

Arf, it should be:

  "$add_nr" == 0
>  		fail_test "got $count ADD_ADDR[s] expected $add_nr"
>  	else
>  		print_ok
> @@ -1774,18 +1772,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
> +	# Tolerate more ADD_ADDR then expected (if any), due to retransmissions
> +	elif [ "$count" -lt "$add_tx_nr" ] ||
> +	     { [ "$count" != "$add_tx_nr" ] && [ "$add_tx_nr" != 0 ]; }; then

Same here:

  "$add_tx_nr" == 0

If there is nothing else, I can fix that when applying the patches.

>  		fail_test "got $count ADD_ADDR[s] TX, expected $add_tx_nr"
>  	else
>  		print_ok
> 

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

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

[Geliang Tang]

Hi Matt,

Thanks for this new version, and thanks for the explanation in v1.

On Thu, 2025-09-04 at 16:54 +0200, Matthieu Baerts wrote:
> On 04/09/2025 13:38, 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,
> > except
> > if no ADD_ADDR are expected.
> > 
> > Before the parent commit, the tolerance was present for each tests
> > where
> > the ADD_ADDR could be retransmitted in a reasonable time (1 sec).
> > Now
> > that all tests can have retransmitted ADD_ADDR, it is normal to
> > apply
> > the same tolerance for all tests.
> > 
> > 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 many tests, it seems better to increase the
> > tolerance.
> > 
> > Later, we could add a new MIB counter to identify the ADD_ADDR
> > retransmissions, and remove the tolerance when this counter is
> > available.
> > 
> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 19 +++++++------
> > ------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index
> > 2f046167a0b6cc6fb5531a033d8d95c9ea399cf9..be49712f91155a1f14f6cc217
> > 76764d65a897c27 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,15 +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
> > +	# Tolerate more ADD_ADDR then expected (if any), due to
> > retransmissions
> > +	elif [ "$count" -lt "$add_nr" ] ||
> > +	     { [ "$count" != "$add_nr" ] && [ "$add_nr" != 0 ]; };
> > then
> 
> Arf, it should be:
> 
>   "$add_nr" == 0

It should indeed be "$add_nr" == 0.

nit:

Is it possible to keep the same order as before when checking
"$timeout", just replace [ "$timeout" -gt 1 ] with [ "$add_nr" == 0 ]?

That is:

elif [ "$count" != "$add_nr" ] && { [ "$add_nr" == 0 ] || [ "$count" -
lt "$add_nr" ]; }; then

Although they are actually the same, I usually try to keep it
consistent with the original code.

> >  		fail_test "got $count ADD_ADDR[s] expected
> > $add_nr"
> >  	else
> >  		print_ok
> > @@ -1774,18 +1772,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
> > +	# Tolerate more ADD_ADDR then expected (if any), due to
> > retransmissions
> > +	elif [ "$count" -lt "$add_tx_nr" ] ||
> > +	     { [ "$count" != "$add_tx_nr" ] && [ "$add_tx_nr" != 0
> > ]; }; then
> 
> Same here:
> 
>   "$add_tx_nr" == 0
> 
> If there is nothing else, I can fix that when applying the patches.

No need to send a v3, please fix it when applying it.

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

Thanks,
-Geliang

> 
> >  		fail_test "got $count ADD_ADDR[s] TX, expected
> > $add_tx_nr"
> >  	else
> >  		print_ok
> > 
> 
> Cheers,
> Matt

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

[Matthieu Baerts]

Hi Geliang,

On 05/09/2025 10:16, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for this new version, and thanks for the explanation in v1.
> 
> On Thu, 2025-09-04 at 16:54 +0200, Matthieu Baerts wrote:
>> On 04/09/2025 13:38, 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,
>>> except
>>> if no ADD_ADDR are expected.
>>>
>>> Before the parent commit, the tolerance was present for each tests
>>> where
>>> the ADD_ADDR could be retransmitted in a reasonable time (1 sec).
>>> Now
>>> that all tests can have retransmitted ADD_ADDR, it is normal to
>>> apply
>>> the same tolerance for all tests.
>>>
>>> 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 many tests, it seems better to increase the
>>> tolerance.
>>>
>>> Later, we could add a new MIB counter to identify the ADD_ADDR
>>> retransmissions, and remove the tolerance when this counter is
>>> available.
>>>
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>> ---
>>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 19 +++++++------
>>> ------
>>>  1 file changed, 7 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> index
>>> 2f046167a0b6cc6fb5531a033d8d95c9ea399cf9..be49712f91155a1f14f6cc217
>>> 76764d65a897c27 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,15 +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
>>> +	# Tolerate more ADD_ADDR then expected (if any), due to
>>> retransmissions
>>> +	elif [ "$count" -lt "$add_nr" ] ||
>>> +	     { [ "$count" != "$add_nr" ] && [ "$add_nr" != 0 ]; };
>>> then
>>
>> Arf, it should be:
>>
>>   "$add_nr" == 0
> 
> It should indeed be "$add_nr" == 0.
> 
> nit:
> 
> Is it possible to keep the same order as before when checking
> "$timeout", just replace [ "$timeout" -gt 1 ] with [ "$add_nr" == 0 ]?
> 
> That is:
> 
> elif [ "$count" != "$add_nr" ] && { [ "$add_nr" == 0 ] || [ "$count" -
> lt "$add_nr" ]; }; then
> 
> Although they are actually the same, I usually try to keep it
> consistent with the original code.

Good point, even if I found the check "-lt || (-gt && = 0)" maybe more
logical, with the v2, we are closer to the original code, I can do this
modification.

> 
>>>  		fail_test "got $count ADD_ADDR[s] expected
>>> $add_nr"
>>>  	else
>>>  		print_ok
>>> @@ -1774,18 +1772,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
>>> +	# Tolerate more ADD_ADDR then expected (if any), due to
>>> retransmissions
>>> +	elif [ "$count" -lt "$add_tx_nr" ] ||
>>> +	     { [ "$count" != "$add_tx_nr" ] && [ "$add_tx_nr" != 0
>>> ]; }; then
>>
>> Same here:
>>
>>   "$add_tx_nr" == 0
>>
>> If there is nothing else, I can fix that when applying the patches.
> 
> No need to send a v3, please fix it when applying it.
> 
> Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks!

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

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

[Matthieu Baerts]

Hi Geliang,

On 04/09/2025 13:38, Matthieu Baerts (NGI0) wrote:
> 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.

Thank you for the review!

Now in our tree, with the suggested fix:

New patches for t/upstream:
- a37ad3e9fe07: "squashed" patch 1/3 in "selftests: mptcp: remove
add_addr_timeout settings"
- 97cd737c96db: selftests: mptcp: join: tolerate more ADD_ADDR
- 1782e66db572: selftests: mptcp: join: allow more time to send ADD_ADDR
- Results: b908f5af710b..9335a0684b09 (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/f109be59e49a8e13d97704bde7c726ff26835034/checks

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