Re: [PATCH mptcp-next v2 1/2] selftests: net: mptcp: add checks for fallback counters

[Matthieu Baerts <matttbe@kernel.org>]

Hi Gang,

On 14/08/2025 05:16, Gang Yan wrote:
> Recently, some mib counters about fallback has been added, this patch
> provides a helper to check the expected behavior of these mib counters
> during the test execution.

Thank you for the patch.

But alone, it doesn't make sense, and it is harder to review without
knowing how the helper is going to be used. Better to squash the two
patches together.

I think having patches dedicated to the introduction of new helpers only
make sense when such helpers will be used on another subsystem
(different maintainers) or when moving existing code to a new helper to
be re-used elsewhere. But here, I think it causes more troubles.

> 
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/571
> Signed-off-by: Gang Yan <yangang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 98 +++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index b8af65373b3a..2c1f0704bc17 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -74,6 +74,15 @@ unset join_create_err
>  unset join_bind_err
>  unset join_connect_err
>  
> +unset infinite_map_tx_fb
> +unset dss_corruption_fb
> +unset simult_conn_fb
> +unset mpc_passive_fb
> +unset mpc_active_fb
> +unset mpc_data_fb
> +unset MD5_Sig_fb

No need to use capital letters here.

> +unset dss_fb

It might make more sense to use the "fb_" prefix, instead of the suffix,
similar to what has been done with "join_" above.

> +
>  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
>  #				  (ip6 && (ip6[74] & 0xf0) == 0x30)'"
>  CBPF_MPTCP_SUBOPTION_ADD_ADDR="14,
> @@ -1399,6 +1408,95 @@ chk_join_tx_nr()
>  	print_results "join Tx" ${rc}
>  }
>  
> +chk_fallback_nr()
> +{
> +	local infinite_map_tx=${infinite_map_tx_fb:-0}
> +	local dss_corruption=${dss_corruption_fb:-0}
> +	local simult_conn=${simult_conn_fb:-0}
> +	local mpc_passive=${mpc_passive_fb:-0}
> +	local mpc_active=${mpc_active_fb:-0}
> +	local mpc_data=${mpc_data_fb:-0}
> +	local MD5_Sig=${MD5_Sig_fb:-0}
> +	local dss=${dss_fb:-0}

No need to respect the reverse Xmas tree declaration for the variables
here in bash. I think it is better here (and above with the unset) to
follow the same order as below, when they are used, so it is easy to
check if they are all used correctly.

> +	local rc=${KSFT_PASS}
> +	local ns=$1
> +	local count
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCapableFallbackACK")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$mpc_passive" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "mpc passive fallback "

All your "print_check" here and below have an extra whitespace before
the last double quote: why? I don't think it is needed.

> +		fail_test "got $count mpc passive fallback[s] expected $mpc_passive"

Because this helper is used twice, we need to know which direction had
an issue: can you pass that info (connector/listener or ns2/ns1) as
argument to this helper, and adding it to "print_check" and "fail_test"?
Same below.
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCapableFallbackSYNACK")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$mpc_active" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "mpc active fallback "
> +		fail_test "got $count mpc active fallback[s] expected $mpc_active"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtDSSCorruptionFallback")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$dss_corruption" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "dss corruption fallback "
> +		fail_test "got $count dss corruption fallback[s] expected $dss_corruption"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtInfiniteMapTx")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$infinite_map_tx" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "infinite map tx fallback "
> +		fail_test "got $count infinite map tx fallback[s] expected $infinite_map_tx"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCapableDataFallback")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$mpc_data" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "mpc data fallback "
> +		fail_test "got $count mpc data fallback[s] expected $mpc_data"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMD5SigFallback")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$MD5_Sig" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "MD5 Sig fallback "
> +		fail_test "got $count MD5 Sig fallback[s] expected $MD5_Sig"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtDssFallback")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$dss" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "dss fallback "
> +		fail_test "got $count dss fallback[s] expected $dss"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtSimultConnectFallback")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$simult_conn" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "simult conn fallback "
> +		fail_test "got $count simult conn fallback[s] expected $simult_conn"
> +	fi
> +
> +	print_results "check fallback nr" ${rc}

Just "fallback"?

EDIT: see the next patch: you should have a different message per netns.

> +}
> +
>  chk_join_nr()
>  {
>  	local syn_nr=$1

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