[PATCH mptcp-next v3 0/8] mptcp: MIB counters for sent MP_JOIN

[PATCH mptcp-next v3 0/8] mptcp: MIB counters for sent MP_JOIN

[Matthieu Baerts (NGI0)]

Recently, a few issues have been discovered around the creation of
additional subflows. Without these counters, it was difficult to point
out the reason why some subflows were not created as expected.

In patch 2, all error paths from __mptcp_subflow_connect() are covered,
except the one related to the 'fully established mode', because it
should only happen with the userspace PM, which will propagate the error
in this  case (ENOTCONN).

These new counters are also verified in the MPTCP Join selftest in patch
5.

While at it, a few other patches are improving the MPJoin selftests:

 - Patch 1: a fix when using an old kernel
 - Patch 3: reduce the number of positional parameters
 - Patch 4: only one line for the 'join' checks, instead of 3
 - Patch 6: more explicit check names, instead of sometimes too cryptic
            ones: rtx, ptx, ftx, ctx, fclzrx, sum
 - Patch 7: specify client/server instead of 'invert' for some checks
            not suggesting one direction
 - Patch 8: mute errors of mptcp_connect when ran in the background

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v3:
- Patch 2: remove fully estab error
- Patch 5: force bind error + always validate join tx
- New patches: 1, 3, 4, 6, 7, 8.
- Link to v2: https://lore.kernel.org/r/20240729-mptcp-join-tx-mib-v2-0-8ace70fd407a@kernel.org

Changes in v2:
- Patch 1/2: Add "ERR" suffix in variable names. (Geliang)
- Link to v1: https://lore.kernel.org/r/20240726-mptcp-join-tx-mib-v1-0-7f2149ba0dcf@kernel.org

---
Matthieu Baerts (NGI0) (8):
      selftests: mptcp: join: no extra msg if no counter
      mptcp: MIB counters for sent MP_JOIN
      selftests: mptcp: join: reduce join_nr params
      selftests: mptcp: join: one line for join check
      selftests: mptcp: join: validate MPJ SYN TX MIB counters
      selftests: mptcp: join: more explicit check name
      selftests: mptcp: join: specify host being checked
      selftests: mptcp: join: mute errors when ran in the background

 net/mptcp/mib.c                                 |   4 +
 net/mptcp/mib.h                                 |   4 +
 net/mptcp/subflow.c                             |  21 +-
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 308 +++++++++++++++---------
 4 files changed, 226 insertions(+), 111 deletions(-)
---
base-commit: d8ff9c0319613035c0fbc35dc2ce12f8f2948dcc
change-id: 20240724-mptcp-join-tx-mib-84e21ea4b236

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Matthieu Baerts (NGI0)]

The checksum and fail counters might not be available. Then no need to
display an extra message with missing info.

While at it, fix the indentation around, which is wrong since the same
commit.

Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB counter not supported")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 01c1e0871aca..a1f80dac59a7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1112,7 +1112,7 @@ chk_csum_nr()
 
 	print_check "sum"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
-	if [ "$count" != "$csum_ns1" ]; then
+	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
 		extra_msg+=" ns1=$count"
 	fi
 	if [ -z "$count" ]; then
@@ -1125,7 +1125,7 @@ chk_csum_nr()
 	fi
 	print_check "csum"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
-	if [ "$count" != "$csum_ns2" ]; then
+	if [ -n "$count" ] && [ "$count" != "$csum_ns2" ]; then
 		extra_msg+=" ns2=$count"
 	fi
 	if [ -z "$count" ]; then
@@ -1169,13 +1169,13 @@ chk_fail_nr()
 
 	print_check "ftx"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
-	if [ "$count" != "$fail_tx" ]; then
+	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
 		extra_msg+=",tx=$count"
 	fi
 	if [ -z "$count" ]; then
 		print_skip
 	elif { [ "$count" != "$fail_tx" ] && [ $allow_tx_lost -eq 0 ]; } ||
-	   { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1 ]; }; then
+	     { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1 ]; }; then
 		fail_test "got $count MP_FAIL[s] TX expected $fail_tx"
 	else
 		print_ok
@@ -1183,13 +1183,13 @@ chk_fail_nr()
 
 	print_check "failrx"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
-	if [ "$count" != "$fail_rx" ]; then
+	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
 		extra_msg+=",rx=$count"
 	fi
 	if [ -z "$count" ]; then
 		print_skip
 	elif { [ "$count" != "$fail_rx" ] && [ $allow_rx_lost -eq 0 ]; } ||
-	   { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1 ]; }; then
+	     { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1 ]; }; then
 		fail_test "got $count MP_FAIL[s] RX expected $fail_rx"
 	else
 		print_ok

-- 
2.45.2

[PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN

[Matthieu Baerts (NGI0)]

Recently, a few issues have been discovered around the creation of
additional subflows. Without these counters, it was difficult to point
out the reason why some subflows were not created as expected.

These counters should have been added earlier, because there is no other
simple ways to extract such information from the kernel, and understand
why subflows have not been created.

While at it, some pr_debug() have been added, just in case the errno
needs to be printed.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/509
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
  - v2:
    - Add "ERR" suffix in variable names. (Geliang)
  - v3:
    - removed Fully Established Error counter: should only happen with
      the userspace PM, which will propagate the error in this case
      (ENOTCONN). (Geliang)
---
 net/mptcp/mib.c     |  4 ++++
 net/mptcp/mib.h     |  4 ++++
 net/mptcp/subflow.c | 21 ++++++++++++++++++---
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 7884217f33eb..ec0d461cb921 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -25,6 +25,10 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MPJoinSynAckHMacFailure", MPTCP_MIB_JOINSYNACKMAC),
 	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
 	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
+	SNMP_MIB_ITEM("MPJoinSynTx", MPTCP_MIB_JOINSYNTX),
+	SNMP_MIB_ITEM("MPJoinSynTxCreatSkErr", MPTCP_MIB_JOINSYNTXCREATSKERR),
+	SNMP_MIB_ITEM("MPJoinSynTxBindErr", MPTCP_MIB_JOINSYNTXBINDERR),
+	SNMP_MIB_ITEM("MPJoinSynTxConnectErr", MPTCP_MIB_JOINSYNTXCONNECTERR),
 	SNMP_MIB_ITEM("DSSNotMatching", MPTCP_MIB_DSSNOMATCH),
 	SNMP_MIB_ITEM("InfiniteMapTx", MPTCP_MIB_INFINITEMAPTX),
 	SNMP_MIB_ITEM("InfiniteMapRx", MPTCP_MIB_INFINITEMAPRX),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 66aa67f49d03..d68136f93dac 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -20,6 +20,10 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_JOINSYNACKMAC,	/* HMAC was wrong on SYN/ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK + MP_JOIN */
+	MPTCP_MIB_JOINSYNTX,		/* Sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXCREATSKERR,	/* Not able to create a socket when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXBINDERR,	/* Not able to bind() the address when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXCONNECTERR,	/* Not able to connect() when sending a SYN + MP_JOIN */
 	MPTCP_MIB_DSSNOMATCH,		/* Received a new mapping that did not match the previous one */
 	MPTCP_MIB_INFINITEMAPTX,	/* Sent an infinite mapping */
 	MPTCP_MIB_INFINITEMAPRX,	/* Received an infinite mapping */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a7fb4d46e024..fdeb7df8b095 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1575,12 +1575,17 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 	u32 remote_token;
 	int addrlen;
 
+	/* The userspace PM sent the request too early? */
 	if (!mptcp_is_fully_established(sk))
 		goto err_out;
 
 	err = mptcp_subflow_create_socket(sk, local->addr.family, &sf);
-	if (err)
+	if (err) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXCREATSKERR);
+		pr_debug("msk=%p local=%d remote:%d create sock error: %d\n",
+			 msk, local_id, remote_id, err);
 		goto err_out;
+	}
 
 	ssk = sf->sk;
 	subflow = mptcp_subflow_ctx(ssk);
@@ -1615,8 +1620,12 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 #endif
 	ssk->sk_bound_dev_if = local->ifindex;
 	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
-	if (err)
+	if (err) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXBINDERR);
+		pr_debug("msk=%p local=%d remote:%d bind error: %d\n",
+			 msk, local_id, remote_id, err);
 		goto failed;
+	}
 
 	mptcp_crypto_key_sha(subflow->remote_key, &remote_token, NULL);
 	pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d", msk,
@@ -1631,8 +1640,14 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 	sock_hold(ssk);
 	list_add_tail(&subflow->node, &msk->conn_list);
 	err = kernel_connect(sf, (struct sockaddr *)&addr, addrlen, O_NONBLOCK);
-	if (err && err != -EINPROGRESS)
+	if (err && err != -EINPROGRESS) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXCONNECTERR);
+		pr_debug("msk=%p local=%d remote:%d connect error: %d\n",
+			 msk, local_id, remote_id, err);
 		goto failed_unlink;
+	}
+
+	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);
 
 	/* discard the subflow socket */
 	mptcp_sock_graft(ssk, sk->sk_socket);

-- 
2.45.2

[PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params

[Matthieu Baerts (NGI0)]

chk_join_nr() currently takes 9 positional parameters, 6 of them are
optional. It makes it hard to read:

  chk_join_nr 1 1 1 1 0 1 1 0 4

Naming these vars helps to make it easier to read:

  join_csum_ns1=1 join_csum_ns2=0 \
    join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
    join_corrupted_pkts=4 \
    chk_join_nr 1 1 1

It will then be easier to add new optional parameters.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 31 ++++++++++++++++++-------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index a1f80dac59a7..0401ba1aaf1b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -61,6 +61,12 @@ unset sflags
 unset fastclose
 unset fullmesh
 unset speed
+unset join_csum_ns1
+unset join_csum_ns2
+unset join_fail_nr
+unset join_rst_nr
+unset join_infi_nr
+unset join_corrupted_pkts
 
 # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
 #				  (ip6 && (ip6[74] & 0xf0) == 0x30)'"
@@ -1314,12 +1320,12 @@ chk_join_nr()
 	local syn_nr=$1
 	local syn_ack_nr=$2
 	local ack_nr=$3
-	local csum_ns1=${4:-0}
-	local csum_ns2=${5:-0}
-	local fail_nr=${6:-0}
-	local rst_nr=${7:-0}
-	local infi_nr=${8:-0}
-	local corrupted_pkts=${9:-0}
+	local csum_ns1=${join_csum_ns1:-0}
+	local csum_ns2=${join_csum_ns2:-0}
+	local fail_nr=${join_fail_nr:-0}
+	local rst_nr=${join_rst_nr:-0}
+	local infi_nr=${join_infi_nr:-0}
+	local corrupted_pkts=${join_corrupted_pkts:-0}
 	local count
 	local with_cookie
 
@@ -3138,7 +3144,8 @@ fastclose_tests()
 		MPTCP_LIB_SUBTEST_FLAKY=1
 		test_linkfail=1024 fastclose=server \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 0 0 0 0 0 0 1
+		join_rst_nr=1 \
+			chk_join_nr 0 0 0
 		chk_fclose_nr 1 1 invert
 		chk_rst_nr 1 1
 	fi
@@ -3157,7 +3164,10 @@ fail_tests()
 		MPTCP_LIB_SUBTEST_FLAKY=1
 		test_linkfail=128 \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 0 0 0 +1 +0 1 0 1 "$(pedit_action_pkts)"
+		join_csum_ns1=+1 join_csum_ns2=+0 \
+			join_fail_nr=1 join_rst_nr=0 join_infi_nr=1 \
+			join_corrupted_pkts="$(pedit_action_pkts)" \
+			chk_join_nr 0 0 0
 		chk_fail_nr 1 -1 invert
 	fi
 
@@ -3170,7 +3180,10 @@ fail_tests()
 		pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
 		test_linkfail=1024 \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1 1 0 1 1 0 "$(pedit_action_pkts)"
+		join_csum_ns1=1 join_csum_ns2=0 \
+			join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
+			join_corrupted_pkts="$(pedit_action_pkts)" \
+			chk_join_nr 1 1 1
 	fi
 }
 

