All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 mptcp-next 0/3] mptcp: add more mibs
@ 2023-04-14 14:19 Paolo Abeni
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs Paolo Abeni
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Paolo Abeni @ 2023-04-14 14:19 UTC (permalink / raw)
  To: mptcp

dumb cover letter for CI's sake.

Slould be applied only after the current tree has been crunched
by syzkaller for a little bit, as this will likely change quite
a bit the timings of some critical path, possibly hiding old races
and make more easy to reach other ones

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/378

v1 -> v2:
 - fix mpj self-tests failures

Paolo Abeni (3):
  mptcp: introduces more address related mibs
  selftests: mptcp: add explicit check for new mibs
  selftests: mptcp: centralize stats dumping

 net/mptcp/mib.c                               |   6 +
 net/mptcp/mib.h                               |  18 +++
 net/mptcp/options.c                           |   5 +-
 net/mptcp/pm.c                                |   6 +-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 112 +++++++++---------
 5 files changed, 91 insertions(+), 56 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs
  2023-04-14 14:19 [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Paolo Abeni
@ 2023-04-14 14:19 ` Paolo Abeni
  2023-04-20  3:57   ` Geliang Tang
  2023-04-21 11:32   ` Matthieu Baerts
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 2/3] selftests: mptcp: add explicit check for new mibs Paolo Abeni
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 11+ messages in thread
From: Paolo Abeni @ 2023-04-14 14:19 UTC (permalink / raw)
  To: mptcp

Currently we don't track explicitly a few events related to address
management suboption handling; this patch adds new mibs for ADD_ADDR
and RM_ADDR options tx and for missed tx events due to internal storage
exahustion.

The self-tests must be updated to properly handle different mibs with
the same/shared prefix.

Additionally removes a couple of warning tracking the loss event.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/378
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/mib.c                                |  6 ++++++
 net/mptcp/mib.h                                | 18 ++++++++++++++++++
 net/mptcp/options.c                            |  5 ++++-
 net/mptcp/pm.c                                 |  6 ++++--
 .../testing/selftests/net/mptcp/mptcp_join.sh  |  4 ++--
 5 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 0dac2863c6e1..a0990c365a2e 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -34,7 +34,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("NoDSSInWindow", MPTCP_MIB_NODSSWINDOW),
 	SNMP_MIB_ITEM("DuplicateData", MPTCP_MIB_DUPDATA),
 	SNMP_MIB_ITEM("AddAddr", MPTCP_MIB_ADDADDR),
+	SNMP_MIB_ITEM("AddAddrTx", MPTCP_MIB_ADDADDRTX),
+	SNMP_MIB_ITEM("AddAddrTxDrop", MPTCP_MIB_ADDADDRTXDROP),
 	SNMP_MIB_ITEM("EchoAdd", MPTCP_MIB_ECHOADD),
+	SNMP_MIB_ITEM("EchoAddTx", MPTCP_MIB_ECHOADDTX),
+	SNMP_MIB_ITEM("EchoAddTxDrop", MPTCP_MIB_ECHOADDTXDROP),
 	SNMP_MIB_ITEM("PortAdd", MPTCP_MIB_PORTADD),
 	SNMP_MIB_ITEM("AddAddrDrop", MPTCP_MIB_ADDADDRDROP),
 	SNMP_MIB_ITEM("MPJoinPortSynRx", MPTCP_MIB_JOINPORTSYNRX),
@@ -44,6 +48,8 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MismatchPortAckRx", MPTCP_MIB_MISMATCHPORTACKRX),
 	SNMP_MIB_ITEM("RmAddr", MPTCP_MIB_RMADDR),
 	SNMP_MIB_ITEM("RmAddrDrop", MPTCP_MIB_RMADDRDROP),
+	SNMP_MIB_ITEM("RmAddrTx", MPTCP_MIB_RMADDRTX),
+	SNMP_MIB_ITEM("RmAddrTxDrop", MPTCP_MIB_RMADDRTXDROP),
 	SNMP_MIB_ITEM("RmSubflow", MPTCP_MIB_RMSUBFLOW),
 	SNMP_MIB_ITEM("MPPrioTx", MPTCP_MIB_MPPRIOTX),
 	SNMP_MIB_ITEM("MPPrioRx", MPTCP_MIB_MPPRIORX),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 2be3596374f4..cae71d947252 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -27,7 +27,15 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_NODSSWINDOW,		/* Segments not in MPTCP windows */
 	MPTCP_MIB_DUPDATA,		/* Segments discarded due to duplicate DSS */
 	MPTCP_MIB_ADDADDR,		/* Received ADD_ADDR with echo-flag=0 */
+	MPTCP_MIB_ADDADDRTX,		/* Sent ADD_ADDR with echo-flag=0 */
+	MPTCP_MIB_ADDADDRTXDROP,	/* ADD_ADDR with echo-flag=0 not send due to
+					 * resource exhaustion
+					 */
 	MPTCP_MIB_ECHOADD,		/* Received ADD_ADDR with echo-flag=1 */
+	MPTCP_MIB_ECHOADDTX,		/* Send ADD_ADDR with echo-flag=1 */
+	MPTCP_MIB_ECHOADDTXDROP,	/* ADD_ADDR with echo-flag=1 not send due
+					 * to resource exhaustion
+					 */
 	MPTCP_MIB_PORTADD,		/* Received ADD_ADDR with a port-number */
 	MPTCP_MIB_ADDADDRDROP,		/* Dropped incoming ADD_ADDR */
 	MPTCP_MIB_JOINPORTSYNRX,	/* Received a SYN MP_JOIN with a different port-number */
@@ -37,6 +45,8 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_MISMATCHPORTACKRX,	/* Received an ACK MP_JOIN with a mismatched port-number */
 	MPTCP_MIB_RMADDR,		/* Received RM_ADDR */
 	MPTCP_MIB_RMADDRDROP,		/* Dropped incoming RM_ADDR */
+	MPTCP_MIB_RMADDRTX,		/* Sent RM_ADDR */
+	MPTCP_MIB_RMADDRTXDROP,		/* RM_ADDR not sent due to resource exhaustion */
 	MPTCP_MIB_RMSUBFLOW,		/* Remove a subflow */
 	MPTCP_MIB_MPPRIOTX,		/* Transmit a MP_PRIO */
 	MPTCP_MIB_MPPRIORX,		/* Received a MP_PRIO */
@@ -63,6 +73,14 @@ struct mptcp_mib {
 	unsigned long mibs[LINUX_MIB_MPTCP_MAX];
 };
 
+static inline void MPTCP_ADD_STATS(struct net *net,
+				   enum linux_mptcp_mib_field field,
+				   int val)
+{
+	if (likely(net->mib.mptcp_statistics))
+		SNMP_ADD_STATS(net->mib.mptcp_statistics, field, val);
+}
+
 static inline void MPTCP_INC_STATS(struct net *net,
 				   enum linux_mptcp_mib_field field)
 {
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 19a01b6566f1..8a8083207be4 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -687,9 +687,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
 	}
 	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
 	if (!echo) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDRTX);
 		opts->ahmac = add_addr_generate_hmac(msk->local_key,
 						     msk->remote_key,
 						     &opts->addr);
+	} else {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADDTX);
 	}
 	pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
 		 opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
@@ -723,7 +726,7 @@ static bool mptcp_established_options_rm_addr(struct sock *sk,
 
 	for (i = 0; i < opts->rm_list.nr; i++)
 		pr_debug("rm_list_ids[%d]=%d", i, opts->rm_list.ids[i]);
-
+	MPTCP_ADD_STATS(sock_net(sk), MPTCP_MIB_RMADDRTX, opts->rm_list.nr);
 	return true;
 }
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 4ed4d29d9c11..7539137719ef 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -26,7 +26,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 
 	if (add_addr &
 	    (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
-		pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo);
+		MPTCP_INC_STATS(sock_net((struct sock *)msk),
+				echo ? MPTCP_MIB_ECHOADDTXDROP : MPTCP_MIB_ADDADDRTXDROP);
 		return -EINVAL;
 	}
 
@@ -48,7 +49,8 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 	pr_debug("msk=%p, rm_list_nr=%d", msk, rm_list->nr);
 
 	if (rm_addr) {
-		pr_warn("addr_signal error, rm_addr=%d", rm_addr);
+		MPTCP_ADD_STATS(sock_net((struct sock *)msk),
+				MPTCP_MIB_RMADDRTXDROP, rm_list->nr);
 		return -EINVAL;
 	}
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fafd19ec7e1f..095ddd747ffc 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1486,7 +1486,7 @@ chk_add_nr()
 	fi
 
 	echo -n " - echo  "
-	count=$(ip netns exec $ns1 nstat -as | grep MPTcpExtEchoAdd | awk '{print $2}')
+	count=$(ip netns exec $ns1 nstat -as MPTcpExtEchoAdd | grep MPTcpExtEchoAdd | awk '{print $2}')
 	[ -z "$count" ] && count=0
 	if [ "$count" != "$echo_nr" ]; then
 		echo "[fail] got $count ADD_ADDR echo[s] expected $echo_nr"
@@ -1608,7 +1608,7 @@ chk_rm_nr()
 	fi
 
 	printf "%-${nr_blank}s %s" " " "rm "
-	count=$(ip netns exec $addr_ns nstat -as | grep MPTcpExtRmAddr | awk '{print $2}')
+	count=$(ip netns exec $addr_ns nstat -as MPTcpExtRmAddr | grep MPTcpExtRmAddr | awk '{print $2}')
 	[ -z "$count" ] && count=0
 	if [ "$count" != "$rm_addr_nr" ]; then
 		echo "[fail] got $count RM_ADDR[s] expected $rm_addr_nr"
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2 mptcp-next 2/3] selftests: mptcp: add explicit check for new mibs
  2023-04-14 14:19 [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Paolo Abeni
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs Paolo Abeni
@ 2023-04-14 14:19 ` Paolo Abeni
  2023-04-21 11:32   ` Matthieu Baerts
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 3/3] selftests: mptcp: centralize stats dumping Paolo Abeni
  2023-04-21 11:32 ` [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Matthieu Baerts
  3 siblings, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2023-04-14 14:19 UTC (permalink / raw)
  To: mptcp

Instead of duplicating the all existing TX check with
the TX side, add the new ones on selected test cases.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 095ddd747ffc..f6ce10130ded 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1579,6 +1579,44 @@ chk_add_nr()
 	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
+chk_add_tx_nr()
+{
+	local add_tx_nr=$1
+	local echo_tx_nr=$2
+	local dump_stats
+	local timeout
+	local count
+
+	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
+
+	printf "%-${nr_blank}s %s" " " "add TX"
+	count=$(ip netns exec $ns1 nstat -as MPTcpExtAddAddrTx | grep MPTcpExtAddAddrTx | awk '{print $2}')
+	[ -z "$count" ] && count=0
+
+	# if the test configured a short timeout tolerate greater then expected
+	# add addrs options, due to retransmissions
+	if [ "$count" != "$add_tx_nr" ] && { [ "$timeout" -gt 1 ] || [ "$count" -lt "$add_tx_nr" ]; }; then
+		echo "[fail] got $count ADD_ADDR[s] TX, expected $add_tx_nr"
+		fail_test
+		dump_stats=1
+	else
+		echo -n "[ ok ]"
+	fi
+
+	echo -n " - echo TX "
+	count=$(ip netns exec $ns2 nstat -as MPTcpExtEchoAddTx | grep MPTcpExtEchoAddTx | awk '{print $2}')
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$echo_tx_nr" ]; then
+		echo "[fail] got $count ADD_ADDR echo[s] TX, expected $echo_tx_nr"
+		fail_test
+		dump_stats=1
+	else
+		echo "[ ok ]"
+	fi
+
+	[ "${dump_stats}" = 1 ] && dump_stats
+}
+
 chk_rm_nr()
 {
 	local rm_addr_nr=$1
@@ -1654,6 +1692,26 @@ chk_rm_nr()
 	echo "$extra_msg"
 }
 
+chk_rm_tx_nr()
+{
+	local rm_addr_tx_nr=$1
+
+	printf "%-${nr_blank}s %s" " " "rm TX "
+	count=$(ip netns exec $ns2 nstat -as MPTcpExtRmAddrTx | grep MPTcpExtRmAddrTx | awk '{print $2}')
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$rm_addr_tx_nr" ]; then
+		echo "[fail] got $count RM_ADDR[s] expected $rm_addr_tx_nr"
+		fail_test
+		dump_stats=1
+	else
+		echo -n "[ ok ]"
+	fi
+
+	[ "${dump_stats}" = 1 ] && dump_stats
+
+	echo "$extra_msg"
+}
+
 chk_prio_nr()
 {
 	local mp_prio_nr_tx=$1
@@ -1933,6 +1991,7 @@ signal_address_tests()
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
 		run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
+		chk_add_tx_nr 1 1
 		chk_add_nr 1 1
 	fi
 
@@ -2114,6 +2173,7 @@ add_addr_timeout_tests()
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
 		run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
 		chk_join_nr 1 1 1
+		chk_add_tx_nr 4 4
 		chk_add_nr 4 0
 	fi
 
@@ -2159,6 +2219,7 @@ remove_tests()
 		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
 		run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
 		chk_join_nr 1 1 1
+		chk_rm_tx_nr 1
 		chk_rm_nr 1 1
 	fi
 
@@ -2257,6 +2318,7 @@ remove_tests()
 		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
 		run_tests $ns1 $ns2 10.0.1.1 0 -8 -8 slow
 		chk_join_nr 3 3 3
+		chk_rm_tx_nr 0
 		chk_rm_nr 0 3 simult
 	fi
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2 mptcp-next 3/3] selftests: mptcp: centralize stats dumping
  2023-04-14 14:19 [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Paolo Abeni
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs Paolo Abeni
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 2/3] selftests: mptcp: add explicit check for new mibs Paolo Abeni
@ 2023-04-14 14:19 ` Paolo Abeni
  2023-04-14 15:23   ` selftests: mptcp: centralize stats dumping: Tests Results MPTCP CI
  2023-04-21 11:32 ` [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Matthieu Baerts
  3 siblings, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2023-04-14 14:19 UTC (permalink / raw)
  To: mptcp

If a test case fails, the mptcp_join.sh script can dump the
netns MIBs multiple times, leading to confusing output.

Let's dump such info only once per test-case, when needed.
This additionally allow removing some code duplication.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 66 ++-----------------
 1 file changed, 5 insertions(+), 61 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f6ce10130ded..5ae2301407e2 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -30,6 +30,7 @@ evts_ns1=""
 evts_ns2=""
 evts_ns1_pid=0
 evts_ns2_pid=0
+stats_dumped=0
 
 declare -A all_tests
 declare -a only_tests_ids
@@ -83,6 +84,7 @@ init_partial()
 		fi
 	done
 
+	stats_dumped=0
 	check_invert=0
 	validate_checksum=$checksum
 	FAILING_LINKS=""
@@ -343,6 +345,9 @@ fail_test()
 {
 	ret=1
 	failed_tests[${TEST_COUNT}]="${TEST_NAME}"
+
+	[ "${stats_dumped}" = 0 ] && dump_stats
+	stats_dumped=1
 }
 
 get_failed_tests_ids()
@@ -1114,7 +1119,6 @@ chk_csum_nr()
 	local csum_ns1=${1:-0}
 	local csum_ns2=${2:-0}
 	local count
-	local dump_stats
 	local extra_msg=""
 	local allow_multi_errors_ns1=0
 	local allow_multi_errors_ns2=0
@@ -1138,7 +1142,6 @@ chk_csum_nr()
 	   { [ "$count" -lt $csum_ns1 ] && [ $allow_multi_errors_ns1 -eq 1 ]; }; then
 		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1152,11 +1155,9 @@ chk_csum_nr()
 	   { [ "$count" -lt $csum_ns2 ] && [ $allow_multi_errors_ns2 -eq 1 ]; }; then
 		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
-	[ "${dump_stats}" = 1 ] && dump_stats
 
 	echo "$extra_msg"
 }
@@ -1167,7 +1168,6 @@ chk_fail_nr()
 	local fail_rx=$2
 	local ns_invert=${3:-""}
 	local count
-	local dump_stats
 	local ns_tx=$ns1
 	local ns_rx=$ns2
 	local extra_msg=""
@@ -1199,7 +1199,6 @@ chk_fail_nr()
 	   { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1 ]; }; then
 		echo "[fail] got $count MP_FAIL[s] TX expected $fail_tx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1214,13 +1213,10 @@ chk_fail_nr()
 	   { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1 ]; }; then
 		echo "[fail] got $count MP_FAIL[s] RX expected $fail_rx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	[ "${dump_stats}" = 1 ] && dump_stats
-
 	echo "$extra_msg"
 }
 
@@ -1230,7 +1226,6 @@ chk_fclose_nr()
 	local fclose_rx=$2
 	local ns_invert=$3
 	local count
-	local dump_stats
 	local ns_tx=$ns2
 	local ns_rx=$ns1
 	local extra_msg="   "
@@ -1248,7 +1243,6 @@ chk_fclose_nr()
 	if [ "$count" != "$fclose_tx" ]; then
 		echo "[fail] got $count MP_FASTCLOSE[s] TX expected $fclose_tx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1260,13 +1254,10 @@ chk_fclose_nr()
 	if [ "$count" != "$fclose_rx" ]; then
 		echo "[fail] got $count MP_FASTCLOSE[s] RX expected $fclose_rx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	[ "${dump_stats}" = 1 ] && dump_stats
-
 	echo "$extra_msg"
 }
 
@@ -1276,7 +1267,6 @@ chk_rst_nr()
 	local rst_rx=$2
 	local ns_invert=${3:-""}
 	local count
-	local dump_stats
 	local ns_tx=$ns1
 	local ns_rx=$ns2
 	local extra_msg=""
@@ -1293,7 +1283,6 @@ chk_rst_nr()
 	if [ $count -lt $rst_tx ]; then
 		echo "[fail] got $count MP_RST[s] TX expected $rst_tx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1304,13 +1293,10 @@ chk_rst_nr()
 	if [ "$count" -lt "$rst_rx" ]; then
 		echo "[fail] got $count MP_RST[s] RX expected $rst_rx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	[ "${dump_stats}" = 1 ] && dump_stats
-
 	echo "$extra_msg"
 }
 
@@ -1319,7 +1305,6 @@ chk_infi_nr()
 	local infi_tx=$1
 	local infi_rx=$2
 	local count
-	local dump_stats
 
 	printf "%-${nr_blank}s %s" " " "itx"
 	count=$(ip netns exec $ns2 nstat -as | grep InfiniteMapTx | awk '{print $2}')
@@ -1327,7 +1312,6 @@ chk_infi_nr()
 	if [ "$count" != "$infi_tx" ]; then
 		echo "[fail] got $count infinite map[s] TX expected $infi_tx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1338,12 +1322,9 @@ chk_infi_nr()
 	if [ "$count" != "$infi_rx" ]; then
 		echo "[fail] got $count infinite map[s] RX expected $infi_rx"
 		fail_test
-		dump_stats=1
 	else
 		echo "[ ok ]"
 	fi
-
-	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
 chk_join_nr()
@@ -1358,7 +1339,6 @@ chk_join_nr()
 	local infi_nr=${8:-0}
 	local corrupted_pkts=${9:-0}
 	local count
-	local dump_stats
 	local with_cookie
 	local title="${TEST_NAME}"
 
@@ -1372,7 +1352,6 @@ chk_join_nr()
 	if [ "$count" != "$syn_nr" ]; then
 		echo "[fail] got $count JOIN[s] syn expected $syn_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1390,7 +1369,6 @@ chk_join_nr()
 		else
 			echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
 			fail_test
-			dump_stats=1
 		fi
 	else
 		echo -n "[ ok ]"
@@ -1402,11 +1380,9 @@ chk_join_nr()
 	if [ "$count" != "$ack_nr" ]; then
 		echo "[fail] got $count JOIN[s] ack expected $ack_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo "[ ok ]"
 	fi
-	[ "${dump_stats}" = 1 ] && dump_stats
 	if [ $validate_checksum -eq 1 ]; then
 		chk_csum_nr $csum_ns1 $csum_ns2
 		chk_fail_nr $fail_nr $fail_nr
@@ -1466,7 +1442,6 @@ chk_add_nr()
 	local mis_syn_nr=${7:-0}
 	local mis_ack_nr=${8:-0}
 	local count
-	local dump_stats
 	local timeout
 
 	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
@@ -1480,7 +1455,6 @@ chk_add_nr()
 	if [ "$count" != "$add_nr" ] && { [ "$timeout" -gt 1 ] || [ "$count" -lt "$add_nr" ]; }; then
 		echo "[fail] got $count ADD_ADDR[s] expected $add_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1491,7 +1465,6 @@ chk_add_nr()
 	if [ "$count" != "$echo_nr" ]; then
 		echo "[fail] got $count ADD_ADDR echo[s] expected $echo_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1503,7 +1476,6 @@ chk_add_nr()
 		if [ "$count" != "$port_nr" ]; then
 			echo "[fail] got $count ADD_ADDR[s] with a port-number expected $port_nr"
 			fail_test
-			dump_stats=1
 		else
 			echo "[ ok ]"
 		fi
@@ -1516,7 +1488,6 @@ chk_add_nr()
 			echo "[fail] got $count JOIN[s] syn with a different \
 				port-number expected $syn_nr"
 			fail_test
-			dump_stats=1
 		else
 			echo -n "[ ok ]"
 		fi
@@ -1529,7 +1500,6 @@ chk_add_nr()
 			echo "[fail] got $count JOIN[s] synack with a different \
 				port-number expected $syn_ack_nr"
 			fail_test
-			dump_stats=1
 		else
 			echo -n "[ ok ]"
 		fi
@@ -1542,7 +1512,6 @@ chk_add_nr()
 			echo "[fail] got $count JOIN[s] ack with a different \
 				port-number expected $ack_nr"
 			fail_test
-			dump_stats=1
 		else
 			echo "[ ok ]"
 		fi
@@ -1555,7 +1524,6 @@ chk_add_nr()
 			echo "[fail] got $count JOIN[s] syn with a mismatched \
 				port-number expected $mis_syn_nr"
 			fail_test
-			dump_stats=1
 		else
 			echo -n "[ ok ]"
 		fi
@@ -1568,22 +1536,18 @@ chk_add_nr()
 			echo "[fail] got $count JOIN[s] ack with a mismatched \
 				port-number expected $mis_ack_nr"
 			fail_test
-			dump_stats=1
 		else
 			echo "[ ok ]"
 		fi
 	else
 		echo ""
 	fi
-
-	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
 chk_add_tx_nr()
 {
 	local add_tx_nr=$1
 	local echo_tx_nr=$2
-	local dump_stats
 	local timeout
 	local count
 
@@ -1598,7 +1562,6 @@ chk_add_tx_nr()
 	if [ "$count" != "$add_tx_nr" ] && { [ "$timeout" -gt 1 ] || [ "$count" -lt "$add_tx_nr" ]; }; then
 		echo "[fail] got $count ADD_ADDR[s] TX, expected $add_tx_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1609,12 +1572,9 @@ chk_add_tx_nr()
 	if [ "$count" != "$echo_tx_nr" ]; then
 		echo "[fail] got $count ADD_ADDR echo[s] TX, expected $echo_tx_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo "[ ok ]"
 	fi
-
-	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
 chk_rm_nr()
@@ -1624,7 +1584,6 @@ chk_rm_nr()
 	local invert
 	local simult
 	local count
-	local dump_stats
 	local addr_ns=$ns1
 	local subflow_ns=$ns2
 	local extra_msg=""
@@ -1651,7 +1610,6 @@ chk_rm_nr()
 	if [ "$count" != "$rm_addr_nr" ]; then
 		echo "[fail] got $count RM_ADDR[s] expected $rm_addr_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1675,20 +1633,16 @@ chk_rm_nr()
 		else
 			echo "[fail] got $count RM_SUBFLOW[s] expected in range [$rm_subflow_nr:$((rm_subflow_nr*2))]"
 			fail_test
-			dump_stats=1
 		fi
 		return
 	fi
 	if [ "$count" != "$rm_subflow_nr" ]; then
 		echo "[fail] got $count RM_SUBFLOW[s] expected $rm_subflow_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	[ "${dump_stats}" = 1 ] && dump_stats
-
 	echo "$extra_msg"
 }
 
@@ -1702,13 +1656,10 @@ chk_rm_tx_nr()
 	if [ "$count" != "$rm_addr_tx_nr" ]; then
 		echo "[fail] got $count RM_ADDR[s] expected $rm_addr_tx_nr"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	[ "${dump_stats}" = 1 ] && dump_stats
-
 	echo "$extra_msg"
 }
 
@@ -1717,7 +1668,6 @@ chk_prio_nr()
 	local mp_prio_nr_tx=$1
 	local mp_prio_nr_rx=$2
 	local count
-	local dump_stats
 
 	printf "%-${nr_blank}s %s" " " "ptx"
 	count=$(ip netns exec $ns1 nstat -as | grep MPTcpExtMPPrioTx | awk '{print $2}')
@@ -1725,7 +1675,6 @@ chk_prio_nr()
 	if [ "$count" != "$mp_prio_nr_tx" ]; then
 		echo "[fail] got $count MP_PRIO[s] TX expected $mp_prio_nr_tx"
 		fail_test
-		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
@@ -1736,12 +1685,9 @@ chk_prio_nr()
 	if [ "$count" != "$mp_prio_nr_rx" ]; then
 		echo "[fail] got $count MP_PRIO[s] RX expected $mp_prio_nr_rx"
 		fail_test
-		dump_stats=1
 	else
 		echo "[ ok ]"
 	fi
-
-	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
 chk_subflow_nr()
@@ -1773,7 +1719,6 @@ chk_subflow_nr()
 		ss -N $ns1 -tOni
 		ss -N $ns1 -tOni | grep token
 		ip -n $ns1 mptcp endpoint
-		dump_stats
 	fi
 }
 
@@ -1813,7 +1758,6 @@ chk_mptcp_info()
 	if [ "$dump_stats" = 1 ]; then
 		ss -N $ns1 -inmHM
 		ss -N $ns2 -inmHM
-		dump_stats
 	fi
 }
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: selftests: mptcp: centralize stats dumping: Tests Results
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 3/3] selftests: mptcp: centralize stats dumping Paolo Abeni
@ 2023-04-14 15:23   ` MPTCP CI
  0 siblings, 0 replies; 11+ messages in thread
From: MPTCP CI @ 2023-04-14 15:23 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5301653952266240
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5301653952266240/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6427553859108864
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6427553859108864/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5020178975555584
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5020178975555584/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6146078882398208
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6146078882398208/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d17c54b50423


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-debug

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 (Tessares)

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs Paolo Abeni
@ 2023-04-20  3:57   ` Geliang Tang
  2023-04-20 10:18     ` Matthieu Baerts
  2023-04-21 11:32   ` Matthieu Baerts
  1 sibling, 1 reply; 11+ messages in thread
From: Geliang Tang @ 2023-04-20  3:57 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

Hi Paolo,

How about also renaming AddAddr to AddAddrRx, and EchoAdd to
EchoAddRx, RmAddr to RmAddrRx, RmAddrDrop to RmAddrRxDrop?

Thanks,
-Geliang

Paolo Abeni <pabeni@redhat.com> 于2023年4月14日周五 22:19写道：
>
> Currently we don't track explicitly a few events related to address
> management suboption handling; this patch adds new mibs for ADD_ADDR
> and RM_ADDR options tx and for missed tx events due to internal storage
> exahustion.
>
> The self-tests must be updated to properly handle different mibs with
> the same/shared prefix.
>
> Additionally removes a couple of warning tracking the loss event.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/378
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/mib.c                                |  6 ++++++
>  net/mptcp/mib.h                                | 18 ++++++++++++++++++
>  net/mptcp/options.c                            |  5 ++++-
>  net/mptcp/pm.c                                 |  6 ++++--
>  .../testing/selftests/net/mptcp/mptcp_join.sh  |  4 ++--
>  5 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index 0dac2863c6e1..a0990c365a2e 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -34,7 +34,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>         SNMP_MIB_ITEM("NoDSSInWindow", MPTCP_MIB_NODSSWINDOW),
>         SNMP_MIB_ITEM("DuplicateData", MPTCP_MIB_DUPDATA),
>         SNMP_MIB_ITEM("AddAddr", MPTCP_MIB_ADDADDR),
> +       SNMP_MIB_ITEM("AddAddrTx", MPTCP_MIB_ADDADDRTX),
> +       SNMP_MIB_ITEM("AddAddrTxDrop", MPTCP_MIB_ADDADDRTXDROP),
>         SNMP_MIB_ITEM("EchoAdd", MPTCP_MIB_ECHOADD),
> +       SNMP_MIB_ITEM("EchoAddTx", MPTCP_MIB_ECHOADDTX),
> +       SNMP_MIB_ITEM("EchoAddTxDrop", MPTCP_MIB_ECHOADDTXDROP),
>         SNMP_MIB_ITEM("PortAdd", MPTCP_MIB_PORTADD),
>         SNMP_MIB_ITEM("AddAddrDrop", MPTCP_MIB_ADDADDRDROP),
>         SNMP_MIB_ITEM("MPJoinPortSynRx", MPTCP_MIB_JOINPORTSYNRX),
> @@ -44,6 +48,8 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>         SNMP_MIB_ITEM("MismatchPortAckRx", MPTCP_MIB_MISMATCHPORTACKRX),
>         SNMP_MIB_ITEM("RmAddr", MPTCP_MIB_RMADDR),
>         SNMP_MIB_ITEM("RmAddrDrop", MPTCP_MIB_RMADDRDROP),
> +       SNMP_MIB_ITEM("RmAddrTx", MPTCP_MIB_RMADDRTX),
> +       SNMP_MIB_ITEM("RmAddrTxDrop", MPTCP_MIB_RMADDRTXDROP),
>         SNMP_MIB_ITEM("RmSubflow", MPTCP_MIB_RMSUBFLOW),
>         SNMP_MIB_ITEM("MPPrioTx", MPTCP_MIB_MPPRIOTX),
>         SNMP_MIB_ITEM("MPPrioRx", MPTCP_MIB_MPPRIORX),
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 2be3596374f4..cae71d947252 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -27,7 +27,15 @@ enum linux_mptcp_mib_field {
>         MPTCP_MIB_NODSSWINDOW,          /* Segments not in MPTCP windows */
>         MPTCP_MIB_DUPDATA,              /* Segments discarded due to duplicate DSS */
>         MPTCP_MIB_ADDADDR,              /* Received ADD_ADDR with echo-flag=0 */
> +       MPTCP_MIB_ADDADDRTX,            /* Sent ADD_ADDR with echo-flag=0 */
> +       MPTCP_MIB_ADDADDRTXDROP,        /* ADD_ADDR with echo-flag=0 not send due to
> +                                        * resource exhaustion
> +                                        */
>         MPTCP_MIB_ECHOADD,              /* Received ADD_ADDR with echo-flag=1 */
> +       MPTCP_MIB_ECHOADDTX,            /* Send ADD_ADDR with echo-flag=1 */
> +       MPTCP_MIB_ECHOADDTXDROP,        /* ADD_ADDR with echo-flag=1 not send due
> +                                        * to resource exhaustion
> +                                        */
>         MPTCP_MIB_PORTADD,              /* Received ADD_ADDR with a port-number */
>         MPTCP_MIB_ADDADDRDROP,          /* Dropped incoming ADD_ADDR */
>         MPTCP_MIB_JOINPORTSYNRX,        /* Received a SYN MP_JOIN with a different port-number */
> @@ -37,6 +45,8 @@ enum linux_mptcp_mib_field {
>         MPTCP_MIB_MISMATCHPORTACKRX,    /* Received an ACK MP_JOIN with a mismatched port-number */
>         MPTCP_MIB_RMADDR,               /* Received RM_ADDR */
>         MPTCP_MIB_RMADDRDROP,           /* Dropped incoming RM_ADDR */
> +       MPTCP_MIB_RMADDRTX,             /* Sent RM_ADDR */
> +       MPTCP_MIB_RMADDRTXDROP,         /* RM_ADDR not sent due to resource exhaustion */
>         MPTCP_MIB_RMSUBFLOW,            /* Remove a subflow */
>         MPTCP_MIB_MPPRIOTX,             /* Transmit a MP_PRIO */
>         MPTCP_MIB_MPPRIORX,             /* Received a MP_PRIO */
> @@ -63,6 +73,14 @@ struct mptcp_mib {
>         unsigned long mibs[LINUX_MIB_MPTCP_MAX];
>  };
>
> +static inline void MPTCP_ADD_STATS(struct net *net,
> +                                  enum linux_mptcp_mib_field field,
> +                                  int val)
> +{
> +       if (likely(net->mib.mptcp_statistics))
> +               SNMP_ADD_STATS(net->mib.mptcp_statistics, field, val);
> +}
> +
>  static inline void MPTCP_INC_STATS(struct net *net,
>                                    enum linux_mptcp_mib_field field)
>  {
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 19a01b6566f1..8a8083207be4 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -687,9 +687,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>         }
>         opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>         if (!echo) {
> +               MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDRTX);
>                 opts->ahmac = add_addr_generate_hmac(msk->local_key,
>                                                      msk->remote_key,
>                                                      &opts->addr);
> +       } else {
> +               MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADDTX);
>         }
>         pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>                  opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> @@ -723,7 +726,7 @@ static bool mptcp_established_options_rm_addr(struct sock *sk,
>
>         for (i = 0; i < opts->rm_list.nr; i++)
>                 pr_debug("rm_list_ids[%d]=%d", i, opts->rm_list.ids[i]);
> -
> +       MPTCP_ADD_STATS(sock_net(sk), MPTCP_MIB_RMADDRTX, opts->rm_list.nr);
>         return true;
>  }
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 4ed4d29d9c11..7539137719ef 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -26,7 +26,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
>         if (add_addr &
>             (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> -               pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo);
> +               MPTCP_INC_STATS(sock_net((struct sock *)msk),
> +                               echo ? MPTCP_MIB_ECHOADDTXDROP : MPTCP_MIB_ADDADDRTXDROP);
>                 return -EINVAL;
>         }
>
> @@ -48,7 +49,8 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
>         pr_debug("msk=%p, rm_list_nr=%d", msk, rm_list->nr);
>
>         if (rm_addr) {
> -               pr_warn("addr_signal error, rm_addr=%d", rm_addr);
> +               MPTCP_ADD_STATS(sock_net((struct sock *)msk),
> +                               MPTCP_MIB_RMADDRTXDROP, rm_list->nr);
>                 return -EINVAL;
>         }
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fafd19ec7e1f..095ddd747ffc 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1486,7 +1486,7 @@ chk_add_nr()
>         fi
>
>         echo -n " - echo  "
> -       count=$(ip netns exec $ns1 nstat -as | grep MPTcpExtEchoAdd | awk '{print $2}')
> +       count=$(ip netns exec $ns1 nstat -as MPTcpExtEchoAdd | grep MPTcpExtEchoAdd | awk '{print $2}')
>         [ -z "$count" ] && count=0
>         if [ "$count" != "$echo_nr" ]; then
>                 echo "[fail] got $count ADD_ADDR echo[s] expected $echo_nr"
> @@ -1608,7 +1608,7 @@ chk_rm_nr()
>         fi
>
>         printf "%-${nr_blank}s %s" " " "rm "
> -       count=$(ip netns exec $addr_ns nstat -as | grep MPTcpExtRmAddr | awk '{print $2}')
> +       count=$(ip netns exec $addr_ns nstat -as MPTcpExtRmAddr | grep MPTcpExtRmAddr | awk '{print $2}')
>         [ -z "$count" ] && count=0
>         if [ "$count" != "$rm_addr_nr" ]; then
>                 echo "[fail] got $count RM_ADDR[s] expected $rm_addr_nr"
> --
> 2.39.2
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs
  2023-04-20  3:57   ` Geliang Tang
@ 2023-04-20 10:18     ` Matthieu Baerts
  0 siblings, 0 replies; 11+ messages in thread
From: Matthieu Baerts @ 2023-04-20 10:18 UTC (permalink / raw)
  To: Geliang Tang, Paolo Abeni; +Cc: mptcp

Hi Geliang,

On 20/04/2023 05:57, Geliang Tang wrote:
> Hi Paolo,
> 
> How about also renaming AddAddr to AddAddrRx, and EchoAdd to
> EchoAddRx, RmAddr to RmAddrRx, RmAddrDrop to RmAddrRxDrop?

I agree that it would be better but I don't think we can change the name.

To me, it looks like we would break the user API: imagine apps/scripts
relying on them, they would need to be adapted depending on the kernel
version, not great.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 mptcp-next 0/3] mptcp: add more mibs
  2023-04-14 14:19 [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Paolo Abeni
                   ` (2 preceding siblings ...)
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 3/3] selftests: mptcp: centralize stats dumping Paolo Abeni
@ 2023-04-21 11:32 ` Matthieu Baerts
  2023-05-03 12:36   ` Matthieu Baerts
  3 siblings, 1 reply; 11+ messages in thread
From: Matthieu Baerts @ 2023-04-21 11:32 UTC (permalink / raw)
  To: Paolo Abeni, mptcp

Hi Paolo,

On 14/04/2023 16:19, Paolo Abeni wrote:
> dumb cover letter for CI's sake.
> 
> Slould be applied only after the current tree has been crunched
> by syzkaller for a little bit, as this will likely change quite
> a bit the timings of some critical path, possibly hiding old races
> and make more easy to reach other ones
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/378

Thank you for having looked at that!

I have a small request in patch 2/3 (and a question in 1/3 but that's a
detail) if you don't mind.

We can also apply the patches and do the suggested improvement after if
you prefer.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs Paolo Abeni
  2023-04-20  3:57   ` Geliang Tang
@ 2023-04-21 11:32   ` Matthieu Baerts
  1 sibling, 0 replies; 11+ messages in thread
From: Matthieu Baerts @ 2023-04-21 11:32 UTC (permalink / raw)
  To: Paolo Abeni, mptcp

Hi Paolo,

On 14/04/2023 16:19, Paolo Abeni wrote:
> Currently we don't track explicitly a few events related to address
> management suboption handling; this patch adds new mibs for ADD_ADDR
> and RM_ADDR options tx and for missed tx events due to internal storage
> exahustion.

(s/exahustion/exhaustion/)

> The self-tests must be updated to properly handle different mibs with
> the same/shared prefix.
> 
> Additionally removes a couple of warning tracking the loss event.

Thank you for having added these MIBs counters!

I have one question below, just in case you need to send a v3.

(...)

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 19a01b6566f1..8a8083207be4 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -687,9 +687,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>  	}
>  	opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
>  	if (!echo) {
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDRTX);
>  		opts->ahmac = add_addr_generate_hmac(msk->local_key,
>  						     msk->remote_key,
>  						     &opts->addr);
> +	} else {
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADDTX);
>  	}
>  	pr_debug("addr_id=%d, ahmac=%llu, echo=%d, port=%d",
>  		 opts->addr.id, opts->ahmac, echo, ntohs(opts->addr.port));
> @@ -723,7 +726,7 @@ static bool mptcp_established_options_rm_addr(struct sock *sk,
>  
>  	for (i = 0; i < opts->rm_list.nr; i++)
>  		pr_debug("rm_list_ids[%d]=%d", i, opts->rm_list.ids[i]);
> -

(out of curiosity, why removing this line :) )

