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.0On 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.
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
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.
© 2016 - 2025 Red Hat, Inc.