Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

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

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

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

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

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

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 2 failed test(s): selftest_mptcp_join selftest_simult_flows 🔴:
  - Task: https://cirrus-ci.com/task/6562201610747904
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6562201610747904/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4732614262128640
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4732614262128640/summary/summary.txt

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/5412647641284608
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5412647641284608/summary/summary.txt

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

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4909154062565376
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt

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

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[Matthieu Baerts]

Hi Menglong,

Thank you for the v6!

It looks like the CI is not happy with it:

On 03/11/2022 14:21, MPTCP CI wrote:
> Hi Menglong,
> 
> Thank you for your modifications, that's great!
> 
> Our CI did some validations and here is its report:
> 
> - KVM Validation: normal:
>   - Success! ✅:
>   - Task: https://cirrus-ci.com/task/4909154062565376
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt
> 
> - KVM Validation: debug:
>   - Unstable: 1 failed test(s): selftest_diag 🔴:
>   - Task: https://cirrus-ci.com/task/6035053969408000
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/6035053969408000/summary/summary.txt

As you can see:

----------------------------
(...)
# all listen sockets                                [  ok  ]
# after MPC handshake                               [  ok  ]
# ....chk remote_key                                [  ok  ]
# ....chk no fallback                               [  ok  ]
# chk 2 msk in use                                  [  ok  ]
# chk 0 msk in use after flush                      [  ok  ]
# check fallback                                    [  ok  ]
# many msk socket present                           [ fail ] timeout
while expecting 200 max 201 last 1
# chk many msk in use                               [ fail ] expected
200 found 0
# chk 0 msk in use after flush                      [  ok  ]
----------------------------

I guess one socket is still present after the 'check fallback': you
probably need to modify flush_pids() to wait for the processes to be
over, as suggested on a comment in your v5, no?

https://lore.kernel.org/all/b3f3c01e-4010-d5ce-970d-394711bcd0e1@tessares.net/

I don't think it is a good idea to wait for >= 200 except if it takes a
very long time to have the previous socket terminated. If it does, maybe
we should re-order the test or re-create the netns instead of re-using it.


About the patch 3/4, note that the SIGUSR1 is probably stopping the test
earlier than expected because the interrupt will cause some actions to
stop but still good to check for the 'quit' variable.


Also, one small detail for patch 4/4: can you add "...." at the
beginning of the new lines you print in the selftest, similar to
"....chk no fallback"?

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

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

On Thu, 3 Nov 2022, Matthieu Baerts wrote:

> Hi Menglong,
>
> Thank you for the v6!
>
> It looks like the CI is not happy with it:
>
> On 03/11/2022 14:21, MPTCP CI wrote:
>> Hi Menglong,
>>
>> Thank you for your modifications, that's great!
>>
>> Our CI did some validations and here is its report:
>>
>> - KVM Validation: normal:
>>   - Success! ✅:
>>   - Task: https://cirrus-ci.com/task/4909154062565376
>>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/4909154062565376/summary/summary.txt
>>
>> - KVM Validation: debug:
>>   - Unstable: 1 failed test(s): selftest_diag 🔴:
>>   - Task: https://cirrus-ci.com/task/6035053969408000
>>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/6035053969408000/summary/summary.txt
>
> As you can see:
>
> ----------------------------
> (...)
> # all listen sockets                                [  ok  ]
> # after MPC handshake                               [  ok  ]
> # ....chk remote_key                                [  ok  ]
> # ....chk no fallback                               [  ok  ]
> # chk 2 msk in use                                  [  ok  ]
> # chk 0 msk in use after flush                      [  ok  ]
> # check fallback                                    [  ok  ]
> # many msk socket present                           [ fail ] timeout
> while expecting 200 max 201 last 1
> # chk many msk in use                               [ fail ] expected
> 200 found 0
> # chk 0 msk in use after flush                      [  ok  ]
> ----------------------------
>
> I guess one socket is still present after the 'check fallback': you
> probably need to modify flush_pids() to wait for the processes to be
> over, as suggested on a comment in your v5, no?
>
> https://lore.kernel.org/all/b3f3c01e-4010-d5ce-970d-394711bcd0e1@tessares.net/
>
> I don't think it is a good idea to wait for >= 200 except if it takes a
> very long time to have the previous socket terminated. If it does, maybe
> we should re-order the test or re-create the netns instead of re-using it.
>
>
> About the patch 3/4, note that the SIGUSR1 is probably stopping the test
> earlier than expected because the interrupt will cause some actions to
> stop but still good to check for the 'quit' variable.
>
>
> Also, one small detail for patch 4/4: can you add "...." at the
> beginning of the new lines you print in the selftest, similar to
> "....chk no fallback"?
>