-- 
2.45.2

[PATCH mptcp-next v3 4/8] selftests: mptcp: join: one line for join check

[Matthieu Baerts (NGI0)]

Most tests are checking if the expected number of SYN/SYN+ACK/ACK JOINs
have been received, each of them on one line.

More Join related tests are going to be checked soon, no need to add 5
new lines per test in case of success, just one is enough. In case of
issue, the errors will still be reported like before.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 45 ++++++++++++++++---------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 0401ba1aaf1b..b10bcb1ac970 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -202,6 +202,22 @@ print_skip()
 	mptcp_lib_pr_skip "${@}"
 }
 
+# $1: check name; $2: rc
+print_results()
+{
+	local check="${1}"
+	local rc=${2}
+
+	print_check "${check}"
+	if [ ${rc} = ${KSFT_PASS} ]; then
+		print_ok
+	elif [ ${rc} = ${KSFT_SKIP} ]; then
+		print_skip
+	else
+		fail_test
+	fi
+}
+
 # [ $1: fail msg ]
 mark_as_skipped()
 {
@@ -1326,6 +1342,7 @@ chk_join_nr()
 	local rst_nr=${join_rst_nr:-0}
 	local infi_nr=${join_infi_nr:-0}
 	local corrupted_pkts=${join_corrupted_pkts:-0}
+	local rc=${KSFT_PASS}
 	local count
 	local with_cookie
 
@@ -1333,43 +1350,41 @@ chk_join_nr()
 		print_info "${corrupted_pkts} corrupted pkts"
 	fi
 
-	print_check "syn"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinSynRx")
 	if [ -z "$count" ]; then
-		print_skip
+		rc=${KSFT_SKIP}
 	elif [ "$count" != "$syn_nr" ]; then
+		rc=${KSFT_FAIL}
+		print_check "syn"
 		fail_test "got $count JOIN[s] syn expected $syn_nr"
-	else
-		print_ok
 	fi
 
-	print_check "synack"
 	with_cookie=$(ip netns exec $ns2 sysctl -n net.ipv4.tcp_syncookies)
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynAckRx")
 	if [ -z "$count" ]; then
-		print_skip
+		rc=${KSFT_SKIP}
 	elif [ "$count" != "$syn_ack_nr" ]; then
 		# simult connections exceeding the limit with cookie enabled could go up to
 		# synack validation as the conn limit can be enforced reliably only after
 		# the subflow creation
-		if [ "$with_cookie" = 2 ] && [ "$count" -gt "$syn_ack_nr" ] && [ "$count" -le "$syn_nr" ]; then
-			print_ok
-		else
+		if [ "$with_cookie" != 2 ] || [ "$count" -le "$syn_ack_nr" ] || [ "$count" -gt "$syn_nr" ]; then
+			rc=${KSFT_FAIL}
+			print_check "synack"
 			fail_test "got $count JOIN[s] synack expected $syn_ack_nr"
 		fi
-	else
-		print_ok
 	fi
 
-	print_check "ack"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
 	if [ -z "$count" ]; then
-		print_skip
+		rc=${KSFT_SKIP}
 	elif [ "$count" != "$ack_nr" ]; then
+		rc=${KSFT_FAIL}
+		print_check "ack"
 		fail_test "got $count JOIN[s] ack expected $ack_nr"
-	else
-		print_ok
 	fi
+
+	print_results "join Rx" ${rc}
+
 	if $validate_checksum; then
 		chk_csum_nr $csum_ns1 $csum_ns2
 		chk_fail_nr $fail_nr $fail_nr

-- 
2.45.2

[PATCH mptcp-next v3 5/8] selftests: mptcp: join: validate MPJ SYN TX MIB counters

[Matthieu Baerts (NGI0)]

A few new MPJoinSynTx MIB counters have been added in a previous commit.
They are being validated here in mptcp_join.sh selftest, each time the
number of received MPJ are checked.

Most of the time, the number of sent SYN+MPJ is the same as the received
ones. But sometimes, there are more, because there are dropped, or there
are errors.

While at it, the "no MPC reuse with single endpoint" subtest has been
modified to force a bind() error.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
  - v3:
    - Force a bind error (Geliang)
    - Validate join tx with rx ones. (Geliang)
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 84 +++++++++++++++++++++----
 1 file changed, 73 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index b10bcb1ac970..23ea8acb7a36 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1331,6 +1331,54 @@ chk_infi_nr()
 	fi
 }
 
+chk_join_tx_nr()
+{
+	local syn_tx=${join_syn_tx:-${syn_nr}}
+	local create=${join_create_err:-0}
+	local bind=${join_bind_err:-0}
+	local connect=${join_connect_err:-0}
+	local rc=${KSFT_PASS}
+	local count
+
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTx")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "$syn_tx" ]; then
+		rc=${KSFT_FAIL}
+		print_check "syn tx"
+		fail_test "got $count JOIN[s] syn tx expected $syn_tx"
+	fi
+
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxCreatSkErr")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "$create" ]; then
+		rc=${KSFT_FAIL}
+		print_check "syn tx create socket error"
+		fail_test "got $count JOIN[s] syn tx create socket error expected $create"
+	fi
+
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxBindErr")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "$bind" ]; then
+		rc=${KSFT_FAIL}
+		print_check "syn tx bind error"
+		fail_test "got $count JOIN[s] syn tx bind error expected $bind"
+	fi
+
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxConnectErr")
+	if [ -z "$count" ]; then
+		rc=${KSFT_SKIP}
+	elif [ "$count" != "$connect" ]; then
+		rc=${KSFT_FAIL}
+		print_check "syn tx connect error"
+		fail_test "got $count JOIN[s] syn tx connect error expected $connect"
+	fi
+
+	print_results "join Tx" ${rc}
+}
+
 chk_join_nr()
 {
 	local syn_nr=$1
@@ -1385,6 +1433,8 @@ chk_join_nr()
 
 	print_results "join Rx" ${rc}
 
+	chk_join_tx_nr
+
 	if $validate_checksum; then
 		chk_csum_nr $csum_ns1 $csum_ns2
 		chk_fail_nr $fail_nr $fail_nr
@@ -1925,9 +1975,11 @@ subflows_error_tests()
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_set_limits $ns2 0 1
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.12.2 flags subflow
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 0 0 0
+		join_bind_err=1 \
+			chk_join_nr 0 0 0
 	fi
 
 	# multiple subflows, with subflow creation error
@@ -1939,7 +1991,8 @@ subflows_error_tests()
 		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1
+		join_syn_tx=2 \
+			chk_join_nr 1 1 1
 	fi
 
 	# multiple subflows, with subflow timeout on MPJ
@@ -1951,7 +2004,8 @@ subflows_error_tests()
 		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1
+		join_syn_tx=2 \
+			chk_join_nr 1 1 1
 	fi
 
 	# multiple subflows, check that the endpoint corresponding to
@@ -1972,7 +2026,8 @@ subflows_error_tests()
 
 		# additional subflow could be created only if the PM select
 		# the later endpoint, skipping the already used one
-		chk_join_nr 1 1 1
+		join_syn_tx=2 \
+			chk_join_nr 1 1 1
 	fi
 }
 
@@ -2058,7 +2113,8 @@ signal_address_tests()
 		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal
 		pm_nl_set_limits $ns2 3 3
 		run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1
+		join_syn_tx=3 \
+			chk_join_nr 1 1 1
 		chk_add_nr 3 3
 	fi
 
@@ -2226,7 +2282,8 @@ add_addr_timeout_tests()
 		pm_nl_set_limits $ns2 2 2
 		speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1
+		join_syn_tx=2 \
+			chk_join_nr 1 1 1
 		chk_add_nr 8 0
 	fi
 }
@@ -2326,7 +2383,8 @@ remove_tests()
 		pm_nl_set_limits $ns2 2 2
 		addr_nr_ns1=-3 speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1
+		join_syn_tx=2 join_connect_err=1 \
+			chk_join_nr 1 1 1
 		chk_add_nr 3 3
 		chk_rm_nr 3 1 invert
 		chk_rst_nr 0 0
@@ -2391,7 +2449,8 @@ remove_tests()
 		pm_nl_set_limits $ns2 3 3
 		addr_nr_ns1=-8 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 1 1 1
+		join_syn_tx=3 \
+			chk_join_nr 1 1 1
 		chk_add_nr 3 3
 		chk_rm_nr 3 1 invert
 		chk_rst_nr 0 0
@@ -3619,7 +3678,8 @@ endpoint_tests()
 
 		mptcp_lib_kill_wait $tests_pid
 
-		chk_join_nr 3 3 3
+		join_syn_tx=4 \
+			chk_join_nr 3 3 3
 		chk_rm_nr 1 1
 	fi
 
@@ -3654,7 +3714,8 @@ endpoint_tests()
 		chk_mptcp_info subflows 2 subflows 2
 		mptcp_lib_kill_wait $tests_pid
 
-		chk_join_nr 3 3 3
+		join_connect_err=1 \
+			chk_join_nr 3 3 3
 		chk_add_nr 4 4
 		chk_rm_nr 2 1 invert
 	fi
@@ -3685,7 +3746,8 @@ endpoint_tests()
 		wait_mpj $ns2
 		mptcp_lib_kill_wait $tests_pid
 
-		chk_join_nr 2 2 2
+		join_syn_tx=3 join_connect_err=1 \
+			chk_join_nr 2 2 2
 		chk_add_nr 2 2
 		chk_rm_nr 1 0 invert
 	fi

-- 
2.45.2

[PATCH mptcp-next v3 6/8] selftests: mptcp: join: more explicit check name

[Matthieu Baerts (NGI0)]

Before, the check names had to be very short. It is no longer the case
now that these checks are printed on a dedicated line.

Then, it looks better to have more explicit names.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 59 +++++++++++++------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 23ea8acb7a36..46e8e43c4a54 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -856,7 +856,7 @@ chk_cestab_nr()
 	local cestab=$2
 	local count
 
-	print_check "cestab $cestab"
+	print_check "currently established: $cestab"
 	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCurrEstab")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1132,7 +1132,7 @@ chk_csum_nr()
 		csum_ns2=${csum_ns2:1}
 	fi
 
-	print_check "sum"
+	print_check "checksum server"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
 	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
 		extra_msg+=" ns1=$count"
@@ -1145,7 +1145,8 @@ chk_csum_nr()
 	else
 		print_ok
 	fi
-	print_check "csum"
+
+	print_check "checksum client"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
 	if [ -n "$count" ] && [ "$count" != "$csum_ns2" ]; then
 		extra_msg+=" ns2=$count"