> +	MPTCP_ADD_STATS(sock_net(sk), MPTCP_MIB_RMADDRTX, opts->rm_list.nr);
>  	return true;
>  }
>  
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 4ed4d29d9c11..7539137719ef 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -26,7 +26,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>  
>  	if (add_addr &
>  	    (echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL))) {
> -		pr_warn("addr_signal error, add_addr=%d, echo=%d", add_addr, echo);
> +		MPTCP_INC_STATS(sock_net((struct sock *)msk),
> +				echo ? MPTCP_MIB_ECHOADDTXDROP : MPTCP_MIB_ADDADDRTXDROP);
>  		return -EINVAL;
>  	}
>  
> @@ -48,7 +49,8 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
>  	pr_debug("msk=%p, rm_list_nr=%d", msk, rm_list->nr);
>  
>  	if (rm_addr) {
> -		pr_warn("addr_signal error, rm_addr=%d", rm_addr);
> +		MPTCP_ADD_STATS(sock_net((struct sock *)msk),
> +				MPTCP_MIB_RMADDRTXDROP, rm_list->nr);
>  		return -EINVAL;
>  	}
>  
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fafd19ec7e1f..095ddd747ffc 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1486,7 +1486,7 @@ chk_add_nr()
>  	fi
>  
>  	echo -n " - echo  "
> -	count=$(ip netns exec $ns1 nstat -as | grep MPTcpExtEchoAdd | awk '{print $2}')
> +	count=$(ip netns exec $ns1 nstat -as MPTcpExtEchoAdd | grep MPTcpExtEchoAdd | awk '{print $2}')