Menglong -

Thanks for the updated patches. The test ran ok on my local system, but 
the CI is slow on the debug build which makes the timing trickier. I don't 
have anything to add to Matthieu's comments above, seems like his 
suggestions will resolve the CI issue.

--
Mat Martineau
Intel

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6201775137488896
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6201775137488896/summary/summary.txt

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4794400253935616
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4794400253935616/summary/summary.txt

Initiator: Matthieu Baerts
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/afd026792ca1


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

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

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4733607273562112
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4733607273562112/summary/summary.txt

Initiator: Matthieu Baerts
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/5afcc5ab13e3


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)

[PATCH mptcp-next v8 0/4] mptcp: add statistics for mptcp socket in use

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

In the 1th patch, we do some code cleanup with replease 'sock->sk'
with 'sk'. In the 2th patch, we add statistics for mptcp socket in
use. In the 3th patch, we make mptcp_connect can exit when receive
'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
for this commit.

With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
I belive that the testing of diag.sh can pass now. In fallback and
simultaneous close case, the msk can't release normal (sometimes?) without
that commit, and makes the testing fail. Enn...let's just see that the
CI is saying~


Changes since v7:
- remove the MPTCP_INUSE flag and do the statistics according to the
  creation and destruction of the token in the 2th patch.

Changes since v6:
- check all processes exit in flush_pids() in the 4th patch

Changes since v5:
- introduce MPTCP_INUSE flag to store if msk is in use, as I find
  that it's not correct to check is a msk is in use by
  !sk_unhashed(sk) in mptcp_destroy_common(), because the token
  can be release in mptcp_check_fastclose()
- add the 3th patch
- reuse __chk_nr in 4th patch

Changes since v4:
- rebase to solve merge conflict

Changes since v3:
- rename MPTCP_DESTROIED to MPTCP_DESTROYED in the 2th patch

Changes since v2:
- add testing

Changes since v1:
- split the code cleanup into the 1th patch.
- decrease the statistics for listening mptcp socket inuse with
  mptcp_listen_inuse_dec()
- add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
  called on the msk. For fallback case, we need to decrease the
  statistics only once, and mptcp_destroy_common() can be called
  more than once.

Menglong Dong (4):
  mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()
  mptcp: add statistics for mptcp socket in use
  selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1
  selftest: mptcp: add test for mptcp socket in use

 net/mptcp/protocol.c                          | 29 +++++++---
 net/mptcp/token.c                             |  7 +++
 tools/testing/selftests/net/mptcp/diag.sh     | 56 +++++++++++++++++--
 .../selftests/net/mptcp/mptcp_connect.c       |  4 +-
 4 files changed, 82 insertions(+), 14 deletions(-)

-- 
2.37.2

[PATCH mptcp-next v8 1/4] mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

'sock->sk' is used frequently in mptcp_listen(). Therefore, we can
introduce the 'sk' and replace 'sock->sk' with it.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/mptcp/protocol.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6d03bdcda33e..ffdccf26af99 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3656,12 +3656,13 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 static int mptcp_listen(struct socket *sock, int backlog)
 {
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
+	struct sock *sk = sock->sk;
 	struct socket *ssock;
 	int err;
 
 	pr_debug("msk=%p", msk);
 
-	lock_sock(sock->sk);
+	lock_sock(sk);
 	ssock = __mptcp_nmpc_socket(msk);
 	if (!ssock) {
 		err = -EINVAL;
@@ -3669,18 +3670,18 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	}
 
 	mptcp_token_destroy(msk);
-	inet_sk_state_store(sock->sk, TCP_LISTEN);
-	sock_set_flag(sock->sk, SOCK_RCU_FREE);
+	inet_sk_state_store(sk, TCP_LISTEN);
+	sock_set_flag(sk, SOCK_RCU_FREE);
 
 	err = ssock->ops->listen(ssock, backlog);
-	inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
+	inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
 	if (!err)
-		mptcp_copy_inaddrs(sock->sk, ssock->sk);
+		mptcp_copy_inaddrs(sk, ssock->sk);
 
 	mptcp_event_pm_listener(ssock->sk, MPTCP_EVENT_LISTENER_CREATED);
 
 unlock:
-	release_sock(sock->sk);
+	release_sock(sk);
 	return err;
 }
 