@@ -1189,7 +1190,7 @@ chk_fail_nr()
 		fail_rx=${fail_rx:1}
 	fi
 
-	print_check "ftx"
+	print_check "fail tx"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
 	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
 		extra_msg+=",tx=$count"
@@ -1203,7 +1204,7 @@ chk_fail_nr()
 		print_ok
 	fi
 
-	print_check "failrx"
+	print_check "fail rx"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
 	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
 		extra_msg+=",rx=$count"
@@ -1236,7 +1237,7 @@ chk_fclose_nr()
 		extra_msg="invert"
 	fi
 
-	print_check "ctx"
+	print_check "fast close tx"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFastcloseTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1247,7 +1248,7 @@ chk_fclose_nr()
 		print_ok
 	fi
 
-	print_check "fclzrx"
+	print_check "fast close rx"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFastcloseRx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1277,7 +1278,7 @@ chk_rst_nr()
 		extra_msg="invert"
 	fi
 
-	print_check "rtx"
+	print_check "reset tx"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPRstTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1289,7 +1290,7 @@ chk_rst_nr()
 		print_ok
 	fi
 
-	print_check "rstrx"
+	print_check "reset rx"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPRstRx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1310,7 +1311,7 @@ chk_infi_nr()
 	local infi_rx=$2
 	local count
 
-	print_check "itx"
+	print_check "infi tx"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtInfiniteMapTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1320,7 +1321,7 @@ chk_infi_nr()
 		print_ok
 	fi
 
-	print_check "infirx"
+	print_check "infi rx"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtInfiniteMapRx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1403,7 +1404,7 @@ chk_join_nr()
 		rc=${KSFT_SKIP}
 	elif [ "$count" != "$syn_nr" ]; then
 		rc=${KSFT_FAIL}
-		print_check "syn"
+		print_check "syn rx"
 		fail_test "got $count JOIN[s] syn expected $syn_nr"
 	fi
 
@@ -1417,7 +1418,7 @@ chk_join_nr()
 		# the subflow creation
 		if [ "$with_cookie" != 2 ] || [ "$count" -le "$syn_ack_nr" ] || [ "$count" -gt "$syn_nr" ]; then
 			rc=${KSFT_FAIL}
-			print_check "synack"
+			print_check "synack rx"
 			fail_test "got $count JOIN[s] synack expected $syn_ack_nr"
 		fi
 	fi
@@ -1427,7 +1428,7 @@ chk_join_nr()
 		rc=${KSFT_SKIP}
 	elif [ "$count" != "$ack_nr" ]; then
 		rc=${KSFT_FAIL}
-		print_check "ack"
+		print_check "ack rx"
 		fail_test "got $count JOIN[s] ack expected $ack_nr"
 	fi
 
@@ -1507,7 +1508,7 @@ chk_add_nr()
 
 	timeout=$(ip netns exec ${ns_tx} sysctl -n net.mptcp.add_addr_timeout)
 
-	print_check "add"
+	print_check "add addr rx"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1519,7 +1520,7 @@ chk_add_nr()
 		print_ok
 	fi
 
-	print_check "echo"
+	print_check "add addr echo rx"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtEchoAdd")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1530,7 +1531,7 @@ chk_add_nr()
 	fi
 
 	if [ $port_nr -gt 0 ]; then
-		print_check "pt"
+		print_check "add addr rx with port"
 		count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtPortAdd")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1540,7 +1541,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "syn"
+		print_check "syn rx port"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortSynRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1551,7 +1552,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "synack"
+		print_check "synack rx port"
 		count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPJoinPortSynAckRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1562,7 +1563,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "ack"
+		print_check "ack rx port"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortAckRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1573,7 +1574,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "syn"
+		print_check "syn rx port mismatch"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortSynRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1584,7 +1585,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "ack"
+		print_check "ack rx port mismatch"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortAckRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1608,7 +1609,7 @@ chk_add_tx_nr()
 
 	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
 
-	print_check "add TX"
+	print_check "add addr tx"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1620,7 +1621,7 @@ chk_add_tx_nr()
 		print_ok
 	fi
 
-	print_check "echo TX"
+	print_check "add addr echo tx"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtEchoAddTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1658,7 +1659,7 @@ chk_rm_nr()
 		extra_msg="invert"
 	fi
 
-	print_check "rm"
+	print_check "rm addr rx"
 	count=$(mptcp_lib_get_counter ${addr_ns} "MPTcpExtRmAddr")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1668,7 +1669,7 @@ chk_rm_nr()
 		print_ok
 	fi
 
-	print_check "rmsf"
+	print_check "rm subflow"
 	count=$(mptcp_lib_get_counter ${subflow_ns} "MPTcpExtRmSubflow")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1703,7 +1704,7 @@ chk_rm_tx_nr()
 {
 	local rm_addr_tx_nr=$1
 
-	print_check "rm TX"
+	print_check "rm addr tx"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtRmAddrTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1722,7 +1723,7 @@ chk_prio_nr()
 	local mpj_syn_ack=$4
 	local count
 
-	print_check "ptx"
+	print_check "mp_prio tx"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1732,7 +1733,7 @@ chk_prio_nr()
 		print_ok
 	fi
 
-	print_check "prx"
+	print_check "mp_prio rx"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioRx")
 	if [ -z "$count" ]; then
 		print_skip

-- 
2.45.2

[PATCH mptcp-next v3 7/8] selftests: mptcp: join: specify host being checked

[Matthieu Baerts (NGI0)]

Instead of displaying 'invert' when looking at some events like MP_FAIL,
MP_FASTCLOSE, MP_RESET, RM_ADDR, which is a bit vague because they are
not traditionnaly sent from one side, the host being checked is now
printed.

For the ADD_ADDR, only display the host when it is the client sending
it, which is more unusual.

Also before, the 'invert' message was printed after a few checks, but it
was not clear which ones exactly.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 89 +++++++++++++------------
 1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 46e8e43c4a54..a8c4d6c04823 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1171,6 +1171,8 @@ chk_fail_nr()
 	local count
 	local ns_tx=$ns1
 	local ns_rx=$ns2
+	local tx="server"
+	local rx="client"
 	local extra_msg=""
 	local allow_tx_lost=0
 	local allow_rx_lost=0
@@ -1178,7 +1180,8 @@ chk_fail_nr()
 	if [[ $ns_invert = "invert" ]]; then
 		ns_tx=$ns2
 		ns_rx=$ns1
-		extra_msg="invert"
+		tx="client"
+		rx="server"
 	fi
 
 	if [[ "${fail_tx}" = "-"* ]]; then
@@ -1190,10 +1193,10 @@ chk_fail_nr()
 		fail_rx=${fail_rx:1}
 	fi
 
-	print_check "fail tx"
+	print_check "fail tx ${tx}"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
 	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
-		extra_msg+=",tx=$count"
+		extra_msg+=" tx=$count"
 	fi
 	if [ -z "$count" ]; then
 		print_skip
@@ -1204,10 +1207,10 @@ chk_fail_nr()
 		print_ok
 	fi
 
-	print_check "fail rx"
+	print_check "fail rx ${rx}"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
 	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
-		extra_msg+=",rx=$count"
+		extra_msg+=" rx=$count"
 	fi
 	if [ -z "$count" ]; then
 		print_skip
@@ -1229,37 +1232,35 @@ chk_fclose_nr()
 	local count
 	local ns_tx=$ns2
 	local ns_rx=$ns1
-	local extra_msg=""
+	local tx="client"
+	local rx="server"
 
 	if [[ $ns_invert = "invert" ]]; then
 		ns_tx=$ns1
 		ns_rx=$ns2
-		extra_msg="invert"
+		tx="server"
+		rx="client"
 	fi
 
-	print_check "fast close tx"
+	print_check "fast close tx ${tx}"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFastcloseTx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$fclose_tx" ]; then
-		extra_msg+=",tx=$count"
 		fail_test "got $count MP_FASTCLOSE[s] TX expected $fclose_tx"
 	else
 		print_ok
 	fi
 
-	print_check "fast close rx"
+	print_check "fast close rx ${rx}"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFastcloseRx")
 	if [ -z "$count" ]; then
 		print_skip
 	elif [ "$count" != "$fclose_rx" ]; then
-		extra_msg+=",rx=$count"
 		fail_test "got $count MP_FASTCLOSE[s] RX expected $fclose_rx"
 	else
 		print_ok
 	fi
-
-	print_info "$extra_msg"
 }
 
 chk_rst_nr()
@@ -1270,15 +1271,17 @@ chk_rst_nr()
 	local count
 	local ns_tx=$ns1
 	local ns_rx=$ns2
-	local extra_msg=""
+	local tx="server"
+	local rx="client"
 
 	if [[ $ns_invert = "invert" ]]; then
 		ns_tx=$ns2
 		ns_rx=$ns1
-		extra_msg="invert"
+		tx="client"
+		rx="server"
 	fi
 
-	print_check "reset tx"
+	print_check "reset tx ${tx}"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPRstTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1290,7 +1293,7 @@ chk_rst_nr()
 		print_ok
 	fi
 
-	print_check "reset rx"
+	print_check "reset rx ${rx}"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPRstRx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1301,8 +1304,6 @@ chk_rst_nr()
 	else
 		print_ok
 	fi
-
-	print_info "$extra_msg"
 }
 
 chk_infi_nr()
@@ -1311,7 +1312,7 @@ chk_infi_nr()
 	local infi_rx=$2
 	local count
 
-	print_check "infi tx"
+	print_check "infi tx client"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtInfiniteMapTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1321,7 +1322,7 @@ chk_infi_nr()
 		print_ok
 	fi
 
-	print_check "infi rx"
+	print_check "infi rx server"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtInfiniteMapRx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1496,19 +1497,21 @@ chk_add_nr()
 	local mis_ack_nr=0
 	local ns_tx=$ns1
 	local ns_rx=$ns2
-	local extra_msg=""
+	local tx=""
+	local rx=""
 	local count
 	local timeout
 
 	if [[ $ns_invert = "invert" ]]; then
 		ns_tx=$ns2
 		ns_rx=$ns1
-		extra_msg="invert"
+		tx=" client"
+		rx=" server"
 	fi
 
 	timeout=$(ip netns exec ${ns_tx} sysctl -n net.mptcp.add_addr_timeout)
 
-	print_check "add addr rx"
+	print_check "add addr rx${rx}"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1520,7 +1523,7 @@ chk_add_nr()
 		print_ok
 	fi
 
-	print_check "add addr echo rx"
+	print_check "add addr echo rx${tx}"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtEchoAdd")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1531,7 +1534,7 @@ chk_add_nr()
 	fi
 
 	if [ $port_nr -gt 0 ]; then
-		print_check "add addr rx with port"
+		print_check "add addr rx with port${rx}"
 		count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtPortAdd")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1541,7 +1544,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "syn rx port"
+		print_check "syn rx port${tx}"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortSynRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1552,7 +1555,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "synack rx port"
+		print_check "synack rx port${rx}"
 		count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPJoinPortSynAckRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1563,7 +1566,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "ack rx port"
+		print_check "ack rx port${tx}"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortAckRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1574,7 +1577,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "syn rx port mismatch"
+		print_check "syn rx port mismatch${tx}"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortSynRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1585,7 +1588,7 @@ chk_add_nr()
 			print_ok
 		fi
 
