[PATCH mptcp-next v5 0/3] add MPTCP_MIB_CURRESTAB

[PATCH mptcp-next v5 0/3] add MPTCP_MIB_CURRESTAB

[Geliang Tang]

v5:
- merge first three patches into one as Paolo suggested.
- drop "EXPORT_SYMBOL_GPL(mptcp_set_state);" as Paolo suggested.

v4:
- rebased.

v3:
- add mptcp_set_state helper

v2:
- use ftrace_regs_get_argument instead of regs_get_kernel_argument to
  fix build warnings reported kernel test robot.

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

Geliang Tang (3):
  mptcp: add mptcp_set_state
  mptcp: use mptcp_set_state
  selftests: mptcp: check CURRESTAB counters

 net/mptcp/mib.c                               |  1 +
 net/mptcp/mib.h                               |  8 +++
 net/mptcp/pm_netlink.c                        |  3 +
 net/mptcp/protocol.c                          | 56 ++++++++++++-------
 net/mptcp/protocol.h                          |  1 +
 net/mptcp/subflow.c                           |  2 +-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 45 +++++++++++++--
 7 files changed, 91 insertions(+), 25 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v5 1/3] mptcp: add mptcp_set_state

[Geliang Tang]

Similar to MPTCP_INC_STATS(), add a new helper named MPTCP_DEC_STATS()
to decrement a MIB counter.

And add a new MIB counter named MPTCP_MIB_CURRESTAB to count current
established MPTCP connections.

This patch adds a new function mptcp_set_state(), in it if switch from
or to ESTABLISH state, increment or decrement this newly added counter
MPTCP_MIB_CURRESTAB.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/460
Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/mib.c      |  1 +
 net/mptcp/mib.h      |  8 ++++++++
 net/mptcp/protocol.c | 18 ++++++++++++++++++
 net/mptcp/protocol.h |  1 +
 4 files changed, 28 insertions(+)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index a0990c365a2e..c30405e76833 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -66,6 +66,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("RcvWndShared", MPTCP_MIB_RCVWNDSHARED),
 	SNMP_MIB_ITEM("RcvWndConflictUpdate", MPTCP_MIB_RCVWNDCONFLICTUPDATE),
 	SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT),
+	SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB),
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index cae71d947252..dd7fd1f246b5 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -65,6 +65,7 @@ enum linux_mptcp_mib_field {
 					 * conflict with another subflow while updating msk rcv wnd
 					 */
 	MPTCP_MIB_RCVWNDCONFLICT,	/* Conflict with while updating msk rcv wnd */
+	MPTCP_MIB_CURRESTAB,		/* Current established MPTCP connections */
 	__MPTCP_MIB_MAX
 };
 
@@ -95,4 +96,11 @@ static inline void __MPTCP_INC_STATS(struct net *net,
 		__SNMP_INC_STATS(net->mib.mptcp_statistics, field);
 }
 
+static inline void MPTCP_DEC_STATS(struct net *net,
+				   enum linux_mptcp_mib_field field)
+{
+	if (likely(net->mib.mptcp_statistics))
+		SNMP_DEC_STATS(net->mib.mptcp_statistics, field);
+}
+
 bool mptcp_mib_alloc(struct net *net);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4fc038d29623..b0b149cc89aa 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2885,6 +2885,24 @@ void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how)
 	release_sock(ssk);
 }
 
+void mptcp_set_state(struct sock *sk, int state)
+{
+	int oldstate = sk->sk_state;
+
+	switch (state) {
+	case TCP_ESTABLISHED:
+		if (oldstate != TCP_ESTABLISHED)
+			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
+		break;
+
+	default:
+		if (oldstate == TCP_ESTABLISHED)
+			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
+	}
+
+	inet_sk_state_store(sk, state);
+}
+
 static const unsigned char new_state[16] = {
 	/* current state:     new state:      action:	*/
 	[0 /* (Invalid) */] = TCP_CLOSE,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ac11044bef1d..f7b9c1b995df 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -646,6 +646,7 @@ bool __mptcp_close(struct sock *sk, long timeout);
 void mptcp_cancel_work(struct sock *sk);
 void __mptcp_unaccepted_force_close(struct sock *sk);
 void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk);