-- 
2.37.2

[PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Do the statistics of mptcp socket in use with sock_prot_inuse_add().
Therefore, we can get the count of used mptcp socket from
/proc/net/protocols:

& cat /proc/net/protocols
protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v8:
- remove the MPTCP_INUSE flag and do the statistics according to the
  creation and destruction of the token

v6:
- introduce the 'MPTCP_INUSE' flag and check if msk is in use by it

v5:
- rebase to solve merge conflict

v4:
- rename MPTCP_DESTROIED to MPTCP_DESTROYED

v2:
- decrease the statistics for listening mptcp socket inuse with
  mptcp_listen_inuse_dec()
- add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
  called on the msk. For fallback case, we need to decrease the
---
 net/mptcp/protocol.c | 16 +++++++++++++++-
 net/mptcp/token.c    |  7 +++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ffdccf26af99..775454a9d49b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3005,6 +3005,16 @@ void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
 	inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
 }
 
+static void mptcp_listen_inuse_dec(struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct socket *ssock;
+
+	ssock = __mptcp_nmpc_socket(msk);
+	if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
+		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+}
+
 static int mptcp_disconnect(struct sock *sk, int flags)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -3025,6 +3035,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	if (msk->token)
 		mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
 
+	mptcp_listen_inuse_dec(sk);
 	/* msk->subflow is still intact, the following will not free the first
 	 * subflow
 	 */
@@ -3203,6 +3214,7 @@ static void mptcp_destroy(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
+	mptcp_listen_inuse_dec(sk);
 	/* clears msk->subflow, allowing the following to close
 	 * even the initial subflow
 	 */
@@ -3675,8 +3687,10 @@ static int mptcp_listen(struct socket *sock, int backlog)
 
 	err = ssock->ops->listen(ssock, backlog);
 	inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
-	if (!err)
+	if (!err) {
+		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 		mptcp_copy_inaddrs(sk, ssock->sk);
+	}
 
 	mptcp_event_pm_listener(ssock->sk, MPTCP_EVENT_LISTENER_CREATED);
 
diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index 65430f314a68..54064146175f 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -175,6 +175,9 @@ int mptcp_token_new_connect(struct sock *sk)
 	__sk_nulls_add_node_rcu((struct sock *)msk, &bucket->msk_chain);
 	bucket->chain_len++;
 	spin_unlock_bh(&bucket->lock);
+	sock_prot_inuse_add(sock_net(subflow->conn),
+			    subflow->conn->sk_prot,
+			    1);
 	return 0;
 }
 