-		print_check "ack rx port mismatch"
+		print_check "ack rx port mismatch${tx}"
 		count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortAckRx")
 		if [ -z "$count" ]; then
 			print_skip
@@ -1596,8 +1599,6 @@ chk_add_nr()
 			print_ok
 		fi
 	fi
-
-	print_info "$extra_msg"
 }
 
 chk_add_tx_nr()
@@ -1641,6 +1642,8 @@ chk_rm_nr()
 	local count
 	local addr_ns=$ns1
 	local subflow_ns=$ns2
+	local addr="server"
+	local subflow="client"
 	local extra_msg=""
 
 	shift 2
@@ -1650,16 +1653,14 @@ chk_rm_nr()
 		shift
 	done
 
-	if [ -z $invert ]; then
-		addr_ns=$ns1
-		subflow_ns=$ns2
-	elif [ $invert = "true" ]; then
+	if [ "$invert" = "true" ]; then
 		addr_ns=$ns2
 		subflow_ns=$ns1
-		extra_msg="invert"
+		addr="client"
+		subflow="server"
 	fi
 
-	print_check "rm addr rx"
+	print_check "rm addr rx ${addr}"
 	count=$(mptcp_lib_get_counter ${addr_ns} "MPTcpExtRmAddr")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1669,7 +1670,7 @@ chk_rm_nr()
 		print_ok
 	fi
 
-	print_check "rm subflow"
+	print_check "rm subflow ${subflow}"
 	count=$(mptcp_lib_get_counter ${subflow_ns} "MPTcpExtRmSubflow")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1683,7 +1684,7 @@ chk_rm_nr()
 		count=$((count + cnt))
 		if [ "$count" != "$rm_subflow_nr" ]; then
 			suffix="$count in [$rm_subflow_nr:$((rm_subflow_nr*2))]"
-			extra_msg+=" simult"
+			extra_msg="simult"
 		fi
 		if [ $count -ge "$rm_subflow_nr" ] && \
 		   [ "$count" -le "$((rm_subflow_nr *2 ))" ]; then
@@ -1704,7 +1705,7 @@ chk_rm_tx_nr()
 {
 	local rm_addr_tx_nr=$1
 
-	print_check "rm addr tx"
+	print_check "rm addr tx client"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtRmAddrTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1723,7 +1724,7 @@ chk_prio_nr()
 	local mpj_syn_ack=$4
 	local count
 
-	print_check "mp_prio tx"
+	print_check "mp_prio tx server"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioTx")
 	if [ -z "$count" ]; then
 		print_skip
@@ -1733,7 +1734,7 @@ chk_prio_nr()
 		print_ok
 	fi
 
-	print_check "mp_prio rx"
+	print_check "mp_prio rx client"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioRx")
 	if [ -z "$count" ]; then
 		print_skip

-- 
2.45.2

[PATCH mptcp-next v3 8/8] selftests: mptcp: join: mute errors when ran in the background

[Matthieu Baerts (NGI0)]

The test is supposed to be killed before the end, which will likely
cause "Connection reset by peer" errors. It is confusing, especially
because in case of real transfer errors, the test will not be marked as
failed. But that's OK, there are many other tests checking that.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 36 ++++++++++++-------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index a8c4d6c04823..6d9fe305c281 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3481,8 +3481,8 @@ userspace_tests()
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns1
 		pm_nl_set_limits $ns2 2 2
-		speed=5 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ speed=5 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 		wait_mpj $ns1
 		userspace_pm_add_addr $ns1 10.0.2.1 10
@@ -3516,8 +3516,8 @@ userspace_tests()
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns2
 		pm_nl_set_limits $ns1 0 1
-		speed=5 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ speed=5 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 		wait_mpj $ns2
 		userspace_pm_add_sf $ns2 10.0.3.2 20
@@ -3545,8 +3545,8 @@ userspace_tests()
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns2
 		pm_nl_set_limits $ns1 0 1
-		speed=5 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ speed=5 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 		wait_mpj $ns2
 		chk_mptcp_info subflows 0 subflows 0
@@ -3566,8 +3566,8 @@ userspace_tests()
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns2
 		pm_nl_set_limits $ns1 0 1
-		speed=5 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ speed=5 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 		wait_mpj $ns2
 		userspace_pm_add_sf $ns2 10.0.3.2 20
@@ -3590,8 +3590,8 @@ userspace_tests()
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns1
 		pm_nl_set_limits $ns2 1 1
-		speed=5 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ speed=5 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 		wait_mpj $ns1
 		userspace_pm_add_addr $ns1 10.0.2.1 10
@@ -3621,8 +3621,8 @@ endpoint_tests()
 		pm_nl_set_limits $ns1 2 2
 		pm_nl_set_limits $ns2 2 2
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
-		speed=slow \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ speed=slow \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 
 		wait_mpj $ns1
@@ -3646,8 +3646,8 @@ endpoint_tests()
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 0 2
 		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow
-		test_linkfail=4 speed=20 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ test_linkfail=4 speed=20 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 
 		wait_mpj $ns2
@@ -3693,8 +3693,8 @@ endpoint_tests()
 		pm_nl_add_endpoint $ns1 10.0.2.1 id 1 flags signal
 		# broadcast IP: no packet for this address will be received on ns1
 		pm_nl_add_endpoint $ns1 224.0.0.1 id 2 flags signal
-		test_linkfail=4 speed=20 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ test_linkfail=4 speed=20 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 
 		wait_mpj $ns2
@@ -3730,8 +3730,8 @@ endpoint_tests()
 		# broadcast IP: no packet for this address will be received on ns1
 		pm_nl_add_endpoint $ns1 224.0.0.1 id 2 flags signal
 		pm_nl_add_endpoint $ns2 10.0.3.2 id 3 flags subflow
-		test_linkfail=4 speed=20 \
-			run_tests $ns1 $ns2 10.0.1.1 &
+		{ test_linkfail=4 speed=20 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
 		local tests_pid=$!
 
 		wait_attempt_fail $ns2

-- 
2.45.2

Re: [PATCH mptcp-next v3 0/8] mptcp: MIB counters for sent MP_JOIN

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10265659749

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a07e36f8ecfa
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=877027


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Geliang Tang]

Hi Matt,

Thanks for these patches.

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> The checksum and fail counters might not be available. Then no need
> to
> display an extra message with missing info.
> 
> While at it, fix the indentation around, which is wrong since the
> same
> commit.
> 
> Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
> counter not supported")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 01c1e0871aca..a1f80dac59a7 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1112,7 +1112,7 @@ chk_csum_nr()
>  
>  	print_check "sum"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
> -	if [ "$count" != "$csum_ns1" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then

We have checked [ -z "$count" ] in mptcp_lib_get_counter() already, so
I think no need to double check it here. Can we just let
mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case, then
we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?

Regards,
-Geliang

>  		extra_msg+=" ns1=$count"
>  	fi
>  	if [ -z "$count" ]; then
> @@ -1125,7 +1125,7 @@ chk_csum_nr()
>  	fi
>  	print_check "csum"
>  	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
> -	if [ "$count" != "$csum_ns2" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$csum_ns2" ]; then
>  		extra_msg+=" ns2=$count"
>  	fi
>  	if [ -z "$count" ]; then
> @@ -1169,13 +1169,13 @@ chk_fail_nr()
>  
>  	print_check "ftx"
>  	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
> -	if [ "$count" != "$fail_tx" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
>  		extra_msg+=",tx=$count"
>  	fi
>  	if [ -z "$count" ]; then
>  		print_skip
>  	elif { [ "$count" != "$fail_tx" ] && [ $allow_tx_lost -eq 0
> ]; } ||
> -	   { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1
> ]; }; then
> +	     { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1
> ]; }; then
>  		fail_test "got $count MP_FAIL[s] TX expected
> $fail_tx"
>  	else
>  		print_ok
> @@ -1183,13 +1183,13 @@ chk_fail_nr()
>  
>  	print_check "failrx"
>  	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
> -	if [ "$count" != "$fail_rx" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
>  		extra_msg+=",rx=$count"
>  	fi
>  	if [ -z "$count" ]; then
>  		print_skip
>  	elif { [ "$count" != "$fail_rx" ] && [ $allow_rx_lost -eq 0
> ]; } ||
> -	   { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1
> ]; }; then
> +	     { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1
> ]; }; then
>  		fail_test "got $count MP_FAIL[s] RX expected
> $fail_rx"
>  	else
>  		print_ok
> 

Re: [PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN

[Geliang Tang]

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> Recently, a few issues have been discovered around the creation of
> additional subflows. Without these counters, it was difficult to
> point
> out the reason why some subflows were not created as expected.
> 
> These counters should have been added earlier, because there is no
> other
> simple ways to extract such information from the kernel, and
> understand
> why subflows have not been created.
> 
> While at it, some pr_debug() have been added, just in case the errno
> needs to be printed.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/509
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>   - v2:
>     - Add "ERR" suffix in variable names. (Geliang)
>   - v3:
>     - removed Fully Established Error counter: should only happen
> with
>       the userspace PM, which will propagate the error in this case
>       (ENOTCONN). (Geliang)
> ---
>  net/mptcp/mib.c     |  4 ++++
>  net/mptcp/mib.h     |  4 ++++
>  net/mptcp/subflow.c | 21 ++++++++++++++++++---
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index 7884217f33eb..ec0d461cb921 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -25,6 +25,10 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>  	SNMP_MIB_ITEM("MPJoinSynAckHMacFailure",
> MPTCP_MIB_JOINSYNACKMAC),
>  	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
>  	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
> +	SNMP_MIB_ITEM("MPJoinSynTx", MPTCP_MIB_JOINSYNTX),
> +	SNMP_MIB_ITEM("MPJoinSynTxCreatSkErr",
> MPTCP_MIB_JOINSYNTXCREATSKERR),
> +	SNMP_MIB_ITEM("MPJoinSynTxBindErr",
> MPTCP_MIB_JOINSYNTXBINDERR),
> +	SNMP_MIB_ITEM("MPJoinSynTxConnectErr",
> MPTCP_MIB_JOINSYNTXCONNECTERR),
>  	SNMP_MIB_ITEM("DSSNotMatching", MPTCP_MIB_DSSNOMATCH),
>  	SNMP_MIB_ITEM("InfiniteMapTx", MPTCP_MIB_INFINITEMAPTX),
>  	SNMP_MIB_ITEM("InfiniteMapRx", MPTCP_MIB_INFINITEMAPRX),
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 66aa67f49d03..d68136f93dac 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -20,6 +20,10 @@ enum linux_mptcp_mib_field {
>  	MPTCP_MIB_JOINSYNACKMAC,	/* HMAC was wrong on SYN/ACK
> + MP_JOIN */
>  	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN
> */
>  	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK +
> MP_JOIN */
> +	MPTCP_MIB_JOINSYNTX,		/* Sending a SYN + MP_JOIN
> */
> +	MPTCP_MIB_JOINSYNTXCREATSKERR,	/* Not able to create a
> socket when sending a SYN + MP_JOIN */
> +	MPTCP_MIB_JOINSYNTXBINDERR,	/* Not able to bind() the
> address when sending a SYN + MP_JOIN */
> +	MPTCP_MIB_JOINSYNTXCONNECTERR,	/* Not able to connect()
> when sending a SYN + MP_JOIN */
>  	MPTCP_MIB_DSSNOMATCH,		/* Received a new mapping
> that did not match the previous one */
>  	MPTCP_MIB_INFINITEMAPTX,	/* Sent an infinite mapping
> */
>  	MPTCP_MIB_INFINITEMAPRX,	/* Received an infinite
> mapping */
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index a7fb4d46e024..fdeb7df8b095 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1575,12 +1575,17 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_pm_local *local,
>  	u32 remote_token;
>  	int addrlen;
>  
> +	/* The userspace PM sent the request too early? */
>  	if (!mptcp_is_fully_established(sk))
>  		goto err_out;
>  
>  	err = mptcp_subflow_create_socket(sk, local->addr.family,
> &sf);
> -	if (err)
> +	if (err) {
> +		MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXCREATSKERR);
> +		pr_debug("msk=%p local=%d remote:%d create sock
> error: %d\n",

It's better to use "remote=%d" instead of "remote:%d" I guess. Same
below.

> +			 msk, local_id, remote_id, err);
>  		goto err_out;
> +	}
>  
>  	ssk = sf->sk;
>  	subflow = mptcp_subflow_ctx(ssk);
> @@ -1615,8 +1620,12 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_pm_local *local,
>  #endif
>  	ssk->sk_bound_dev_if = local->ifindex;
>  	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
> -	if (err)
> +	if (err) {
> +		MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXBINDERR);
> +		pr_debug("msk=%p local=%d remote:%d bind error:
> %d\n",
> +			 msk, local_id, remote_id, err);
>  		goto failed;
> +	}
>  
>  	mptcp_crypto_key_sha(subflow->remote_key, &remote_token,
> NULL);
>  	pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d",
> msk,
> @@ -1631,8 +1640,14 @@ int __mptcp_subflow_connect(struct sock *sk,
> const struct mptcp_pm_local *local,
>  	sock_hold(ssk);
>  	list_add_tail(&subflow->node, &msk->conn_list);
>  	err = kernel_connect(sf, (struct sockaddr *)&addr, addrlen,
> O_NONBLOCK);
> -	if (err && err != -EINPROGRESS)
> +	if (err && err != -EINPROGRESS) {
> +		MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNTXCONNECTERR);
> +		pr_debug("msk=%p local=%d remote:%d connect error:
> %d\n",
> +			 msk, local_id, remote_id, err);
>  		goto failed_unlink;
> +	}
> +
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);
>  
>  	/* discard the subflow socket */
>  	mptcp_sock_graft(ssk, sk->sk_socket);
> 

Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params