+void mptcp_set_state(struct sock *sk, int state);
 
 bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
 			   const struct mptcp_addr_info *b, bool use_port);
-- 
2.35.3

[PATCH mptcp-next v5 2/3] mptcp: use mptcp_set_state

[Geliang Tang]

This patch replaces all the 'inet_sk_state_store()' calls under net/mptcp
with the new helper mptcp_set_state().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c |  3 +++
 net/mptcp/protocol.c   | 38 +++++++++++++++++++-------------------
 net/mptcp/subflow.c    |  2 +-
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index bf4d96f6f99a..b93683b5e618 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1048,6 +1048,9 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (err)
 		return err;
 
+	/* avoid replacing inet_sk_state_store with mptcp_set_state here, as the
+	 * old status is known to be TCP_CLOSE, hence will not affect the count.
+	 */
 	inet_sk_state_store(newsk, TCP_LISTEN);
 	lock_sock(ssk);
 	err = __inet_listen_sk(ssk, backlog);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b0b149cc89aa..d73dc76e5bfc 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -443,11 +443,11 @@ static void mptcp_check_data_fin_ack(struct sock *sk)
 
 		switch (sk->sk_state) {
 		case TCP_FIN_WAIT1:
-			inet_sk_state_store(sk, TCP_FIN_WAIT2);
+			mptcp_set_state(sk, TCP_FIN_WAIT2);
 			break;
 		case TCP_CLOSING:
 		case TCP_LAST_ACK:
-			inet_sk_state_store(sk, TCP_CLOSE);
+			mptcp_set_state(sk, TCP_CLOSE);
 			break;
 		}
 
@@ -608,13 +608,13 @@ static bool mptcp_check_data_fin(struct sock *sk)
 
 		switch (sk->sk_state) {
 		case TCP_ESTABLISHED:
-			inet_sk_state_store(sk, TCP_CLOSE_WAIT);
+			mptcp_set_state(sk, TCP_CLOSE_WAIT);
 			break;
 		case TCP_FIN_WAIT1:
-			inet_sk_state_store(sk, TCP_CLOSING);
+			mptcp_set_state(sk, TCP_CLOSING);
 			break;
 		case TCP_FIN_WAIT2:
-			inet_sk_state_store(sk, TCP_CLOSE);
+			mptcp_set_state(sk, TCP_CLOSE);
 			break;
 		default:
 			/* Other states not expected */
@@ -789,7 +789,7 @@ static bool __mptcp_subflow_error_report(struct sock *sk, struct sock *ssk)
 	 */
 	ssk_state = inet_sk_state_load(ssk);
 	if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
-		inet_sk_state_store(sk, ssk_state);
+		mptcp_set_state(sk, ssk_state);
 	WRITE_ONCE(sk->sk_err, -err);
 
 	/* This barrier is coupled with smp_rmb() in mptcp_poll() */
@@ -2477,7 +2477,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	    inet_sk_state_load(msk->first) == TCP_CLOSE) {
 		if (sk->sk_state != TCP_ESTABLISHED ||
 		    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
-			inet_sk_state_store(sk, TCP_CLOSE);
+			mptcp_set_state(sk, TCP_CLOSE);
 			mptcp_close_wake_up(sk);
 		} else {
 			mptcp_start_tout_timer(sk);
@@ -2572,7 +2572,7 @@ static void mptcp_check_fastclose(struct mptcp_sock *msk)
 		WRITE_ONCE(sk->sk_err, ECONNRESET);
 	}
 
-	inet_sk_state_store(sk, TCP_CLOSE);
+	mptcp_set_state(sk, TCP_CLOSE);
 	WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);
 	smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
 	set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags);