@@ -190,8 +193,10 @@ void mptcp_token_accept(struct mptcp_subflow_request_sock *req,
 			struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_request_sock *pos;
+	struct sock *sk = (struct sock *)msk;
 	struct token_bucket *bucket;
 
+	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 	bucket = token_bucket(req->token);
 	spin_lock_bh(&bucket->lock);
 
@@ -370,12 +375,14 @@ void mptcp_token_destroy_request(struct request_sock *req)
  */
 void mptcp_token_destroy(struct mptcp_sock *msk)
 {
+	struct sock *sk = (struct sock *)msk;
 	struct token_bucket *bucket;
 	struct mptcp_sock *pos;
 
 	if (sk_unhashed((struct sock *)msk))
 		return;
 
+	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
 	bucket = token_bucket(msk->token);
 	spin_lock_bh(&bucket->lock);
 	pos = __token_lookup_msk(bucket, msk->token);
-- 
2.37.2

[PATCH mptcp-next v8 3/4] selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

For now, mptcp_connect won't exit after receiving the 'SIGUSR1' signal
if '-r' is set. Fix this by skipping poll and sleep in copyfd_io_poll()
if 'quit' is set.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 8a8266957bc5..b25a31445ded 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -627,7 +627,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 		char rbuf[8192];
 		ssize_t len;
 
-		if (fds.events == 0)
+		if (fds.events == 0 || quit)
 			break;
 
 		switch (poll(&fds, 1, poll_timeout)) {
@@ -733,7 +733,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 	}
 
 	/* leave some time for late join/announce */
-	if (cfg_remove)
+	if (cfg_remove && !quit)
 		usleep(cfg_wait);
 
 	return 0;
-- 
2.37.2

[PATCH mptcp-next v8 4/4] selftest: mptcp: add test for mptcp socket in use

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Add the function chk_msk_inuse() to diag.sh, which is used to check the
statistics of mptcp socket in use. As mptcp socket in listen state will
be closed randomly after 'accept', we need to get the count of listening
mptcp socket through 'ss' command.

All tests pass.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v7:
- check all processes exit in flush_pids()
---
 tools/testing/selftests/net/mptcp/diag.sh | 56 +++++++++++++++++++++--
 1 file changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 24bcd7b9bdb2..ef628b16fe9b 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -17,6 +17,11 @@ flush_pids()
 	sleep 1.1
 
 	ip netns pids "${ns}" | xargs --no-run-if-empty kill -SIGUSR1 &>/dev/null
+
+	for _ in $(seq 10); do
+		[ -z "$(ip netns pids "${ns}")" ] && break
+		sleep 0.1
+	done
 }
 
 cleanup()
@@ -37,15 +42,20 @@ if [ $? -ne 0 ];then
 	exit $ksft_skip
 fi
 
+get_msk_inuse()
+{
+	ip netns exec $ns cat /proc/net/protocols | awk '$1~/^MPTCP$/{print $3}'
+}
+
 __chk_nr()
 {
-	local condition="$1"
+	local command="$1"
 	local expected=$2
 	local msg nr
 
 	shift 2
 	msg=$*
-	nr=$(ss -inmHMN $ns | $condition)
+	nr=$(eval $command)
 
 	printf "%-50s" "$msg"
 	if [ $nr != $expected ]; then
@@ -57,9 +67,17 @@ __chk_nr()
 	test_cnt=$((test_cnt+1))
 }
 
+__chk_msk_nr()
+{
+	local condition=$1
+	shift 1
+
+	__chk_nr "ss -inmHMN $ns | $condition" $*
+}
+
 chk_msk_nr()
 {
-	__chk_nr "grep -c token:" $*
+	__chk_msk_nr "grep -c token:" $*
 }
 
 wait_msk_nr()
@@ -97,12 +115,12 @@ wait_msk_nr()
 
 chk_msk_fallback_nr()
 {
-		__chk_nr "grep -c fallback" $*
+		__chk_msk_nr "grep -c fallback" $*
 }
 
 chk_msk_remote_key_nr()
 {
-		__chk_nr "grep -c remote_key" $*
+		__chk_msk_nr "grep -c remote_key" $*
 }
 
 __chk_listen()
@@ -142,6 +160,26 @@ chk_msk_listen()
 	nr=$(ss -Ml $filter | wc -l)
 }
 
+chk_msk_inuse()
+{
+	local expected=$1
+	local listen_nr
+
+	shift 1
+
+	listen_nr=$(ss -N "${ns}" -Ml | grep -c LISTEN)
+	expected=$((expected + listen_nr))
+
+	for _ in $(seq 10); do
+		if [ $(get_msk_inuse) -eq $expected ];then
+			break
+		fi
+		sleep 0.1
+	done
+
+	__chk_nr get_msk_inuse $expected $*
+}
+
 # $1: ns, $2: port
 wait_local_port_listen()
 {
@@ -195,8 +233,10 @@ wait_connected $ns 10000
 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"
 flush_pids
 
+chk_msk_inuse 0 "....chk 0 msk in use after flush"
 
 echo "a" | \
 	timeout ${timeout_test} \
@@ -211,8 +251,11 @@ echo "b" | \
 				127.0.0.1 >/dev/null &
 wait_connected $ns 10001
 chk_msk_fallback_nr 1 "check fallback"
+chk_msk_inuse 1 "....chk 1 msk in use"
 flush_pids
 
+chk_msk_inuse 0 "....chk 0 msk in use after flush"
+
 NR_CLIENTS=100
 for I in `seq 1 $NR_CLIENTS`; do
 	echo "a" | \
@@ -232,6 +275,9 @@ for I in `seq 1 $NR_CLIENTS`; do
 done
 
 wait_msk_nr $((NR_CLIENTS*2)) "many msk socket present"
+chk_msk_inuse $((NR_CLIENTS*2)) "....chk many msk in use"
 flush_pids
 
+chk_msk_inuse 0 "....chk 0 msk in use after flush"
+
 exit $ret
-- 
2.37.2

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

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): insmod - Critical: 3 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5270512411082752
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5270512411082752/summary/summary.txt

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

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/4989037434372096
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4989037434372096/summary/summary.txt

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

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


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)

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Matthieu Baerts]