[Geliang Tang]

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> chk_join_nr() currently takes 9 positional parameters, 6 of them are
> optional. It makes it hard to read:
> 
>   chk_join_nr 1 1 1 1 0 1 1 0 4
> 
> Naming these vars helps to make it easier to read:
> 
>   join_csum_ns1=1 join_csum_ns2=0 \
>     join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
>     join_corrupted_pkts=4 \
>     chk_join_nr 1 1 1
> 
> It will then be easier to add new optional parameters.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 31
> ++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index a1f80dac59a7..0401ba1aaf1b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -61,6 +61,12 @@ unset sflags
>  unset fastclose
>  unset fullmesh
>  unset speed
> +unset join_csum_ns1
> +unset join_csum_ns2
> +unset join_fail_nr
> +unset join_rst_nr
> +unset join_infi_nr
> +unset join_corrupted_pkts
>  
>  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
>  #				  (ip6 && (ip6[74] & 0xf0) ==
> 0x30)'"
> @@ -1314,12 +1320,12 @@ chk_join_nr()
>  	local syn_nr=$1
>  	local syn_ack_nr=$2
>  	local ack_nr=$3
> -	local csum_ns1=${4:-0}
> -	local csum_ns2=${5:-0}
> -	local fail_nr=${6:-0}
> -	local rst_nr=${7:-0}
> -	local infi_nr=${8:-0}
> -	local corrupted_pkts=${9:-0}
> +	local csum_ns1=${join_csum_ns1:-0}
> +	local csum_ns2=${join_csum_ns2:-0}
> +	local fail_nr=${join_fail_nr:-0}
> +	local rst_nr=${join_rst_nr:-0}
> +	local infi_nr=${join_infi_nr:-0}
> +	local corrupted_pkts=${join_corrupted_pkts:-0}
>  	local count
>  	local with_cookie
>  
> @@ -3138,7 +3144,8 @@ fastclose_tests()
>  		MPTCP_LIB_SUBTEST_FLAKY=1
>  		test_linkfail=1024 fastclose=server \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 0 0 0 0 0 0 1
> +		join_rst_nr=1 \
> +			chk_join_nr 0 0 0
>  		chk_fclose_nr 1 1 invert
>  		chk_rst_nr 1 1
>  	fi
> @@ -3157,7 +3164,10 @@ fail_tests()
>  		MPTCP_LIB_SUBTEST_FLAKY=1
>  		test_linkfail=128 \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 0 0 0 +1 +0 1 0 1 "$(pedit_action_pkts)"
> +		join_csum_ns1=+1 join_csum_ns2=+0 \
> +			join_fail_nr=1 join_rst_nr=0 join_infi_nr=1
> \

Can we drop this "join_rst_nr=0"?

> +			join_corrupted_pkts="$(pedit_action_pkts)" \
> +			chk_join_nr 0 0 0
>  		chk_fail_nr 1 -1 invert
>  	fi
>  
> @@ -3170,7 +3180,10 @@ fail_tests()
>  		pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags
> subflow
>  		test_linkfail=1024 \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1 1 0 1 1 0 "$(pedit_action_pkts)"
> +		join_csum_ns1=1 join_csum_ns2=0 \
> +			join_fail_nr=1 join_rst_nr=1 join_infi_nr=0
> \

And this "join_infi_nr=0".

> +			join_corrupted_pkts="$(pedit_action_pkts)" \
> +			chk_join_nr 1 1 1
>  	fi
>  }
>  
> 

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Matthieu Baerts]

Hi Geliang,

Thank you for the review!

On 08/08/2024 04:38, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for these patches.
> 
> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>> The checksum and fail counters might not be available. Then no need
>> to
>> display an extra message with missing info.
>>
>> While at it, fix the indentation around, which is wrong since the
>> same
>> commit.
>>
>> Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
>> counter not supported")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 01c1e0871aca..a1f80dac59a7 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1112,7 +1112,7 @@ chk_csum_nr()
>>  
>>  	print_check "sum"
>>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
>> -	if [ "$count" != "$csum_ns1" ]; then
>> +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
> 
> We have checked [ -z "$count" ] in mptcp_lib_get_counter() already, so
> I think no need to double check it here. Can we just let
> mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case, then
> we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
> chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?

I don't think we can do that: mptcp_lib_get_counter() will return
nothing if the counter doesn't exist: if the kernel being used doesn't
support it. If it is our CI running the tests, there will be a failure
thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.

So here, we need to check if '$count' is not empty, before comparing it
with '$csum_ns1'. If it is empty, the check will be skipped (see 4 lines
below), but we don't want to print a useless "extra message" in this
case, with just "ns1=".

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

Re: [PATCH mptcp-next v3 2/8] mptcp: MIB counters for sent MP_JOIN

[Matthieu Baerts]

Hi Geliang,

On 08/08/2024 04:46, Geliang Tang wrote:
> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>> Recently, a few issues have been discovered around the creation of
>> additional subflows. Without these counters, it was difficult to
>> point
>> out the reason why some subflows were not created as expected.
>>
>> These counters should have been added earlier, because there is no
>> other
>> simple ways to extract such information from the kernel, and
>> understand
>> why subflows have not been created.
>>
>> While at it, some pr_debug() have been added, just in case the errno
>> needs to be printed.

(...)