@@ -2707,7 +2707,7 @@ static void mptcp_do_fastclose(struct sock *sk)
 	struct mptcp_subflow_context *subflow, *tmp;
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
-	inet_sk_state_store(sk, TCP_CLOSE);
+	mptcp_set_state(sk, TCP_CLOSE);
 	mptcp_for_each_subflow_safe(msk, subflow, tmp)
 		__mptcp_close_ssk(sk, mptcp_subflow_tcp_sock(subflow),
 				  subflow, MPTCP_CF_FASTCLOSE);
@@ -2925,7 +2925,7 @@ static int mptcp_close_state(struct sock *sk)
 	int next = (int)new_state[sk->sk_state];
 	int ns = next & TCP_STATE_MASK;
 
-	inet_sk_state_store(sk, ns);
+	mptcp_set_state(sk, ns);
 
 	return next & TCP_ACTION_FIN;
 }
@@ -3036,7 +3036,7 @@ bool __mptcp_close(struct sock *sk, long timeout)
 
 	if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
 		mptcp_check_listen_stop(sk);
-		inet_sk_state_store(sk, TCP_CLOSE);
+		mptcp_set_state(sk, TCP_CLOSE);
 		goto cleanup;
 	}
 
@@ -3079,7 +3079,7 @@ bool __mptcp_close(struct sock *sk, long timeout)
 	 * state, let's not keep resources busy for no reasons
 	 */
 	if (subflows_alive == 0)
-		inet_sk_state_store(sk, TCP_CLOSE);
+		mptcp_set_state(sk, TCP_CLOSE);
 
 	sock_hold(sk);
 	pr_debug("msk=%p state=%d", sk, sk->sk_state);
@@ -3145,7 +3145,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 		return -EBUSY;
 
 	mptcp_check_listen_stop(sk);
-	inet_sk_state_store(sk, TCP_CLOSE);
+	mptcp_set_state(sk, TCP_CLOSE);
 
 	mptcp_stop_rtx_timer(sk);
 	mptcp_stop_tout_timer(sk);
@@ -3233,7 +3233,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 	/* this can't race with mptcp_close(), as the msk is
 	 * not yet exposted to user-space
 	 */
-	inet_sk_state_store(nsk, TCP_ESTABLISHED);
+	mptcp_set_state(nsk, TCP_ESTABLISHED);
 
 	/* The msk maintain a ref to each subflow in the connections list */
 	WRITE_ONCE(msk->first, ssk);
@@ -3691,7 +3691,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	if (IS_ERR(ssk))
 		return PTR_ERR(ssk);
 
-	inet_sk_state_store(sk, TCP_SYN_SENT);
+	mptcp_set_state(sk, TCP_SYN_SENT);
 	subflow = mptcp_subflow_ctx(ssk);
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
@@ -3741,7 +3741,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	if (unlikely(err)) {
 		/* avoid leaving a dangling token in an unconnected socket */
 		mptcp_token_destroy(msk);
-		inet_sk_state_store(sk, TCP_CLOSE);
+		mptcp_set_state(sk, TCP_CLOSE);
 		return err;
 	}
 
@@ -3831,13 +3831,13 @@ static int mptcp_listen(struct socket *sock, int backlog)
 		goto unlock;
 	}
 
-	inet_sk_state_store(sk, TCP_LISTEN);
+	mptcp_set_state(sk, TCP_LISTEN);
 	sock_set_flag(sk, SOCK_RCU_FREE);
 
 	lock_sock(ssk);
 	err = __inet_listen_sk(ssk, backlog);
 	release_sock(ssk);
