[PATCH mptcp-next v2 0/4] fixes for CURRESTAB

[PATCH mptcp-next v2 0/4] fixes for CURRESTAB

[Geliang Tang]

v2:
 - Address Paolo & Matt's comments for v1.

Geliang Tang (4):
  Squash to "mptcp: use mptcp_set_state" 1
  mptcp: move inet_sk_state_store under lock
  Squash to "mptcp: use mptcp_set_state" 2
  selftests: mptcp: diag: check CURRESTAB counters

 net/mptcp/pm_netlink.c                    |  5 +----
 tools/testing/selftests/net/mptcp/diag.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v2 1/4] Squash to "mptcp: use mptcp_set_state" 1

[Geliang Tang]

Remove the comment.

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index b93683b5e618..bf4d96f6f99a 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1048,9 +1048,6 @@ 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);
-- 
2.35.3

[PATCH mptcp-next v2 2/4] mptcp: move inet_sk_state_store under lock

[Geliang Tang]

This patch moves inet_sk_state_store() under the socket lock in
mptcp_pm_nl_create_listen_socket(). This is a pre-req patch for
using mptcp_set_state() instead of inet_sk_state_store().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index bf4d96f6f99a..c35ae045a925 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1030,6 +1030,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 
 	lock_sock(newsk);
 	ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
+	inet_sk_state_store(newsk, TCP_LISTEN);
 	release_sock(newsk);
 	if (IS_ERR(ssk))
 		return PTR_ERR(ssk);
@@ -1048,7 +1049,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (err)
 		return err;
 
-	inet_sk_state_store(newsk, TCP_LISTEN);
 	lock_sock(ssk);
 	err = __inet_listen_sk(ssk, backlog);
 	if (!err)
-- 
2.35.3

[PATCH mptcp-next v2 3/4] Squash to "mptcp: use mptcp_set_state" 2

[Geliang Tang]

Use mptcp_set_state() in mptcp_pm_nl_create_listen_socket().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/pm_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index c35ae045a925..1b754cf525de 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1030,7 +1030,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 
 	lock_sock(newsk);
 	ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
-	inet_sk_state_store(newsk, TCP_LISTEN);
+	mptcp_set_state(newsk, TCP_LISTEN);
 	release_sock(newsk);
 	if (IS_ERR(ssk))
 		return PTR_ERR(ssk);
-- 
2.35.3

[PATCH mptcp-next v2 4/4] selftests: mptcp: diag: check CURRESTAB counters

[Geliang Tang]

This patch adds a new helper chk_msk_cestab() to check the current
established connections counter MIB_CURRESTAB in diag.sh. Invoke it
to check the counter during the connection after every chk_msk_inuse().

Signed-off-by: Geliang Tang <geliang.tang@linux.dev>
---
 tools/testing/selftests/net/mptcp/diag.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 95b498efacd1..649017addb3e 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -182,6 +182,16 @@ chk_msk_inuse()
 	__chk_nr get_msk_inuse $expected "$msg" 0
 }
 
+# $1: ns, $2: cestab nr
+chk_msk_cestab()
+{
+	local ns=$1
+	local cestab=$2
+
+	__chk_nr "mptcp_lib_get_counter ${ns} MPTcpExtMPCurrEstab" \
+		 "${cestab}" "....chk ${cestab} cestab" ""
+}
+
 wait_connected()
 {
 	local listener_ns="${1}"
@@ -219,9 +229,11 @@ chk_msk_nr 2 "after MPC handshake "
 chk_msk_remote_key_nr 2 "....chk remote_key"
 chk_msk_fallback_nr 0 "....chk no fallback"
 chk_msk_inuse 2 "....chk 2 msk in use"
+chk_msk_cestab $ns 2
 flush_pids
 
 chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_cestab $ns 0
 
 echo "a" | \
 	timeout ${timeout_test} \
@@ -237,9 +249,11 @@ echo "b" | \
 wait_connected $ns 10001
 chk_msk_fallback_nr 1 "check fallback"
 chk_msk_inuse 1 "....chk 1 msk in use"
+chk_msk_cestab $ns 1
 flush_pids
 
 chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_cestab $ns 0
 
 NR_CLIENTS=100
 for I in `seq 1 $NR_CLIENTS`; do
@@ -261,9 +275,11 @@ done
 
 wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
 chk_msk_inuse $((NR_CLIENTS*2)) "....chk many msk in use"
+chk_msk_cestab $ns $((NR_CLIENTS*2))
 flush_pids
 
 chk_msk_inuse 0 "....chk 0 msk in use after flush"
+chk_msk_cestab $ns 0
 
 mptcp_lib_result_print_all_tap
 exit $ret
-- 
2.35.3

Re: selftests: mptcp: diag: 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):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4894198806085632
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4894198806085632/summary/summary.txt

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

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

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

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


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 v2 4/4] selftests: mptcp: diag: check CURRESTAB counters

[Matthieu Baerts]

Hi Geliang,

On 19/12/2023 03:03, Geliang Tang wrote:
> This patch adds a new helper chk_msk_cestab() to check the current
> established connections counter MIB_CURRESTAB in diag.sh. Invoke it
> to check the counter during the connection after every chk_msk_inuse().

Thank you for this v2!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 95b498efacd1..649017addb3e 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -182,6 +182,16 @@ chk_msk_inuse()
>  	__chk_nr get_msk_inuse $expected "$msg" 0
>  }
>  
> +# $1: ns, $2: cestab nr
> +chk_msk_cestab()
> +{
> +	local ns=$1

It looks like there is only one 'netns' in this selftests. I guess we
can drop this parameter and use `${ns}` directly, no?

> +	local cestab=$2
> +
> +	__chk_nr "mptcp_lib_get_counter ${ns} MPTcpExtMPCurrEstab" \
> +		 "${cestab}" "....chk ${cestab} cestab" ""

You still need to modify __chk_nr() as I mentioned in my previous
comment on the v1. If you don't, it means this test will not be skipped
(and failed) when running on an older kernel where the new counter is
not available:

  $ chk() { v="${1:-NOSKIP}"; [ "${v}" = "" ] && echo "[ skip ]";
            echo "\${v} == '${v}'"; }
  $ chk "test"
  ${v} == 'test'
  $ chk              ## <- we don't set the parameter, we don't skip: OK
  ${v} == 'NOSKIP'
  $ chk ""           ## <- not what we want: mptcp_lib_get_counter()
  ${v} == 'NOSKIP'   ##    will return nothing if there is no counter


With the modification I suggested, we will skip the test when the new
counter is not available:

  $ chk() { v="${1-NOSKIP}"; [ "${v}" = "" ] && echo "[ skip ]";
            echo "\${v} == '${v}'"; }
  $ chk "test"
  ${v} == 'test'
  $ chk              ## <- we don't set the parameter, we don't skip: OK
  ${v} == 'NOSKIP'
  $ chk ""           ## <- what we want: if there is no counter, we skip
  [ skip ]
  ${v} == ''

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