>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index a7fb4d46e024..fdeb7df8b095 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1575,12 +1575,17 @@ int __mptcp_subflow_connect(struct sock *sk,
>> const struct mptcp_pm_local *local,
>>  	u32 remote_token;
>>  	int addrlen;
>>  
>> +	/* The userspace PM sent the request too early? */
>>  	if (!mptcp_is_fully_established(sk))
>>  		goto err_out;
>>  
>>  	err = mptcp_subflow_create_socket(sk, local->addr.family,
>> &sf);
>> -	if (err)
>> +	if (err) {
>> +		MPTCP_INC_STATS(sock_net(sk),
>> MPTCP_MIB_JOINSYNTXCREATSKERR);
>> +		pr_debug("msk=%p local=%d remote:%d create sock
>> error: %d\n",
> 
> It's better to use "remote=%d" instead of "remote:%d" I guess. Same
> below.

Indeed, I see we usually use '=', I can do the modification when
applying the patches if there is nothing bigger to fix.

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

Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params

[Matthieu Baerts]

Hi Geliang,

On 08/08/2024 05:28, Geliang Tang wrote:
> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>> chk_join_nr() currently takes 9 positional parameters, 6 of them are
>> optional. It makes it hard to read:
>>
>>   chk_join_nr 1 1 1 1 0 1 1 0 4
>>
>> Naming these vars helps to make it easier to read:
>>
>>   join_csum_ns1=1 join_csum_ns2=0 \
>>     join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
>>     join_corrupted_pkts=4 \
>>     chk_join_nr 1 1 1
>>
>> It will then be easier to add new optional parameters.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 31
>> ++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index a1f80dac59a7..0401ba1aaf1b 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -61,6 +61,12 @@ unset sflags
>>  unset fastclose
>>  unset fullmesh
>>  unset speed
>> +unset join_csum_ns1
>> +unset join_csum_ns2
>> +unset join_fail_nr
>> +unset join_rst_nr
>> +unset join_infi_nr
>> +unset join_corrupted_pkts
>>  
>>  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
>>  #				  (ip6 && (ip6[74] & 0xf0) ==
>> 0x30)'"
>> @@ -1314,12 +1320,12 @@ chk_join_nr()
>>  	local syn_nr=$1
>>  	local syn_ack_nr=$2
>>  	local ack_nr=$3
>> -	local csum_ns1=${4:-0}
>> -	local csum_ns2=${5:-0}
>> -	local fail_nr=${6:-0}
>> -	local rst_nr=${7:-0}
>> -	local infi_nr=${8:-0}
>> -	local corrupted_pkts=${9:-0}
>> +	local csum_ns1=${join_csum_ns1:-0}
>> +	local csum_ns2=${join_csum_ns2:-0}
>> +	local fail_nr=${join_fail_nr:-0}
>> +	local rst_nr=${join_rst_nr:-0}
>> +	local infi_nr=${join_infi_nr:-0}
>> +	local corrupted_pkts=${join_corrupted_pkts:-0}
>>  	local count
>>  	local with_cookie
>>  
>> @@ -3138,7 +3144,8 @@ fastclose_tests()
>>  		MPTCP_LIB_SUBTEST_FLAKY=1
>>  		test_linkfail=1024 fastclose=server \
>>  			run_tests $ns1 $ns2 10.0.1.1
>> -		chk_join_nr 0 0 0 0 0 0 1
>> +		join_rst_nr=1 \
>> +			chk_join_nr 0 0 0
>>  		chk_fclose_nr 1 1 invert
>>  		chk_rst_nr 1 1
>>  	fi
>> @@ -3157,7 +3164,10 @@ fail_tests()
>>  		MPTCP_LIB_SUBTEST_FLAKY=1
>>  		test_linkfail=128 \
>>  			run_tests $ns1 $ns2 10.0.1.1
>> -		chk_join_nr 0 0 0 +1 +0 1 0 1 "$(pedit_action_pkts)"
>> +		join_csum_ns1=+1 join_csum_ns2=+0 \
>> +			join_fail_nr=1 join_rst_nr=0 join_infi_nr=1
>> \
> 
> Can we drop this "join_rst_nr=0"?

We could, but I prefer not: I think it is better to specify all
variables here, to make it clear we expect no RST, compared to the next
test where instead we expect an infinite mapping. It is easier to
compare the expectations from the two tests.

It is different from the 'fastclose_tests()' test, where it is just an
exception there, just one test where there is a rst.

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

Re: [PATCH mptcp-next v3 6/8] selftests: mptcp: join: more explicit check name

[Geliang Tang]

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> Before, the check names had to be very short. It is no longer the
> case
> now that these checks are printed on a dedicated line.
> 
> Then, it looks better to have more explicit names.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 59 +++++++++++++--
> ----------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 23ea8acb7a36..46e8e43c4a54 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -856,7 +856,7 @@ chk_cestab_nr()
>  	local cestab=$2
>  	local count
>  
> -	print_check "cestab $cestab"
> +	print_check "currently established: $cestab"
>  	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCurrEstab")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1132,7 +1132,7 @@ chk_csum_nr()
>  		csum_ns2=${csum_ns2:1}
>  	fi
>  
> -	print_check "sum"
> +	print_check "checksum server"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
>  	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
>  		extra_msg+=" ns1=$count"
> @@ -1145,7 +1145,8 @@ chk_csum_nr()
>  	else
>  		print_ok
>  	fi
> -	print_check "csum"
> +
> +	print_check "checksum client"
>  	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
>  	if [ -n "$count" ] && [ "$count" != "$csum_ns2" ]; then
>  		extra_msg+=" ns2=$count"
> @@ -1189,7 +1190,7 @@ chk_fail_nr()
>  		fail_rx=${fail_rx:1}
>  	fi
>  
> -	print_check "ftx"
> +	print_check "fail tx"
>  	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
>  	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
>  		extra_msg+=",tx=$count"
> @@ -1203,7 +1204,7 @@ chk_fail_nr()
>  		print_ok
>  	fi
>  
> -	print_check "failrx"
> +	print_check "fail rx"
>  	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
>  	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
>  		extra_msg+=",rx=$count"
> @@ -1236,7 +1237,7 @@ chk_fclose_nr()
>  		extra_msg="invert"
>  	fi
>  
> -	print_check "ctx"
> +	print_check "fast close tx"
>  	count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMPFastcloseTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1247,7 +1248,7 @@ chk_fclose_nr()
>  		print_ok
>  	fi
>  
> -	print_check "fclzrx"
> +	print_check "fast close rx"
>  	count=$(mptcp_lib_get_counter ${ns_rx}
> "MPTcpExtMPFastcloseRx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1277,7 +1278,7 @@ chk_rst_nr()
>  		extra_msg="invert"
>  	fi
>  
> -	print_check "rtx"
> +	print_check "reset tx"
>  	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPRstTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1289,7 +1290,7 @@ chk_rst_nr()
>  		print_ok
>  	fi
>  
> -	print_check "rstrx"
> +	print_check "reset rx"
>  	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPRstRx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1310,7 +1311,7 @@ chk_infi_nr()
>  	local infi_rx=$2
>  	local count
>  
> -	print_check "itx"
> +	print_check "infi tx"
>  	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtInfiniteMapTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1320,7 +1321,7 @@ chk_infi_nr()
>  		print_ok
>  	fi
>  
> -	print_check "infirx"
> +	print_check "infi rx"
>  	count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtInfiniteMapRx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1403,7 +1404,7 @@ chk_join_nr()
>  		rc=${KSFT_SKIP}
>  	elif [ "$count" != "$syn_nr" ]; then
>  		rc=${KSFT_FAIL}
> -		print_check "syn"
> +		print_check "syn rx"
>  		fail_test "got $count JOIN[s] syn expected $syn_nr"

I think it's better to update fail_test messages too:

		fail_test "got $count JOIN[s] syn rx expected $syn_nr"

>  	fi
>  
> @@ -1417,7 +1418,7 @@ chk_join_nr()
>  		# the subflow creation
>  		if [ "$with_cookie" != 2 ] || [ "$count" -le
> "$syn_ack_nr" ] || [ "$count" -gt "$syn_nr" ]; then
>  			rc=${KSFT_FAIL}
> -			print_check "synack"
> +			print_check "synack rx"
>  			fail_test "got $count JOIN[s] synack
> expected $syn_ack_nr"

Same here:

	fail_test "got $count JOIN[s] synack rx expected $syn_ack_nr"

>  		fi
>  	fi
> @@ -1427,7 +1428,7 @@ chk_join_nr()
>  		rc=${KSFT_SKIP}
>  	elif [ "$count" != "$ack_nr" ]; then
>  		rc=${KSFT_FAIL}
> -		print_check "ack"
> +		print_check "ack rx"
>  		fail_test "got $count JOIN[s] ack expected $ack_nr"

And here:

		fail_test "got $count JOIN[s] ack rx expected $ack_nr"

>  	fi
>  
> @@ -1507,7 +1508,7 @@ chk_add_nr()
>  
>  	timeout=$(ip netns exec ${ns_tx} sysctl -n
> net.mptcp.add_addr_timeout)
>  
> -	print_check "add"
> +	print_check "add addr rx"
>  	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1519,7 +1520,7 @@ chk_add_nr()
>  		print_ok
>  	fi
>  
> -	print_check "echo"
> +	print_check "add addr echo rx"
>  	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtEchoAdd")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1530,7 +1531,7 @@ chk_add_nr()
>  	fi
>  
>  	if [ $port_nr -gt 0 ]; then
> -		print_check "pt"
> +		print_check "add addr rx with port"
>  		count=$(mptcp_lib_get_counter ${ns_rx}
> "MPTcpExtPortAdd")
>  		if [ -z "$count" ]; then
>  			print_skip
> @@ -1540,7 +1541,7 @@ chk_add_nr()
>  			print_ok
>  		fi
>  
> -		print_check "syn"
> +		print_check "syn rx port"
>  		count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMPJoinPortSynRx")
>  		if [ -z "$count" ]; then
>  			print_skip
> @@ -1551,7 +1552,7 @@ chk_add_nr()
>  			print_ok
>  		fi
>  
> -		print_check "synack"
> +		print_check "synack rx port"
>  		count=$(mptcp_lib_get_counter ${ns_rx}
> "MPTcpExtMPJoinPortSynAckRx")
>  		if [ -z "$count" ]; then
>  			print_skip
> @@ -1562,7 +1563,7 @@ chk_add_nr()
>  			print_ok
>  		fi
>  
> -		print_check "ack"
> +		print_check "ack rx port"
>  		count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMPJoinPortAckRx")
>  		if [ -z "$count" ]; then
>  			print_skip
> @@ -1573,7 +1574,7 @@ chk_add_nr()
>  			print_ok
>  		fi
>  
> -		print_check "syn"
> +		print_check "syn rx port mismatch"
>  		count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMismatchPortSynRx")
>  		if [ -z "$count" ]; then
>  			print_skip
> @@ -1584,7 +1585,7 @@ chk_add_nr()
>  			print_ok
>  		fi
>  
> -		print_check "ack"
> +		print_check "ack rx port mismatch"
>  		count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMismatchPortAckRx")
>  		if [ -z "$count" ]; then
>  			print_skip
> @@ -1608,7 +1609,7 @@ chk_add_tx_nr()
>  
>  	timeout=$(ip netns exec $ns1 sysctl -n
> net.mptcp.add_addr_timeout)
>  
> -	print_check "add TX"
> +	print_check "add addr tx"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1620,7 +1621,7 @@ chk_add_tx_nr()
>  		print_ok
>  	fi
>  
> -	print_check "echo TX"
> +	print_check "add addr echo tx"
>  	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtEchoAddTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1658,7 +1659,7 @@ chk_rm_nr()
>  		extra_msg="invert"
>  	fi
>  
> -	print_check "rm"
> +	print_check "rm addr rx"
>  	count=$(mptcp_lib_get_counter ${addr_ns} "MPTcpExtRmAddr")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1668,7 +1669,7 @@ chk_rm_nr()
>  		print_ok
>  	fi
>  
> -	print_check "rmsf"
> +	print_check "rm subflow"
>  	count=$(mptcp_lib_get_counter ${subflow_ns}
> "MPTcpExtRmSubflow")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1703,7 +1704,7 @@ chk_rm_tx_nr()
>  {
>  	local rm_addr_tx_nr=$1
>  
> -	print_check "rm TX"
> +	print_check "rm addr tx"
>  	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtRmAddrTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1722,7 +1723,7 @@ chk_prio_nr()
>  	local mpj_syn_ack=$4
>  	local count
>  
> -	print_check "ptx"
> +	print_check "mp_prio tx"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioTx")
>  	if [ -z "$count" ]; then
>  		print_skip
> @@ -1732,7 +1733,7 @@ chk_prio_nr()
>  		print_ok
>  	fi
>  
> -	print_check "prx"
> +	print_check "mp_prio rx"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPPrioRx")
>  	if [ -z "$count" ]; then
>  		print_skip
> 

Re: [PATCH mptcp-next v3 4/8] selftests: mptcp: join: one line for join check

[Geliang Tang]

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> Most tests are checking if the expected number of SYN/SYN+ACK/ACK
> JOINs
> have been received, each of them on one line.
> 
> More Join related tests are going to be checked soon, no need to add
> 5
> new lines per test in case of success, just one is enough. In case of
> issue, the errors will still be reported like before.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 45
> ++++++++++++++++---------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 0401ba1aaf1b..b10bcb1ac970 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -202,6 +202,22 @@ print_skip()
>  	mptcp_lib_pr_skip "${@}"
>  }
>  
> +# $1: check name; $2: rc
> +print_results()
> +{
> +	local check="${1}"
> +	local rc=${2}
> +
> +	print_check "${check}"
> +	if [ ${rc} = ${KSFT_PASS} ]; then
> +		print_ok
> +	elif [ ${rc} = ${KSFT_SKIP} ]; then
> +		print_skip
> +	else
> +		fail_test

To align error messages, it's better to pass an empty string to
fail_test to make sure an "EOL" is printed out:

		fail_test ""

001 no JOIN
      syn rx                 [FAIL] got 0 JOIN[s] syn rx expected 1
      synack rx              [FAIL] got 0 JOIN[s] synack rx expected 1
      ack rx                 [FAIL] got 0 JOIN[s] ack rx expected 1
      join Rx                syn tx                              [FAIL]
got 0 JOIN[s] syn tx expected 1
      join Tx                002 single subflow, limited by client
      join Rx                             [ OK ]
      join Tx                             [ OK ]

->

001 no JOIN
      syn rx                 [FAIL] got 0 JOIN[s] syn rx 
      synack rx              [FAIL] got 0 JOIN[s] synack rx expected 1
      ack rx                 [FAIL] got 0 JOIN[s] ack rx expected 1
      join Rx                [FAIL]
      syn tx                 [FAIL] got 0 JOIN[s] syn tx expected 1
      join Tx                [FAIL]
002 single subflow, limited by client

WDYT?

Regards,
-Geliang

> +	fi
> +}
> +
>  # [ $1: fail msg ]
>  mark_as_skipped()
>  {
> @@ -1326,6 +1342,7 @@ chk_join_nr()
>  	local rst_nr=${join_rst_nr:-0}
>  	local infi_nr=${join_infi_nr:-0}
>  	local corrupted_pkts=${join_corrupted_pkts:-0}
> +	local rc=${KSFT_PASS}
>  	local count
>  	local with_cookie
>  
> @@ -1333,43 +1350,41 @@ chk_join_nr()
>  		print_info "${corrupted_pkts} corrupted pkts"
>  	fi
>  
> -	print_check "syn"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinSynRx")
>  	if [ -z "$count" ]; then
> -		print_skip
> +		rc=${KSFT_SKIP}
>  	elif [ "$count" != "$syn_nr" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "syn"
>  		fail_test "got $count JOIN[s] syn expected $syn_nr"
> -	else
> -		print_ok
>  	fi
>  
> -	print_check "synack"
>  	with_cookie=$(ip netns exec $ns2 sysctl -n
> net.ipv4.tcp_syncookies)
>  	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynAckRx")
>  	if [ -z "$count" ]; then
> -		print_skip
> +		rc=${KSFT_SKIP}
>  	elif [ "$count" != "$syn_ack_nr" ]; then
>  		# simult connections exceeding the limit with cookie
> enabled could go up to
>  		# synack validation as the conn limit can be
> enforced reliably only after
>  		# the subflow creation
> -		if [ "$with_cookie" = 2 ] && [ "$count" -gt
> "$syn_ack_nr" ] && [ "$count" -le "$syn_nr" ]; then
> -			print_ok
> -		else
> +		if [ "$with_cookie" != 2 ] || [ "$count" -le
> "$syn_ack_nr" ] || [ "$count" -gt "$syn_nr" ]; then
> +			rc=${KSFT_FAIL}
> +			print_check "synack"
>  			fail_test "got $count JOIN[s] synack
> expected $syn_ack_nr"
>  		fi
> -	else
> -		print_ok
>  	fi
>  
> -	print_check "ack"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
>  	if [ -z "$count" ]; then
> -		print_skip
> +		rc=${KSFT_SKIP}
>  	elif [ "$count" != "$ack_nr" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "ack"
>  		fail_test "got $count JOIN[s] ack expected $ack_nr"
> -	else
> -		print_ok
>  	fi
> +
> +	print_results "join Rx" ${rc}
> +
>  	if $validate_checksum; then
>  		chk_csum_nr $csum_ns1 $csum_ns2
>  		chk_fail_nr $fail_nr $fail_nr
> 

Re: [PATCH mptcp-next v3 5/8] selftests: mptcp: join: validate MPJ SYN TX MIB counters

[Geliang Tang]

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> A few new MPJoinSynTx MIB counters have been added in a previous
> commit.
> They are being validated here in mptcp_join.sh selftest, each time
> the
> number of received MPJ are checked.
> 
> Most of the time, the number of sent SYN+MPJ is the same as the
> received
> ones. But sometimes, there are more, because there are dropped, or
> there
> are errors.
> 
> While at it, the "no MPC reuse with single endpoint" subtest has been
> modified to force a bind() error.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>   - v3:
>     - Force a bind error (Geliang)
>     - Validate join tx with rx ones. (Geliang)
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 84
> +++++++++++++++++++++----
>  1 file changed, 73 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index b10bcb1ac970..23ea8acb7a36 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1331,6 +1331,54 @@ chk_infi_nr()
>  	fi
>  }
>  
> +chk_join_tx_nr()
> +{
> +	local syn_tx=${join_syn_tx:-${syn_nr}}

syn_nr is only a local variable of chk_join_nr(), does this limit that
this chk_join_tx_nr() can only be called in chk_join_nr()? I think it
is necessary to break this limit. WDYT?

> +	local create=${join_create_err:-0}
> +	local bind=${join_bind_err:-0}
> +	local connect=${join_connect_err:-0}
> +	local rc=${KSFT_PASS}
> +	local count
> +
> +	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTx")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$syn_tx" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "syn tx"
> +		fail_test "got $count JOIN[s] syn tx expected
> $syn_tx"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxCreatSkErr")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$create" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "syn tx create socket error"
> +		fail_test "got $count JOIN[s] syn tx create socket
> error expected $create"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxBindErr")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$bind" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "syn tx bind error"
> +		fail_test "got $count JOIN[s] syn tx bind error
> expected $bind"
> +	fi
> +
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxConnectErr")
> +	if [ -z "$count" ]; then
> +		rc=${KSFT_SKIP}
> +	elif [ "$count" != "$connect" ]; then
> +		rc=${KSFT_FAIL}
> +		print_check "syn tx connect error"
> +		fail_test "got $count JOIN[s] syn tx connect error
> expected $connect"
> +	fi
> +
> +	print_results "join Tx" ${rc}
> +}
> +
>  chk_join_nr()
>  {
>  	local syn_nr=$1
> @@ -1385,6 +1433,8 @@ chk_join_nr()
>  
>  	print_results "join Rx" ${rc}
>  
> +	chk_join_tx_nr
> +
>  	if $validate_checksum; then
>  		chk_csum_nr $csum_ns1 $csum_ns2
>  		chk_fail_nr $fail_nr $fail_nr
> @@ -1925,9 +1975,11 @@ subflows_error_tests()
>  		pm_nl_set_limits $ns1 0 1
>  		pm_nl_set_limits $ns2 0 1
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.12.2 flags subflow
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 0 0 0
> +		join_bind_err=1 \
> +			chk_join_nr 0 0 0
>  	fi
>  
>  	# multiple subflows, with subflow creation error
> @@ -1939,7 +1991,8 @@ subflows_error_tests()
>  		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1
> +		join_syn_tx=2 \
> +			chk_join_nr 1 1 1
>  	fi
>  
>  	# multiple subflows, with subflow timeout on MPJ
> @@ -1951,7 +2004,8 @@ subflows_error_tests()
>  		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1
> +		join_syn_tx=2 \
> +			chk_join_nr 1 1 1
>  	fi
>  
>  	# multiple subflows, check that the endpoint corresponding
> to
> @@ -1972,7 +2026,8 @@ subflows_error_tests()
>  
>  		# additional subflow could be created only if the PM
> select
>  		# the later endpoint, skipping the already used one
> -		chk_join_nr 1 1 1
> +		join_syn_tx=2 \
> +			chk_join_nr 1 1 1
>  	fi
>  }
>  
> @@ -2058,7 +2113,8 @@ signal_address_tests()
>  		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal
>  		pm_nl_set_limits $ns2 3 3
>  		run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1
> +		join_syn_tx=3 \
> +			chk_join_nr 1 1 1
>  		chk_add_nr 3 3
>  	fi
>  
> @@ -2226,7 +2282,8 @@ add_addr_timeout_tests()
>  		pm_nl_set_limits $ns2 2 2
>  		speed=10 \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1
> +		join_syn_tx=2 \
> +			chk_join_nr 1 1 1
>  		chk_add_nr 8 0
>  	fi
>  }
> @@ -2326,7 +2383,8 @@ remove_tests()
>  		pm_nl_set_limits $ns2 2 2
>  		addr_nr_ns1=-3 speed=10 \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1
> +		join_syn_tx=2 join_connect_err=1 \
> +			chk_join_nr 1 1 1
>  		chk_add_nr 3 3
>  		chk_rm_nr 3 1 invert
>  		chk_rst_nr 0 0
> @@ -2391,7 +2449,8 @@ remove_tests()
>  		pm_nl_set_limits $ns2 3 3
>  		addr_nr_ns1=-8 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 1 1 1
> +		join_syn_tx=3 \
> +			chk_join_nr 1 1 1
>  		chk_add_nr 3 3
>  		chk_rm_nr 3 1 invert
>  		chk_rst_nr 0 0
> @@ -3619,7 +3678,8 @@ endpoint_tests()
>  
>  		mptcp_lib_kill_wait $tests_pid
>  
> -		chk_join_nr 3 3 3
> +		join_syn_tx=4 \
> +			chk_join_nr 3 3 3
>  		chk_rm_nr 1 1
>  	fi
>  
> @@ -3654,7 +3714,8 @@ endpoint_tests()
>  		chk_mptcp_info subflows 2 subflows 2
>  		mptcp_lib_kill_wait $tests_pid
>  
> -		chk_join_nr 3 3 3
> +		join_connect_err=1 \
> +			chk_join_nr 3 3 3
>  		chk_add_nr 4 4
>  		chk_rm_nr 2 1 invert
>  	fi
> @@ -3685,7 +3746,8 @@ endpoint_tests()
>  		wait_mpj $ns2
>  		mptcp_lib_kill_wait $tests_pid
>  
> -		chk_join_nr 2 2 2
> +		join_syn_tx=3 join_connect_err=1 \
> +			chk_join_nr 2 2 2
>  		chk_add_nr 2 2
>  		chk_rm_nr 1 0 invert
>  	fi
> 

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Geliang Tang]

On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 08/08/2024 04:38, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for these patches.
> > 
> > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > The checksum and fail counters might not be available. Then no
> > > need
> > > to
> > > display an extra message with missing info.
> > > 
> > > While at it, fix the indentation around, which is wrong since the
> > > same
> > > commit.
> > > 
> > > Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
> > > counter not supported")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-----
> > > -
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index 01c1e0871aca..a1f80dac59a7 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -1112,7 +1112,7 @@ chk_csum_nr()
> > >  
> > >  	print_check "sum"
> > >  	count=$(mptcp_lib_get_counter ${ns1}
> > > "MPTcpExtDataCsumErr")
> > > -	if [ "$count" != "$csum_ns1" ]; then
> > > +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
> > 
> > We have checked [ -z "$count" ] in mptcp_lib_get_counter() already,
> > so
> > I think no need to double check it here. Can we just let
> > mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case,
> > then
> > we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
> > chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
> 
> I don't think we can do that: mptcp_lib_get_counter() will return
> nothing if the counter doesn't exist: if the kernel being used
> doesn't
> support it. If it is our CI running the tests, there will be a
> failure
> thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
> CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.
> 
> So here, we need to check if '$count' is not empty, before comparing
> it
> with '$csum_ns1'. If it is empty, the check will be skipped (see 4
> lines
> below), but we don't want to print a useless "extra message" in this
> case, with just "ns1=".

If so, let's keep this patch as is. I'll try to fix it later.

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 3/8] selftests: mptcp: join: reduce join_nr params

