Series comparison

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.

All error paths from __mptcp_subflow_connect() are covered.

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

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (2):
      mptcp: MIB counters for sent MP_JOIN
      selftests: mptcp: join: validate MPJ SYN TX MIB counters

net/mptcp/mib.c                                 |  5 ++
 net/mptcp/mib.h                                 |  5 ++
 net/mptcp/subflow.c                             | 24 ++++++++--
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 63 +++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 4 deletions(-)
---
base-commit: a9a9b3b154d187864b73eb0e86a0ff737d79a10a
change-id: 20240724-mptcp-join-tx-mib-84e21ea4b236

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

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>
---
 net/mptcp/mib.c     |  5 +++++
 net/mptcp/mib.h     |  5 +++++
 net/mptcp/subflow.c | 24 ++++++++++++++++++++----
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index XXXXXXX..XXXXXXX 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -XXX,XX +XXX,XX @@ 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("MPJoinSynTxFEstabErr", MPTCP_MIB_JOINSYNTXFULLYESTAB),
+	SNMP_MIB_ITEM("MPJoinSynTxCreatSkErr", MPTCP_MIB_JOINSYNTXCREATESOCK),
+	SNMP_MIB_ITEM("MPJoinSynTxBindErr", MPTCP_MIB_JOINSYNTXBIND),
+	SNMP_MIB_ITEM("MPJoinSynTxConnectErr", MPTCP_MIB_JOINSYNTXCONNECT),
 	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 XXXXXXX..XXXXXXX 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -XXX,XX +XXX,XX @@ 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_JOINSYNTXFULLYESTAB,	/* Not in fully established when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXCREATESOCK,	/* Not able to create a socket when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXBIND,	/* Not able to bind() the address when sending a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNTXCONNECT,	/* 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 XXXXXXX..XXXXXXX 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -XXX,XX +XXX,XX @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	int ifindex;
 	u8 flags;
 
-	if (!mptcp_is_fully_established(sk))
+	if (!mptcp_is_fully_established(sk)) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXFULLYESTAB);
 		goto err_out;
+	}
 
 	err = mptcp_subflow_create_socket(sk, loc->family, &sf);
-	if (err)
+	if (err) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXCREATESOCK);
+		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);
@@ -XXX,XX +XXX,XX @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 #endif
 	ssk->sk_bound_dev_if = ifindex;
 	err = kernel_bind(sf, (struct sockaddr *)&addr, addrlen);
-	if (err)
+	if (err) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXBIND);
+		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,
@@ -XXX,XX +XXX,XX @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	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_JOINSYNTXCONNECT);
+		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

A few new MIB counters have been added. They are being validated here.

Most of the time, there are no errors, but sometimes, more MPJ SYN are
queued compared to the numbers that are received.

Only one test has an error, the one to connect to a broadcast IP address.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index XXXXXXX..XXXXXXX 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -XXX,XX +XXX,XX @@ chk_join_nr()
 	fi
 }
 
+chk_join_tx_nr()
+{
+	local syn_nr=$1
+	local festab=$2
+	local create=$3
+	local bind=$4
+	local connect=$5
+	local count
+
+	print_check "syn TX"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTx")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$syn_nr" ]; then
+		fail_test "got $count JOIN[s] syn TX expected $syn_nr"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Fully Estab Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxFEstabErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$festab" ]; then
+		fail_test "got $count JOIN[s] syn TX Fully Estab Error expected $festab"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Create Socket Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxCreatSkErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$create" ]; then
+		fail_test "got $count JOIN[s] syn TX Create Socket Error expected $create"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Bind Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxBindErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$bind" ]; then
+		fail_test "got $count JOIN[s] syn TX Bind Error expected $bind"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Connect Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxConnectErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$connect" ]; then
+		fail_test "got $count JOIN[s] syn TX Connect Error expected $connect"
+	else
+		print_ok
+	fi
+}
+
 # a negative value for 'stale_max' means no upper bound:
 # for bidirectional transfer, if one peer sleep for a while
 # - as these tests do - we can have a quite high number of
@@ -XXX,XX +XXX,XX @@ subflows_error_tests()
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
+		chk_join_tx_nr 0 0 0 0 0
 	fi
 
 	# multiple subflows, with subflow creation error
@@ -XXX,XX +XXX,XX @@ subflows_error_tests()
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
+		chk_join_tx_nr 2 0 0 0 0
 	fi
 
 	# multiple subflows, with subflow timeout on MPJ
@@ -XXX,XX +XXX,XX @@ remove_tests()
 		addr_nr_ns1=-3 speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
+		chk_join_tx_nr 2 0 0 0 1
 		chk_add_nr 3 3
 		chk_rm_nr 3 1 invert
 		chk_rst_nr 0 0

-- 
2.45.2

All error paths from __mptcp_subflow_connect() are covered.

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

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@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) (2):
      mptcp: MIB counters for sent MP_JOIN
      selftests: mptcp: join: validate MPJ SYN TX MIB counters

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

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)
---
 net/mptcp/mib.c     |  5 +++++
 net/mptcp/mib.h     |  5 +++++
 net/mptcp/subflow.c | 24 ++++++++++++++++++++----
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index XXXXXXX..XXXXXXX 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -XXX,XX +XXX,XX @@ 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("MPJoinSynTxFEstabErr", MPTCP_MIB_JOINSYNTXFESTABERR),
+	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 XXXXXXX..XXXXXXX 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -XXX,XX +XXX,XX @@ 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_JOINSYNTXFESTABERR,	/* Not in fully established when 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 XXXXXXX..XXXXXXX 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -XXX,XX +XXX,XX @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	int ifindex;
 	u8 flags;
 
-	if (!mptcp_is_fully_established(sk))
+	if (!mptcp_is_fully_established(sk)) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXFESTABERR);
 		goto err_out;
+	}
 
 	err = mptcp_subflow_create_socket(sk, loc->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);
@@ -XXX,XX +XXX,XX @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 #endif
 	ssk->sk_bound_dev_if = 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,
@@ -XXX,XX +XXX,XX @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	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

A few new MIB counters have been added. They are being validated here.

Most of the time, there are no errors, but sometimes, more MPJ SYN are
queued compared to the numbers that are received.

Only one test has an error, the one to connect to a broadcast IP address.

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

-- 
2.45.2