Hi Menglong Dong,

On 08/12/2022 03:45, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Do the statistics of mptcp socket in use with sock_prot_inuse_add().
> Therefore, we can get the count of used mptcp socket from
> /proc/net/protocols:
> 
> & cat /proc/net/protocols
> protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
> MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v8:
> - remove the MPTCP_INUSE flag and do the statistics according to the
>   creation and destruction of the token

Thank you for the new version!

I marked this series as "Changes Requested" on Patchwork because I guess
you saw the CI was not happy with the KUnit token tests, e.g.:

- KVM Validation: normal (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): insmod - Critical: 3 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5270512411082752
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/5270512411082752/summary/summary.txt
  - Debug: https://cirrus-ci.com/task/4989037434372096

Do you mind looking at that please?

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

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

On Thu, Dec 8, 2022 at 9:53 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong Dong,
>
> On 08/12/2022 03:45, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Do the statistics of mptcp socket in use with sock_prot_inuse_add().
> > Therefore, we can get the count of used mptcp socket from
> > /proc/net/protocols:
> >
> > & cat /proc/net/protocols
> > protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
> > MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> > MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> >
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> > v8:
> > - remove the MPTCP_INUSE flag and do the statistics according to the
> >   creation and destruction of the token
>
> Thank you for the new version!
>
> I marked this series as "Changes Requested" on Patchwork because I guess
> you saw the CI was not happy with the KUnit token tests, e.g.:
>
> - KVM Validation: normal (except selftest_mptcp_join):
>   - Unstable: 1 failed test(s): insmod - Critical: 3 Call Trace(s) ❌:
>   - Task: https://cirrus-ci.com/task/5270512411082752
>   - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/5270512411082752/summary/summary.txt
>   - Debug: https://cirrus-ci.com/task/4989037434372096
>
> Do you mind looking at that please?
>

Of course! I'll run all of the tests to make sure they pass.

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

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Paolo Abeni]

On Thu, 2022-12-08 at 10:45 +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Do the statistics of mptcp socket in use with sock_prot_inuse_add().
> Therefore, we can get the count of used mptcp socket from
> /proc/net/protocols:
> 
> & cat /proc/net/protocols
> protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
> MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v8:
> - remove the MPTCP_INUSE flag and do the statistics according to the
>   creation and destruction of the token
> 
> v6:
> - introduce the 'MPTCP_INUSE' flag and check if msk is in use by it
> 
> v5:
> - rebase to solve merge conflict
> 
> v4:
> - rename MPTCP_DESTROIED to MPTCP_DESTROYED
> 
> v2:
> - decrease the statistics for listening mptcp socket inuse with
>   mptcp_listen_inuse_dec()
> - add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
>   called on the msk. For fallback case, we need to decrease the
> ---
>  net/mptcp/protocol.c | 16 +++++++++++++++-
>  net/mptcp/token.c    |  7 +++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ffdccf26af99..775454a9d49b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3005,6 +3005,16 @@ void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
>  	inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
>  }
>  
> +static void mptcp_listen_inuse_dec(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct socket *ssock;
> +
> +	ssock = __mptcp_nmpc_socket(msk);
> +	if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)

Here I think you can check directly: sk->sk_state == TCP_LISTEN, which
should be both simpler and safer, since this should be called under the
(m)sk socket lock.

It would be nice adding a comment describing why the state check is
needed, e.g.

	/* Listener sockets do not have a valid token, but we need to
	 * account them, too.
	 */

Other then that, LGTM.

Cheers,

Paolo

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Paolo Abeni]