[Geliang Tang]

On Thu, 2024-08-08 at 12:22 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/08/2024 05:28, Geliang Tang wrote:
> > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > chk_join_nr() currently takes 9 positional parameters, 6 of them
> > > are
> > > optional. It makes it hard to read:
> > > 
> > >   chk_join_nr 1 1 1 1 0 1 1 0 4
> > > 
> > > Naming these vars helps to make it easier to read:
> > > 
> > >   join_csum_ns1=1 join_csum_ns2=0 \
> > >     join_fail_nr=1 join_rst_nr=1 join_infi_nr=0 \
> > >     join_corrupted_pkts=4 \
> > >     chk_join_nr 1 1 1
> > > 
> > > It will then be easier to add new optional parameters.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 31
> > > ++++++++++++++++++-------
> > >  1 file changed, 22 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index a1f80dac59a7..0401ba1aaf1b 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -61,6 +61,12 @@ unset sflags
> > >  unset fastclose
> > >  unset fullmesh
> > >  unset speed
> > > +unset join_csum_ns1
> > > +unset join_csum_ns2
> > > +unset join_fail_nr
> > > +unset join_rst_nr
> > > +unset join_infi_nr
> > > +unset join_corrupted_pkts
> > >  
> > >  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) ==
> > > 0x30) ||
> > >  #				  (ip6 && (ip6[74] & 0xf0) ==
> > > 0x30)'"
> > > @@ -1314,12 +1320,12 @@ chk_join_nr()
> > >  	local syn_nr=$1
> > >  	local syn_ack_nr=$2
> > >  	local ack_nr=$3
> > > -	local csum_ns1=${4:-0}
> > > -	local csum_ns2=${5:-0}
> > > -	local fail_nr=${6:-0}
> > > -	local rst_nr=${7:-0}
> > > -	local infi_nr=${8:-0}
> > > -	local corrupted_pkts=${9:-0}
> > > +	local csum_ns1=${join_csum_ns1:-0}
> > > +	local csum_ns2=${join_csum_ns2:-0}
> > > +	local fail_nr=${join_fail_nr:-0}
> > > +	local rst_nr=${join_rst_nr:-0}
> > > +	local infi_nr=${join_infi_nr:-0}
> > > +	local corrupted_pkts=${join_corrupted_pkts:-0}
> > >  	local count
> > >  	local with_cookie
> > >  
> > > @@ -3138,7 +3144,8 @@ fastclose_tests()
> > >  		MPTCP_LIB_SUBTEST_FLAKY=1
> > >  		test_linkfail=1024 fastclose=server \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > > -		chk_join_nr 0 0 0 0 0 0 1
> > > +		join_rst_nr=1 \
> > > +			chk_join_nr 0 0 0
> > >  		chk_fclose_nr 1 1 invert
> > >  		chk_rst_nr 1 1
> > >  	fi
> > > @@ -3157,7 +3164,10 @@ fail_tests()
> > >  		MPTCP_LIB_SUBTEST_FLAKY=1
> > >  		test_linkfail=128 \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > > -		chk_join_nr 0 0 0 +1 +0 1 0 1
> > > "$(pedit_action_pkts)"
> > > +		join_csum_ns1=+1 join_csum_ns2=+0 \
> > > +			join_fail_nr=1 join_rst_nr=0
> > > join_infi_nr=1
> > > \
> > 
> > Can we drop this "join_rst_nr=0"?
> 
> We could, but I prefer not: I think it is better to specify all
> variables here, to make it clear we expect no RST, compared to the
> next
> test where instead we expect an infinite mapping. It is easier to
> compare the expectations from the two tests.
> 
> It is different from the 'fastclose_tests()' test, where it is just
> an
> exception there, just one test where there is a rst.