-	inet_sk_state_store(sk, inet_sk_state_load(ssk));
+	mptcp_set_state(sk, inet_sk_state_load(ssk));
 
 	if (!err) {
 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
@@ -3897,7 +3897,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 			__mptcp_close_ssk(newsk, msk->first,
 					  mptcp_subflow_ctx(msk->first), 0);
 			if (unlikely(list_is_singular(&msk->conn_list)))
-				inet_sk_state_store(newsk, TCP_CLOSE);
+				mptcp_set_state(newsk, TCP_CLOSE);
 		}
 	}
 	release_sock(newsk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 64302a1ac306..7ca70dc785af 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -422,7 +422,7 @@ static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct soc
 void __mptcp_sync_state(struct sock *sk, int state)
 {
 	if (sk->sk_state == TCP_SYN_SENT) {
-		inet_sk_state_store(sk, state);
+		mptcp_set_state(sk, state);
 		sk->sk_state_change(sk);
 	}
 }
-- 
2.35.3

[PATCH mptcp-next v5 3/3] selftests: mptcp: check CURRESTAB counters

[Geliang Tang]

This patch adds a new helper chk_cestab_nr() to check the current
established connections counter MIB_CURRESTAB. Set the newly added
variables cestab_ns1 and cestab_ns2 to indicate how many connections
are expected in ns1 or ns2.

Invoke check_cestab() to check the counter during the connection in
do_transfer() and invoke chk_cestab_nr() to re-check it when the
connectin closed. These checks are embedded in add_tests().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 45 ++++++++++++++++---
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 87590a43b50d..3cd066e6e2b0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -56,6 +56,8 @@ unset FAILING_LINKS
 unset test_linkfail
 unset addr_nr_ns1
 unset addr_nr_ns2
+unset cestab_ns1
+unset cestab_ns2
 unset sflags
 unset fastclose
 unset fullmesh
@@ -976,6 +978,33 @@ pm_nl_set_endpoint()
 	fi
 }
 
+chk_cestab_nr()
+{
+	local ns=$1
+	local cestab=$2
+	local count
+
+	print_check "cestab $cestab"
+	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCurrEstab")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$cestab" ]; then
+		fail_test "got $count current establish[s] expected $cestab"
+	else
+		print_ok
+	fi
+}
+
+check_cestab()
+{
+	if [ -n "${cestab_ns1}" ]; then
+		chk_cestab_nr $1 1
+	fi
+	if [ -n "${cestab_ns2}" ]; then
+		chk_cestab_nr $2 1
+	fi
+}
+
 do_transfer()
 {
 	local listener_ns="$1"
@@ -1089,6 +1118,7 @@ do_transfer()
 	local cpid=$!
 
 	pm_nl_set_endpoint $listener_ns $connector_ns $connect_addr
+	check_cestab $listener_ns $connector_ns
 
 	wait $cpid
 	local retc=$?
@@ -2477,47 +2507,52 @@ add_tests()
 	if reset "add single subflow"; then
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_set_limits $ns2 0 1
-		addr_nr_ns2=1 speed=slow \
+		addr_nr_ns2=1 speed=slow cestab_ns2=1 \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
+		chk_cestab_nr $ns2 0
 	fi
 
 	# add signal address
 	if reset "add signal address"; then
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_set_limits $ns2 1 1
-		addr_nr_ns1=1 speed=slow \
+		addr_nr_ns1=1 speed=slow cestab_ns1=1 \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
+		chk_cestab_nr $ns1 0
 	fi
 
 	# add multiple subflows
 	if reset "add multiple subflows"; then
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 0 2
-		addr_nr_ns2=2 speed=slow \
+		addr_nr_ns2=2 speed=slow cestab_ns2=1 \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 2 2 2
+		chk_cestab_nr $ns2 0
 	fi
 
 	# add multiple subflows IPv6
 	if reset "add multiple subflows IPv6"; then
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 0 2
-		addr_nr_ns2=2 speed=slow \
+		addr_nr_ns2=2 speed=slow cestab_ns2=1 \
 			run_tests $ns1 $ns2 dead:beef:1::1
 		chk_join_nr 2 2 2
+		chk_cestab_nr $ns2 0
 	fi
 
 	# add multiple addresses IPv6
 	if reset "add multiple addresses IPv6"; then
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 2 2
-		addr_nr_ns1=2 speed=slow \
+		addr_nr_ns1=2 speed=slow cestab_ns1=1 \
 			run_tests $ns1 $ns2 dead:beef:1::1
 		chk_join_nr 2 2 2
 		chk_add_nr 2 2
+		chk_cestab_nr $ns1 0
 	fi
 }
 