On Thu, 2022-12-08 at 10:45 +0800, menglong8.dong@gmail.com wrote:
> diff --git a/net/mptcp/token.c b/net/mptcp/token.c
> index 65430f314a68..54064146175f 100644
> --- a/net/mptcp/token.c
> +++ b/net/mptcp/token.c
> @@ -175,6 +175,9 @@ int mptcp_token_new_connect(struct sock *sk)
>  	__sk_nulls_add_node_rcu((struct sock *)msk, &bucket->msk_chain);
>  	bucket->chain_len++;
>  	spin_unlock_bh(&bucket->lock);
> +	sock_prot_inuse_add(sock_net(subflow->conn),
> +			    subflow->conn->sk_prot,
> +			    1);

Whoops, sorry for not noticing the above earlier...
Here it would be better rename the mptcp_token_new_connect argument to
'ssk', and add a new local variable:

	struct sock *sk = subflow->conn;

and replace the '(struct sock *)msk', 'subflow->conn' above with 'sk'. 

Side note: the kunit crash is due to build_icsk() being too naive: it
creates a completely zeroed inet_connection_sock, while after this
patch mptcp_token_new_connect() requires that the sk_prot field is
initialized, too. 

The issue could be addresses with an addtional chunk in token_test.c.
Note that for token's test sake, sk_prot could be initialized to any
available/already exported proto.
---
diff --git a/net/mptcp/token_test.c b/net/mptcp/token_test.c
index 5d984bec1cd8..3bffe8d0d36f 100644
--- a/net/mptcp/token_test.c
+++ b/net/mptcp/token_test.c
@@ -36,6 +36,9 @@ static struct inet_connection_sock *build_icsk(struct kunit *test)
 	icsk = kunit_kzalloc(test, sizeof(struct inet_connection_sock),
 			     GFP_USER);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, icsk);
+
+	/* be sure the tocken helpers can dereference sk->sk_prot */
+	icsk->icsk_inet.sk.sk_prot = &tcp_prot;
 	return icsk;
 }
 

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

Hello,

On Tue, Dec 13, 2022 at 12:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Thu, 2022-12-08 at 10:45 +0800, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Do the statistics of mptcp socket in use with sock_prot_inuse_add().
> > Therefore, we can get the count of used mptcp socket from
> > /proc/net/protocols:
> >
> > & cat /proc/net/protocols
> > protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
> > MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> > MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> >
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> > v8:
> > - remove the MPTCP_INUSE flag and do the statistics according to the
> >   creation and destruction of the token
> >
> > v6:
> > - introduce the 'MPTCP_INUSE' flag and check if msk is in use by it
> >
> > v5:
> > - rebase to solve merge conflict
> >
> > v4:
> > - rename MPTCP_DESTROIED to MPTCP_DESTROYED
> >
> > v2:
> > - decrease the statistics for listening mptcp socket inuse with
> >   mptcp_listen_inuse_dec()
> > - add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
> >   called on the msk. For fallback case, we need to decrease the
> > ---
> >  net/mptcp/protocol.c | 16 +++++++++++++++-
> >  net/mptcp/token.c    |  7 +++++++
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index ffdccf26af99..775454a9d49b 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -3005,6 +3005,16 @@ void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
> >       inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
> >  }
> >
> > +static void mptcp_listen_inuse_dec(struct sock *sk)
> > +{
> > +     struct mptcp_sock *msk = mptcp_sk(sk);
> > +     struct socket *ssock;
> > +
> > +     ssock = __mptcp_nmpc_socket(msk);
> > +     if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
>
> Here I think you can check directly: sk->sk_state == TCP_LISTEN, which
> should be both simpler and safer, since this should be called under the
> (m)sk socket lock.
>

Then, mptcp_listen_inuse_dec() needs to be called before msk
enters TCP_CLOSE state, which means __mptcp_close()
and mptcp_disconnect(). Do I miss any case that a listening
msk enters TCP_CLOSE state?

> It would be nice adding a comment describing why the state check is
> needed, e.g.
>
>         /* Listener sockets do not have a valid token, but we need to
>          * account them, too.
>          */
>

Sounds nice!

> Other then that, LGTM.
>
> Cheers,
>
> Paolo
>

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

On Tue, Dec 13, 2022 at 1:03 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Thu, 2022-12-08 at 10:45 +0800, menglong8.dong@gmail.com wrote:
> > diff --git a/net/mptcp/token.c b/net/mptcp/token.c
> > index 65430f314a68..54064146175f 100644
> > --- a/net/mptcp/token.c
> > +++ b/net/mptcp/token.c
> > @@ -175,6 +175,9 @@ int mptcp_token_new_connect(struct sock *sk)
> >       __sk_nulls_add_node_rcu((struct sock *)msk, &bucket->msk_chain);
> >       bucket->chain_len++;
> >       spin_unlock_bh(&bucket->lock);
> > +     sock_prot_inuse_add(sock_net(subflow->conn),
> > +                         subflow->conn->sk_prot,
> > +                         1);
>
> Whoops, sorry for not noticing the above earlier...
> Here it would be better rename the mptcp_token_new_connect argument to
> 'ssk', and add a new local variable:
>
>         struct sock *sk = subflow->conn;
>
> and replace the '(struct sock *)msk', 'subflow->conn' above with 'sk'.
>

Okay!

> Side note: the kunit crash is due to build_icsk() being too naive: it
> creates a completely zeroed inet_connection_sock, while after this
> patch mptcp_token_new_connect() requires that the sk_prot field is
> initialized, too.
>
> The issue could be addresses with an addtional chunk in token_test.c.
> Note that for token's test sake, sk_prot could be initialized to any
> available/already exported proto.
> ---
> diff --git a/net/mptcp/token_test.c b/net/mptcp/token_test.c
> index 5d984bec1cd8..3bffe8d0d36f 100644
> --- a/net/mptcp/token_test.c
> +++ b/net/mptcp/token_test.c
> @@ -36,6 +36,9 @@ static struct inet_connection_sock *build_icsk(struct kunit *test)
>         icsk = kunit_kzalloc(test, sizeof(struct inet_connection_sock),
>                              GFP_USER);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test, icsk);
> +
> +       /* be sure the tocken helpers can dereference sk->sk_prot */
> +       icsk->icsk_inet.sk.sk_prot = &tcp_prot;
>         return icsk;
>  }
>

