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

[Geliang Tang <geliang@kernel.org>]

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