(we could also use "grep -w" but we did that kind of thing with nstat a
few times in the past, no need to change)

>  	[ -z "$count" ] && count=0
>  	if [ "$count" != "$echo_nr" ]; then
>  		echo "[fail] got $count ADD_ADDR echo[s] expected $echo_nr"
> @@ -1608,7 +1608,7 @@ chk_rm_nr()
>  	fi
>  
>  	printf "%-${nr_blank}s %s" " " "rm "
> -	count=$(ip netns exec $addr_ns nstat -as | grep MPTcpExtRmAddr | awk '{print $2}')
> +	count=$(ip netns exec $addr_ns nstat -as MPTcpExtRmAddr | grep MPTcpExtRmAddr | awk '{print $2}')
>  	[ -z "$count" ] && count=0
>  	if [ "$count" != "$rm_addr_nr" ]; then
>  		echo "[fail] got $count RM_ADDR[s] expected $rm_addr_nr"

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 mptcp-next 2/3] selftests: mptcp: add explicit check for new mibs
  2023-04-14 14:19 ` [PATCH v2 mptcp-next 2/3] selftests: mptcp: add explicit check for new mibs Paolo Abeni
@ 2023-04-21 11:32   ` Matthieu Baerts
  0 siblings, 0 replies; 11+ messages in thread