Get it! Thanks~

Menglong Dong

>

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Paolo Abeni]

On Fri, 2022-12-16 at 16:01 +0800, Menglong Dong wrote:
> Hello,
> 
> On Tue, Dec 13, 2022 at 12:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Thu, 2022-12-08 at 10:45 +0800, menglong8.dong@gmail.com wrote:
> > > From: Menglong Dong <imagedong@tencent.com>
> > > 
> > > Do the statistics of mptcp socket in use with sock_prot_inuse_add().
> > > Therefore, we can get the count of used mptcp socket from
> > > /proc/net/protocols:
> > > 
> > > & cat /proc/net/protocols
> > > protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
> > > MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> > > MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> > > 
> > > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > > ---
> > > v8:
> > > - remove the MPTCP_INUSE flag and do the statistics according to the
> > >   creation and destruction of the token
> > > 
> > > v6:
> > > - introduce the 'MPTCP_INUSE' flag and check if msk is in use by it
> > > 
> > > v5:
> > > - rebase to solve merge conflict
> > > 
> > > v4:
> > > - rename MPTCP_DESTROIED to MPTCP_DESTROYED
> > > 
> > > v2:
> > > - decrease the statistics for listening mptcp socket inuse with
> > >   mptcp_listen_inuse_dec()
> > > - add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
> > >   called on the msk. For fallback case, we need to decrease the
> > > ---
> > >  net/mptcp/protocol.c | 16 +++++++++++++++-
> > >  net/mptcp/token.c    |  7 +++++++
> > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index ffdccf26af99..775454a9d49b 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -3005,6 +3005,16 @@ void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
> > >       inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
> > >  }
> > > 
> > > +static void mptcp_listen_inuse_dec(struct sock *sk)
> > > +{
> > > +     struct mptcp_sock *msk = mptcp_sk(sk);
> > > +     struct socket *ssock;
> > > +
> > > +     ssock = __mptcp_nmpc_socket(msk);
> > > +     if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
> > 
> > Here I think you can check directly: sk->sk_state == TCP_LISTEN, which
> > should be both simpler and safer, since this should be called under the
> > (m)sk socket lock.
> > 
> 
> Then, mptcp_listen_inuse_dec() needs to be called before msk
> enters TCP_CLOSE state, which means __mptcp_close()
> and mptcp_disconnect(). Do I miss any case that a listening
> msk enters TCP_CLOSE state?

AFAIK, that is correct. Note that the number of hooks will not change
compared to the current patch revision (mptcp_disconnect() +
mptcp_destroy()).


Cheers,

Paolo

Re: [PATCH mptcp-next v8 2/4] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

Hello,

On Fri, Dec 16, 2022 at 4:45 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2022-12-16 at 16:01 +0800, Menglong Dong wrote:
> > Hello,
> >
> > On Tue, Dec 13, 2022 at 12:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > > On Thu, 2022-12-08 at 10:45 +0800, menglong8.dong@gmail.com wrote:
> > > > From: Menglong Dong <imagedong@tencent.com>
> > > >
> > > > Do the statistics of mptcp socket in use with sock_prot_inuse_add().
> > > > Therefore, we can get the count of used mptcp socket from
> > > > /proc/net/protocols:
> > > >
> > > > & cat /proc/net/protocols
> > > > protocol  size sockets  memory press maxhdr  slab module     cl co di ac io in de sh ss gs se re sp bi br ha uh gp em
> > > > MPTCPv6   2048      0       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> > > > MPTCP     1896      1       0   no       0   yes  kernel      y  n  y  y  y  y  y  y  y  y  y  y  n  n  n  y  y  y  n
> > > >
> > > > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > > > ---
> > > > v8:
> > > > - remove the MPTCP_INUSE flag and do the statistics according to the
> > > >   creation and destruction of the token
> > > >
> > > > v6:
> > > > - introduce the 'MPTCP_INUSE' flag and check if msk is in use by it
> > > >
> > > > v5:
> > > > - rebase to solve merge conflict
> > > >
> > > > v4:
> > > > - rename MPTCP_DESTROIED to MPTCP_DESTROYED
> > > >
> > > > v2:
> > > > - decrease the statistics for listening mptcp socket inuse with
> > > >   mptcp_listen_inuse_dec()
> > > > - add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
> > > >   called on the msk. For fallback case, we need to decrease the
> > > > ---
> > > >  net/mptcp/protocol.c | 16 +++++++++++++++-
> > > >  net/mptcp/token.c    |  7 +++++++
> > > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > > index ffdccf26af99..775454a9d49b 100644
> > > > --- a/net/mptcp/protocol.c
> > > > +++ b/net/mptcp/protocol.c
> > > > @@ -3005,6 +3005,16 @@ void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
> > > >       inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
> > > >  }
> > > >
> > > > +static void mptcp_listen_inuse_dec(struct sock *sk)
> > > > +{
> > > > +     struct mptcp_sock *msk = mptcp_sk(sk);
> > > > +     struct socket *ssock;
> > > > +
> > > > +     ssock = __mptcp_nmpc_socket(msk);
> > > > +     if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
> > >
> > > Here I think you can check directly: sk->sk_state == TCP_LISTEN, which
> > > should be both simpler and safer, since this should be called under the
> > > (m)sk socket lock.
> > >
> >
> > Then, mptcp_listen_inuse_dec() needs to be called before msk
> > enters TCP_CLOSE state, which means __mptcp_close()
> > and mptcp_disconnect(). Do I miss any case that a listening
> > msk enters TCP_CLOSE state?
>
> AFAIK, that is correct. Note that the number of hooks will not change
> compared to the current patch revision (mptcp_disconnect() +
> mptcp_destroy()).
>

With the changes you suggested, I need to the move mptcp_listen_inuse_dec()
that called in mptcp_destroy() to __mptcp_close(), as msk is already in
CLOSE state in mptcp_destroy(). Isn't it?

Thanks!
Menglong Dong

>
> Cheers,
>
> Paolo
>

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

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/6683962794311680
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6683962794311680/summary/summary.txt

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

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

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

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

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/5688095572492288
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5688095572492288/summary/summary.txt

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

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

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

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


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)

Re: selftest: mptcp: add test for mptcp socket in use: Tests Results

[MPTCP CI]

Hi Menglong,

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): selftest_simult_flows 🔴:
  - Task: https://cirrus-ci.com/task/5235989799501824
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5235989799501824/summary/summary.txt

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

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

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

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


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)