Sure! thanks for the explanation.

-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Matthieu Baerts]

Hi Geliang,

On 09/08/2024 04:30, Geliang Tang wrote:
> On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review!
>>
>> On 08/08/2024 04:38, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for these patches.
>>>
>>> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>>>> The checksum and fail counters might not be available. Then no
>>>> need
>>>> to
>>>> display an extra message with missing info.
>>>>
>>>> While at it, fix the indentation around, which is wrong since the
>>>> same
>>>> commit.
>>>>
>>>> Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
>>>> counter not supported")
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-----
>>>> -
>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> index 01c1e0871aca..a1f80dac59a7 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -1112,7 +1112,7 @@ chk_csum_nr()
>>>>  
>>>>  	print_check "sum"
>>>>  	count=$(mptcp_lib_get_counter ${ns1}
>>>> "MPTcpExtDataCsumErr")
>>>> -	if [ "$count" != "$csum_ns1" ]; then
>>>> +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
>>>
>>> We have checked [ -z "$count" ] in mptcp_lib_get_counter() already,
>>> so
>>> I think no need to double check it here. Can we just let
>>> mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case,
>>> then
>>> we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
>>> chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
>>
>> I don't think we can do that: mptcp_lib_get_counter() will return
>> nothing if the counter doesn't exist: if the kernel being used
>> doesn't
>> support it. If it is our CI running the tests, there will be a
>> failure
>> thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
>> CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.
>>
>> So here, we need to check if '$count' is not empty, before comparing
>> it
>> with '$csum_ns1'. If it is empty, the check will be skipped (see 4
>> lines
>> below), but we don't want to print a useless "extra message" in this
>> case, with just "ns1=".
> 
> If so, let's keep this patch as is. I'll try to fix it later.

I don't think there is anything to fix there. We need these two checks:
one for our CI to detect missing features, and the other one to skip the
test if the (old) kernel (ran by another CI) doesn't support the counter.

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

Re: [PATCH mptcp-next v3 4/8] selftests: mptcp: join: one line for join check

[Matthieu Baerts]

Hi Geliang,

On 09/08/2024 04:10, Geliang Tang wrote:
> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>> Most tests are checking if the expected number of SYN/SYN+ACK/ACK
>> JOINs
>> have been received, each of them on one line.
>>
>> More Join related tests are going to be checked soon, no need to add
>> 5
>> new lines per test in case of success, just one is enough. In case of
>> issue, the errors will still be reported like before.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 45
>> ++++++++++++++++---------
>>  1 file changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 0401ba1aaf1b..b10bcb1ac970 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -202,6 +202,22 @@ print_skip()
>>  	mptcp_lib_pr_skip "${@}"
>>  }
>>  
>> +# $1: check name; $2: rc
>> +print_results()
>> +{
>> +	local check="${1}"
>> +	local rc=${2}
>> +
>> +	print_check "${check}"
>> +	if [ ${rc} = ${KSFT_PASS} ]; then
>> +		print_ok
>> +	elif [ ${rc} = ${KSFT_SKIP} ]; then
>> +		print_skip
>> +	else
>> +		fail_test
> 
> To align error messages, it's better to pass an empty string to
> fail_test to make sure an "EOL" is printed out:
> 
> 		fail_test ""
> 
> 001 no JOIN
>       syn rx                 [FAIL] got 0 JOIN[s] syn rx expected 1
>       synack rx              [FAIL] got 0 JOIN[s] synack rx expected 1
>       ack rx                 [FAIL] got 0 JOIN[s] ack rx expected 1
>       join Rx                syn tx                              [FAIL]
> got 0 JOIN[s] syn tx expected 1
>       join Tx                002 single subflow, limited by client
>       join Rx                             [ OK ]
>       join Tx                             [ OK ]
> 
> ->
> 
> 001 no JOIN
>       syn rx                 [FAIL] got 0 JOIN[s] syn rx 
>       synack rx              [FAIL] got 0 JOIN[s] synack rx expected 1
>       ack rx                 [FAIL] got 0 JOIN[s] ack rx expected 1
>       join Rx                [FAIL]
>       syn tx                 [FAIL] got 0 JOIN[s] syn tx expected 1
>       join Tx                [FAIL]
> 002 single subflow, limited by client
> 
> WDYT?

Good point. We can also print "see above" instead of nothing.

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

Re: [PATCH mptcp-next v3 5/8] selftests: mptcp: join: validate MPJ SYN TX MIB counters

[Matthieu Baerts]

Hi Geliang,

On 09/08/2024 04:21, Geliang Tang wrote:
> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>> A few new MPJoinSynTx MIB counters have been added in a previous
>> commit.
>> They are being validated here in mptcp_join.sh selftest, each time
>> the
>> number of received MPJ are checked.
>>
>> Most of the time, the number of sent SYN+MPJ is the same as the
>> received
>> ones. But sometimes, there are more, because there are dropped, or
>> there
>> are errors.
>>
>> While at it, the "no MPC reuse with single endpoint" subtest has been
>> modified to force a bind() error.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>   - v3:
>>     - Force a bind error (Geliang)
>>     - Validate join tx with rx ones. (Geliang)
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 84
>> +++++++++++++++++++++----
>>  1 file changed, 73 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index b10bcb1ac970..23ea8acb7a36 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1331,6 +1331,54 @@ chk_infi_nr()
>>  	fi
>>  }
>>  
>> +chk_join_tx_nr()
>> +{
>> +	local syn_tx=${join_syn_tx:-${syn_nr}}
> 
> syn_nr is only a local variable of chk_join_nr(), does this limit that
> this chk_join_tx_nr() can only be called in chk_join_nr()? I think it
> is necessary to break this limit. WDYT?

Yes, I can do the modification, just in case for later.

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

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

[Geliang Tang]

On Fri, 2024-08-09 at 13:32 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/08/2024 04:30, Geliang Tang wrote:
> > On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for the review!
> > > 
> > > On 08/08/2024 04:38, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > Thanks for these patches.
> > > > 
> > > > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > The checksum and fail counters might not be available. Then
> > > > > no
> > > > > need
> > > > > to
> > > > > display an extra message with missing info.
> > > > > 
> > > > > While at it, fix the indentation around, which is wrong since
> > > > > the
> > > > > same
> > > > > commit.
> > > > > 
> > > > > Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if
> > > > > MIB
> > > > > counter not supported")
> > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > > > ---
> > > > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-
> > > > > ----
> > > > > -
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > index 01c1e0871aca..a1f80dac59a7 100755
> > > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > @@ -1112,7 +1112,7 @@ chk_csum_nr()
> > > > >  
> > > > >  	print_check "sum"
> > > > >  	count=$(mptcp_lib_get_counter ${ns1}
> > > > > "MPTcpExtDataCsumErr")
> > > > > -	if [ "$count" != "$csum_ns1" ]; then
> > > > > +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ];
> > > > > then
> > > > 
> > > > We have checked [ -z "$count" ] in mptcp_lib_get_counter()
> > > > already,
> > > > so
> > > > I think no need to double check it here. Can we just let
> > > > mptcp_lib_get_counter() return "0" in this [ -z "$count" ]
> > > > case,
> > > > then
> > > > we can drop all these [ -n "$count" ] or [ -z "$count" ] (like
> > > > in
> > > > chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
> > > 
> > > I don't think we can do that: mptcp_lib_get_counter() will return
> > > nothing if the counter doesn't exist: if the kernel being used
> > > doesn't
> > > support it. If it is our CI running the tests, there will be a
> > > failure
> > > thanks to "mptcp_lib_fail_if_expected_feature()", but only with
> > > *our*
> > > CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to
> > > 1.
> > > 
> > > So here, we need to check if '$count' is not empty, before
> > > comparing
> > > it
> > > with '$csum_ns1'. If it is empty, the check will be skipped (see
> > > 4
> > > lines
> > > below), but we don't want to print a useless "extra message" in
> > > this
> > > case, with just "ns1=".
> > 
> > If so, let's keep this patch as is. I'll try to fix it later.
> 
> I don't think there is anything to fix there. We need these two
> checks:
> one for our CI to detect missing features, and the other one to skip
> the
> test if the (old) kernel (ran by another CI) doesn't support the
> counter.

Yes, indeed, you are right.

> 
> Cheers,
> Matt