-- 
2.35.3

Re: selftests: mptcp: check CURRESTAB counters: Tests Results

[MPTCP CI]

Hi Geliang,

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):
  - Unstable: 1 failed test(s): packetdrill_fastclose 🔴:
  - Task: https://cirrus-ci.com/task/4995622244188160
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4995622244188160/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 2 failed test(s): packetdrill_fastclose packetdrill_regressions 🔴:
  - Task: https://cirrus-ci.com/task/5558572197609472
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5558572197609472/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6684472104452096
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6684472104452096/summary/summary.txt

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

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


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 (NGI0 Core)

Re: [PATCH mptcp-next v5 0/3] add MPTCP_MIB_CURRESTAB

[Paolo Abeni]

On Wed, 2023-12-13 at 13:56 +0800, Geliang Tang wrote:
> v5:
> - merge first three patches into one as Paolo suggested.
> - drop "EXPORT_SYMBOL_GPL(mptcp_set_state);" as Paolo suggested.
> 
> v4:
> - rebased.
> 
> v3:
> - add mptcp_set_state helper
> 
> v2:
> - use ftrace_regs_get_argument instead of regs_get_kernel_argument to
>   fix build warnings reported kernel test robot.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/460
> 
> Geliang Tang (3):
>   mptcp: add mptcp_set_state
>   mptcp: use mptcp_set_state
>   selftests: mptcp: check CURRESTAB counters
> 
>  net/mptcp/mib.c                               |  1 +
>  net/mptcp/mib.h                               |  8 +++
>  net/mptcp/pm_netlink.c                        |  3 +
>  net/mptcp/protocol.c                          | 56 ++++++++++++-------
>  net/mptcp/protocol.h                          |  1 +
>  net/mptcp/subflow.c                           |  2 +-
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 45 +++++++++++++--
>  7 files changed, 91 insertions(+), 25 deletions(-)
> 
LGTM, thanks!

Acked-by: Paolo Abeni <pabeni@redhat.com>

Re: selftests: mptcp: check CURRESTAB counters: Tests Results

[MPTCP CI]

Hi Geliang,

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):
  - Unstable: 1 failed test(s): packetdrill_mp_join 🔴:
  - Task: https://cirrus-ci.com/task/6631721181904896
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6631721181904896/summary/summary.txt

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

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_diag 🔴:
  - Task: https://cirrus-ci.com/task/5716927507595264
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5716927507595264/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5153977554173952
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5153977554173952/summary/summary.txt

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


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 (NGI0 Core)

Re: [PATCH mptcp-next v5 2/3] mptcp: use mptcp_set_state

[Matthieu Baerts]

Hi Geliang, Paolo,

On 13/12/2023 06:56, Geliang Tang wrote:
> This patch replaces all the 'inet_sk_state_store()' calls under net/mptcp
> with the new helper mptcp_set_state().
> 
> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
> ---
>  net/mptcp/pm_netlink.c |  3 +++
>  net/mptcp/protocol.c   | 38 +++++++++++++++++++-------------------
>  net/mptcp/subflow.c    |  2 +-
>  3 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index bf4d96f6f99a..b93683b5e618 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1048,6 +1048,9 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  	if (err)
>  		return err;
>  
> +	/* avoid replacing inet_sk_state_store with mptcp_set_state here, as the
> +	 * old status is known to be TCP_CLOSE, hence will not affect the count.
> +	 */
>  	inet_sk_state_store(newsk, TCP_LISTEN);

Why not calling mptcp_set_state() here as well? I agree that it will do
nothing more than calling inet_sk_state_store(), but maybe better for
the uniformity?

Also, mptcp_set_state() function name is generic: it is not specific to
change a counter (even if it is what it does *for the moment*). Using it
here could even be useful for extensions done in BPF (a useful hook), maybe?