From: Matthieu Baerts @ 2023-04-21 11:32 UTC (permalink / raw)
  To: Paolo Abeni, mptcp

Hi Paolo,

On 14/04/2023 16:19, Paolo Abeni wrote:
> Instead of duplicating the all existing TX check with
> the TX side, add the new ones on selected test cases.

Good idea!

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 095ddd747ffc..f6ce10130ded 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1579,6 +1579,44 @@ chk_add_nr()
>  	[ "${dump_stats}" = 1 ] && dump_stats
>  }
>  
> +chk_add_tx_nr()
> +{
> +	local add_tx_nr=$1
> +	local echo_tx_nr=$2
> +	local dump_stats
> +	local timeout
> +	local count
> +
> +	timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
> +
> +	printf "%-${nr_blank}s %s" " " "add TX"
> +	count=$(ip netns exec $ns1 nstat -as MPTcpExtAddAddrTx | grep MPTcpExtAddAddrTx | awk '{print $2}')
> +	[ -z "$count" ] && count=0

In order to support running this on old kernels, could we check if the
counter is available first and skip the checks if not?

e.g. by using "nstat" with "-z" and return if '[ -z "${count}" ]'?

We could also exit if there is no counter if
${SELFTESTS_MPTCP_LIB_EXTRA_CHECKS} is set, see:

https://patchwork.kernel.org/project/mptcp/patch/20230406-mptcp-issue-368-selftests-old-kernels-v1-8-246ac567724d@tessares.net/

(We should add a helper to do this exit if the variable is set so we
make sure this doesn't happen when our CI does the tests, always with
the same version for the selftests and the kernel)

> +
> +	# if the test configured a short timeout tolerate greater then expected
> +	# add addrs options, due to retransmissions
> +	if [ "$count" != "$add_tx_nr" ] && { [ "$timeout" -gt 1 ] || [ "$count" -lt "$add_tx_nr" ]; }; then
> +		echo "[fail] got $count ADD_ADDR[s] TX, expected $add_tx_nr"
> +		fail_test
> +		dump_stats=1
> +	else
> +		echo -n "[ ok ]"
> +	fi
> +
> +	echo -n " - echo TX "
> +	count=$(ip netns exec $ns2 nstat -as MPTcpExtEchoAddTx | grep MPTcpExtEchoAddTx | awk '{print $2}')
> +	[ -z "$count" ] && count=0

(if you use "nstat" with "-z", you don't need this line)

> +	if [ "$count" != "$echo_tx_nr" ]; then
> +		echo "[fail] got $count ADD_ADDR echo[s] TX, expected $echo_tx_nr"
> +		fail_test
> +		dump_stats=1
> +	else
> +		echo "[ ok ]"
> +	fi
> +
> +	[ "${dump_stats}" = 1 ] && dump_stats
> +}
> +
>  chk_rm_nr()
>  {
>  	local rm_addr_nr=$1
> @@ -1654,6 +1692,26 @@ chk_rm_nr()
>  	echo "$extra_msg"
>  }
>  
> +chk_rm_tx_nr()
> +{
> +	local rm_addr_tx_nr=$1
> +
> +	printf "%-${nr_blank}s %s" " " "rm TX "
> +	count=$(ip netns exec $ns2 nstat -as MPTcpExtRmAddrTx | grep MPTcpExtRmAddrTx | awk '{print $2}')
> +	[ -z "$count" ] && count=0

Same here: skip the check if 'nstat -asz' return nothing?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 mptcp-next 0/3] mptcp: add more mibs
  2023-04-21 11:32 ` [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Matthieu Baerts
@ 2023-05-03 12:36   ` Matthieu Baerts
  0 siblings, 0 replies; 11+ messages in thread
From: Matthieu Baerts @ 2023-05-03 12:36 UTC (permalink / raw)
  To: Paolo Abeni, mptcp

Hi Paolo,

On 21/04/2023 13:32, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 14/04/2023 16:19, Paolo Abeni wrote:
>> dumb cover letter for CI's sake.
>>
>> Slould be applied only after the current tree has been crunched
>> by syzkaller for a little bit, as this will likely change quite
>> a bit the timings of some critical path, possibly hiding old races
>> and make more easy to reach other ones
>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/378
> 
> Thank you for having looked at that!
> 
> I have a small request in patch 2/3 (and a question in 1/3 but that's a
> detail) if you don't mind.
> 
> We can also apply the patches and do the suggested improvement after if
> you prefer.

As discussed at the last weekly meeting, I just applied the patches in
our tree (feat. for net-next) with my RvB (and without a typo):

New patches for t/upstream:
- 11f98962ef03: mptcp: introduces more address related mibs
- ae2301fadbf9: selftests: mptcp: add explicit check for new mibs
- 3001ad5baef7: selftests: mptcp: centralize stats dumping
- Results: 269bb6df6e7f..b2cd747b2dff (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230503T123221

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-03 12:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-14 14:19 [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Paolo Abeni
2023-04-14 14:19 ` [PATCH v2 mptcp-next 1/3] mptcp: introduces more address related mibs Paolo Abeni
2023-04-20  3:57   ` Geliang Tang
2023-04-20 10:18     ` Matthieu Baerts
2023-04-21 11:32   ` Matthieu Baerts
2023-04-14 14:19 ` [PATCH v2 mptcp-next 2/3] selftests: mptcp: add explicit check for new mibs Paolo Abeni
2023-04-21 11:32   ` Matthieu Baerts
2023-04-14 14:19 ` [PATCH v2 mptcp-next 3/3] selftests: mptcp: centralize stats dumping Paolo Abeni
2023-04-14 15:23   ` selftests: mptcp: centralize stats dumping: Tests Results MPTCP CI
2023-04-21 11:32 ` [PATCH v2 mptcp-next 0/3] mptcp: add more mibs Matthieu Baerts
2023-05-03 12:36   ` Matthieu Baerts

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.