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

[Matthieu Baerts <matttbe@kernel.org>]

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.