Anyway, this is not blocking the series. I can apply it, and we can fix
that later with a Squash-to patch if needed.

A small detail: I think the 'Closes' tag should be added on this patch,
not the previous one. (I will try not to forget to fix that when
applying the patch)

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

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

Re: [PATCH mptcp-next v5 3/3] selftests: mptcp: check CURRESTAB counters

[Matthieu Baerts]

Hi Geliang,

On 13/12/2023 06:56, Geliang Tang wrote:
> This patch adds a new helper chk_cestab_nr() to check the current
> established connections counter MIB_CURRESTAB. Set the newly added
> variables cestab_ns1 and cestab_ns2 to indicate how many connections
> are expected in ns1 or ns2.
> 
> Invoke check_cestab() to check the counter during the connection in
> do_transfer() and invoke chk_cestab_nr() to re-check it when the
> connectin closed. These checks are embedded in add_tests().
> 
> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 45 ++++++++++++++++---
>  1 file changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 87590a43b50d..3cd066e6e2b0 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

(...)

>  unset fullmesh
> @@ -976,6 +978,33 @@ pm_nl_set_endpoint()
>  	fi
>  }
>  
> +chk_cestab_nr()
> +{
> +	local ns=$1
> +	local cestab=$2
> +	local count
> +
> +	print_check "cestab $cestab"
> +	count=$(mptcp_lib_get_counter ${ns} "MPTcpExtMPCurrEstab")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$cestab" ]; then
> +		fail_test "got $count current establish[s] expected $cestab"
> +	else
> +		print_ok
> +	fi
> +}
> +
> +check_cestab()
> +{
> +	if [ -n "${cestab_ns1}" ]; then
> +		chk_cestab_nr $1 1
> +	fi
> +	if [ -n "${cestab_ns2}" ]; then
> +		chk_cestab_nr $2 1

Maybe clearer to add a comment (or use local variables?) to say what is
$1 and $2, no?

And maybe best to replace 1 by the value of cestab_ns[12]?

  chk_cestab_nr ${2} ${cestab_ns2}

(but we only create one MPTCP connection at a time... it is just that
'cestab_ns2' seems to refer to a counter, not an action
'chk_cestab_ns2'? Or use it as a counter?)

> +	fi
> +}
> +
>  do_transfer()
>  {
>  	local listener_ns="$1"

In this selftest, we only create one MPTCP connection at a time. Should
we not test this feature in diag.sh as well/instead? Similar to what we
do with chk_msk_inuse(). WDYT?

We can also do that after having reviewed and applied your series
modifying the selftests if it is easier.

Anyway, that's not blocking the series.

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

Re: [PATCH mptcp-next v5 0/3] add MPTCP_MIB_CURRESTAB

[Matthieu Baerts]

Hi Geliang, Paolo,

On 13/12/2023 06:56, Geliang Tang wrote:
> v5:
> - merge first three patches into one as Paolo suggested.
> - drop "EXPORT_SYMBOL_GPL(mptcp_set_state);" as Paolo suggested.

Thank you for the new version and the reviews!

I just applied it as is in our tree (feat. for net-next), with just a
few modifications in the commit messages (s/connectin/connection/ in
patch 3, moved the "Closes" tag from patch 1 to patch 2 and added
Paolo's ACK and my RvB tags):

New patches for t/upstream:
- 2392f842bb49: mptcp: add mptcp_set_state
- d2b2599f8558: mptcp: use mptcp_set_state
- a4a27026e6f7: selftests: mptcp: check CURRESTAB counters
- Results: bfda5dfb6a94..68bf16e584a7 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20231213T115101

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

Re: [PATCH mptcp-next v5 2/3] mptcp: use mptcp_set_state

[Paolo Abeni]

On Wed, 2023-12-13 at 12:25 +0100, Matthieu Baerts wrote:
> Hi Geliang, Paolo,
> 
> On 13/12/2023 06:56, Geliang Tang wrote:
> > This patch replaces all the 'inet_sk_state_store()' calls under net/mptcp
> > with the new helper mptcp_set_state().
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
> > ---
> >  net/mptcp/pm_netlink.c |  3 +++
> >  net/mptcp/protocol.c   | 38 +++++++++++++++++++-------------------
> >  net/mptcp/subflow.c    |  2 +-
> >  3 files changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index bf4d96f6f99a..b93683b5e618 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1048,6 +1048,9 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> >  	if (err)
> >  		return err;
> >  
> > +	/* avoid replacing inet_sk_state_store with mptcp_set_state here, as the
> > +	 * old status is known to be TCP_CLOSE, hence will not affect the count.
> > +	 */
> >  	inet_sk_state_store(newsk, TCP_LISTEN);
> 
> Why not calling mptcp_set_state() here as well? I agree that it will do
> nothing more than calling inet_sk_state_store(), but maybe better for
> the uniformity?

mptcp_set_state() needs to be called under the msk socket lock, while
we can avoid that for inet_sk_state_store() - with some care.

The above code is outside the msk socket lock scope. We can extend it,
but will make the patch more noisy - all other chunks are simple helper
replacements.

If we want this change, we can add a pre-req patch moving
inet_sk_state_store() under the socket lock.

Cheers,

Paolo

> Also, mptcp_set_state() function name is generic: it is not specific to
> change a counter (even if it is what it does *for the moment*). Using it
> here could even be useful for extensions done in BPF (a useful hook), maybe?

Yes, that was the idea: with time we can consolidate in 
mptcp_set_state() more actions. e.g. token destruction.


Cheers,

Paolo

Re: [PATCH mptcp-next v5 2/3] mptcp: use mptcp_set_state

[Matthieu Baerts]

Hi Paolo,

On 15/12/2023 17:30, Paolo Abeni wrote:
> On Wed, 2023-12-13 at 12:25 +0100, Matthieu Baerts wrote:
>> Hi Geliang, Paolo,
>>
>> On 13/12/2023 06:56, Geliang Tang wrote:
>>> This patch replaces all the 'inet_sk_state_store()' calls under net/mptcp
>>> with the new helper mptcp_set_state().
>>>
>>> Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
>>> ---
>>>  net/mptcp/pm_netlink.c |  3 +++
>>>  net/mptcp/protocol.c   | 38 +++++++++++++++++++-------------------
>>>  net/mptcp/subflow.c    |  2 +-
>>>  3 files changed, 23 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index bf4d96f6f99a..b93683b5e618 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -1048,6 +1048,9 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	/* avoid replacing inet_sk_state_store with mptcp_set_state here, as the
>>> +	 * old status is known to be TCP_CLOSE, hence will not affect the count.
>>> +	 */
>>>  	inet_sk_state_store(newsk, TCP_LISTEN);
>>
>> Why not calling mptcp_set_state() here as well? I agree that it will do
>> nothing more than calling inet_sk_state_store(), but maybe better for
>> the uniformity?
> 
> mptcp_set_state() needs to be called under the msk socket lock, while
> we can avoid that for inet_sk_state_store() - with some care.
> 
> The above code is outside the msk socket lock scope. We can extend it,
> but will make the patch more noisy - all other chunks are simple helper
> replacements.

Thank you for the explanations and recommendations!

> If we want this change, we can add a pre-req patch moving
> inet_sk_state_store() under the socket lock.
Is the version from Geliang what you had in mind?

https://lore.kernel.org/r/84b7f01dcee92c4c1ae00523baa5f98f0cb64671.1702889134.git.geliang.tang@linux.dev

>> Also, mptcp_set_state() function name is generic: it is not specific to
>> change a counter (even if it is what it does *for the moment*). Using it
>> here could even be useful for extensions done in BPF (a useful hook), maybe?
> 
> Yes, that was the idea: with time we can consolidate in 
> mptcp_set_state() more actions. e.g. token destruction.

Good idea!

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