Re: [PATCH mptcp-next v3 3/3] selftests: mptcp: add last time actions tests

[Mat Martineau <martineau@kernel.org>]

On Sat, 30 Mar 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch adds a new helper chk_msk_info() to show the counters in
> mptcp_info of the given infos, and check that the timestamps move
> forward. Use it to show newly added last_data_sent, last_data_recv
> and last_ack_recv in mptcp_info in diag.sh.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> tools/testing/selftests/net/mptcp/diag.sh | 29 +++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index bc97ab33a00e..2f9f734015b4 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -200,6 +200,34 @@ chk_msk_cestab()
> 		 "${expected}" "${msg}" ""
> }
>
> +chk_msk_info()
> +{
> +	local port="${1}"
> +	local info
> +
> +	for info in "${@:2}"; do
> +		local cnt1 cnt2 msg
> +
> +		cnt1=$(ss -N ${ns} -inHM dport ${port} | mptcp_lib_get_info_value "$info" "$info")
> +		sleep 0.5

Hi Geliang -

With the loop structured this way, it sleeps 3 times. You could collect 
the first timestamp for all three values (last_data_sent last_data_recv 
last_ack_recv), then sleep once, then get all 3 "after" timestamps.

(Unless Matthieu doesn't care about the extra test time :) )

> +		cnt2=$(ss -N ${ns} -inHM dport ${port} | mptcp_lib_get_info_value "$info" "$info")
> +		msg="....chk ${info}"
> +		mptcp_lib_print_title "${msg}"
> +		if { [ -z "${cnt1}" ] || [ -z "${cnt2}" ]; } &&
> +		   ! mptcp_lib_expect_all_features; then
> +			mptcp_lib_pr_skip "Feature probably not supported"
> +			mptcp_lib_result_skip "${msg}"
> +		elif [ "$((cnt1 + 500))" -lt "${cnt2}" ]; then

Using 500 here is likely to make this test flaky - Matthieu had suggested 
250ms as a way to make sure the value moved forward but would not be 
sensitive to small changes in packet timing.

Also please add a local "delta_ms" variable instead of the magic number 
500.

> +			mptcp_lib_pr_ok
> +			mptcp_lib_result_pass "${msg}"
> +		else
> +			mptcp_lib_pr_fail "expected $((cnt1 + 500)) < $cnt2"

Suggest rephrasing: "value of ${info} changed by $((cnt2 - cnt1))ms, 
expected at least ${delta_ms}ms"

- Mat

> +			mptcp_lib_result_fail "${msg}"
> +			ret=${KSFT_FAIL}
> +		fi
> +	done
> +}
> +
> wait_connected()
> {
> 	local listener_ns="${1}"
> @@ -233,6 +261,7 @@ echo "b" | \
> 				127.0.0.1 >/dev/null &
> wait_connected $ns 10000
> chk_msk_nr 2 "after MPC handshake "
> +chk_msk_info 10000 last_data_sent last_data_recv last_ack_recv
> chk_msk_remote_key_nr 2 "....chk remote_key"
> chk_msk_fallback_nr 0 "....chk no fallback"
> chk_msk_inuse 2
> -- 
> 2.40.1
>
>
>