[PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[PATCH mptcp-next v5 0/3] 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. And in the 3th patch, we add the testing for this commit.

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 (3):
  mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()
  mptcp: add statistics for mptcp socket in use
  selftest: mptcp: add test for mptcp socket in use

 net/mptcp/protocol.c                      | 35 ++++++++++++++++++-----
 net/mptcp/protocol.h                      |  1 +
 net/mptcp/subflow.c                       |  3 ++
 tools/testing/selftests/net/mptcp/diag.sh | 28 ++++++++++++++++++
 4 files changed, 60 insertions(+), 7 deletions(-)

-- 
2.37.2

[PATCH mptcp-next v5 1/3] 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 1aa940928b4f..04d92dc0941f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3642,12 +3642,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;
@@ -3655,16 +3656,16 @@ 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);
 
 unlock:
-	release_sock(sock->sk);
+	release_sock(sk);
 	return err;
 }
 
-- 
2.37.2

[PATCH mptcp-next v5 2/3] 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>
---
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
  statistics only once, and mptcp_destroy_common() can be called
  more than once.
---
 net/mptcp/protocol.c | 22 +++++++++++++++++++++-
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  |  3 +++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 04d92dc0941f..3d1570a9d3e9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2974,6 +2974,16 @@ static 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);
@@ -2986,6 +2996,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	if (mptcp_sk(sk)->token)
 		mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
 
+	mptcp_listen_inuse_dec(sk);
 	/* msk->subflow is still intact, the following will not free the first
 	 * subflow
 	 */
@@ -3160,6 +3171,11 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
 	skb_rbtree_purge(&msk->out_of_order_queue);
 	mptcp_data_unlock(sk);
 
+	if ((__mptcp_check_fallback(msk) &&
+	     !test_and_set_bit(MPTCP_DESTROYED, &msk->flags)) ||
+	    !sk_unhashed(sk))
+		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+
 	/* move all the rx fwd alloc into the sk_mem_reclaim_final in
 	 * inet_sock_destruct() will dispose it
 	 */
@@ -3174,6 +3190,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
 	 */
@@ -3529,6 +3546,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 
 	mptcp_token_destroy(msk);
 	inet_sk_state_store(sk, TCP_SYN_SENT);
+	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 	subflow = mptcp_subflow_ctx(ssock->sk);
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
@@ -3661,8 +3679,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);
+	}
 
 unlock:
 	release_sock(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 18f866b1afda..374ef75e4bb9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -116,6 +116,7 @@
 #define MPTCP_WORK_EOF		3
 #define MPTCP_FALLBACK_DONE	4
 #define MPTCP_WORK_CLOSE_SUBFLOW 5
+#define MPTCP_DESTROYED		6
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 07dd23d0fe04..da6cfa73a3bd 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -747,6 +747,9 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
 			mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
 			mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
+			sock_prot_inuse_add(sock_net(new_msk),
+					    new_msk->sk_prot,
+					    1);
 			ctx->conn = new_msk;
 			new_msk = NULL;
 
-- 
2.37.2

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

[Mat Martineau]

On Fri, 7 Oct 2022, 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>
> ---
> 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
>  statistics only once, and mptcp_destroy_common() can be called
>  more than once.
> ---
> net/mptcp/protocol.c | 22 +++++++++++++++++++++-
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  3 +++
> 3 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 04d92dc0941f..3d1570a9d3e9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2974,6 +2974,16 @@ static 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);
> @@ -2986,6 +2996,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
> 	if (mptcp_sk(sk)->token)
> 		mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
>
> +	mptcp_listen_inuse_dec(sk);
> 	/* msk->subflow is still intact, the following will not free the first
> 	 * subflow
> 	 */
> @@ -3160,6 +3171,11 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
> 	skb_rbtree_purge(&msk->out_of_order_queue);
> 	mptcp_data_unlock(sk);
>
> +	if ((__mptcp_check_fallback(msk) &&
> +	     !test_and_set_bit(MPTCP_DESTROYED, &msk->flags)) ||

Hi Menglong -

Sorry I didn't look at this more closely earlier, but did you find that 
mptcp_destroy_common() is only called multiple times with fallback 
sockets?

(This makes me wonder if we have a separate bug to look at, not 
necessarily related to your patch series)

- Mat

> +	    !sk_unhashed(sk))
> +		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> +
> 	/* move all the rx fwd alloc into the sk_mem_reclaim_final in
> 	 * inet_sock_destruct() will dispose it
> 	 */
> @@ -3174,6 +3190,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
> 	 */
> @@ -3529,6 +3546,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>
> 	mptcp_token_destroy(msk);
> 	inet_sk_state_store(sk, TCP_SYN_SENT);
> +	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> 	subflow = mptcp_subflow_ctx(ssock->sk);
> #ifdef CONFIG_TCP_MD5SIG
> 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> @@ -3661,8 +3679,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);
> +	}
>
> unlock:
> 	release_sock(sk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 18f866b1afda..374ef75e4bb9 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -116,6 +116,7 @@
> #define MPTCP_WORK_EOF		3
> #define MPTCP_FALLBACK_DONE	4
> #define MPTCP_WORK_CLOSE_SUBFLOW 5
> +#define MPTCP_DESTROYED		6
>
> /* MPTCP socket release cb flags */
> #define MPTCP_PUSH_PENDING	1
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 07dd23d0fe04..da6cfa73a3bd 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -747,6 +747,9 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 			mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
> 			mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
> 			mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
> +			sock_prot_inuse_add(sock_net(new_msk),
> +					    new_msk->sk_prot,
> +					    1);
> 			ctx->conn = new_msk;
> 			new_msk = NULL;
>
> -- 
> 2.37.2
>
>

--
Mat Martineau
Intel

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

[Menglong Dong]

On Tue, Oct 11, 2022 at 9:11 AM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Fri, 7 Oct 2022, 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>
> > ---
> > 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
> >  statistics only once, and mptcp_destroy_common() can be called
> >  more than once.
> > ---
> > net/mptcp/protocol.c | 22 +++++++++++++++++++++-
> > net/mptcp/protocol.h |  1 +
> > net/mptcp/subflow.c  |  3 +++
> > 3 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 04d92dc0941f..3d1570a9d3e9 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2974,6 +2974,16 @@ static 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);
> > @@ -2986,6 +2996,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
> >       if (mptcp_sk(sk)->token)
> >               mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
> >
> > +     mptcp_listen_inuse_dec(sk);
> >       /* msk->subflow is still intact, the following will not free the first
> >        * subflow
> >        */
> > @@ -3160,6 +3171,11 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
> >       skb_rbtree_purge(&msk->out_of_order_queue);
> >       mptcp_data_unlock(sk);
> >
> > +     if ((__mptcp_check_fallback(msk) &&
> > +          !test_and_set_bit(MPTCP_DESTROYED, &msk->flags)) ||
>
> Hi Menglong -
>
> Sorry I didn't look at this more closely earlier, but did you find that
> mptcp_destroy_common() is only called multiple times with fallback
> sockets?
>
> (This makes me wonder if we have a separate bug to look at, not
> necessarily related to your patch series)

Hello,

mptcp_destroy_common() can be called multiple times with all
sockets, if shutdown is called before close(). Isn't it designed that
mptcp_destroy_common() can be called multiple times?

For no-fallback sockets, it will be unhashed when mptcp_destroy_common()
is called for the first time. So we can check if it is in use by this.

Thanks!
Menglong Dong

>
> - Mat
>
> > +         !sk_unhashed(sk))
> > +             sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> > +
> >       /* move all the rx fwd alloc into the sk_mem_reclaim_final in
> >        * inet_sock_destruct() will dispose it
> >        */
> > @@ -3174,6 +3190,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
> >        */
> > @@ -3529,6 +3546,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> >
> >       mptcp_token_destroy(msk);
> >       inet_sk_state_store(sk, TCP_SYN_SENT);
> > +     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> >       subflow = mptcp_subflow_ctx(ssock->sk);
> > #ifdef CONFIG_TCP_MD5SIG
> >       /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> > @@ -3661,8 +3679,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);
> > +     }
> >
> > unlock:
> >       release_sock(sk);
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 18f866b1afda..374ef75e4bb9 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -116,6 +116,7 @@
> > #define MPTCP_WORK_EOF                3
> > #define MPTCP_FALLBACK_DONE   4
> > #define MPTCP_WORK_CLOSE_SUBFLOW 5
> > +#define MPTCP_DESTROYED              6
> >
> > /* MPTCP socket release cb flags */
> > #define MPTCP_PUSH_PENDING    1
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 07dd23d0fe04..da6cfa73a3bd 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -747,6 +747,9 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >                       mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
> >                       mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
> >                       mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
> > +                     sock_prot_inuse_add(sock_net(new_msk),
> > +                                         new_msk->sk_prot,
> > +                                         1);
> >                       ctx->conn = new_msk;
> >                       new_msk = NULL;
> >
> > --
> > 2.37.2
> >
> >
>
> --
> Mat Martineau
> Intel

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

[Mat Martineau]

On Tue, 11 Oct 2022, Menglong Dong wrote:

> On Tue, Oct 11, 2022 at 9:11 AM Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Fri, 7 Oct 2022, 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>
>>> ---
>>> 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
>>>  statistics only once, and mptcp_destroy_common() can be called
>>>  more than once.
>>> ---
>>> net/mptcp/protocol.c | 22 +++++++++++++++++++++-
>>> net/mptcp/protocol.h |  1 +
>>> net/mptcp/subflow.c  |  3 +++
>>> 3 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 04d92dc0941f..3d1570a9d3e9 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2974,6 +2974,16 @@ static 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);
>>> @@ -2986,6 +2996,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>>>       if (mptcp_sk(sk)->token)
>>>               mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
>>>
>>> +     mptcp_listen_inuse_dec(sk);
>>>       /* msk->subflow is still intact, the following will not free the first
>>>        * subflow
>>>        */
>>> @@ -3160,6 +3171,11 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
>>>       skb_rbtree_purge(&msk->out_of_order_queue);
>>>       mptcp_data_unlock(sk);
>>>
>>> +     if ((__mptcp_check_fallback(msk) &&
>>> +          !test_and_set_bit(MPTCP_DESTROYED, &msk->flags)) ||
>>
>> Hi Menglong -
>>
>> Sorry I didn't look at this more closely earlier, but did you find that
>> mptcp_destroy_common() is only called multiple times with fallback
>> sockets?
>>
>> (This makes me wonder if we have a separate bug to look at, not
>> necessarily related to your patch series)
>
> Hello,
>
> mptcp_destroy_common() can be called multiple times with all
> sockets, if shutdown is called before close(). Isn't it designed that
> mptcp_destroy_common() can be called multiple times?
>

It did look like mptcp_destroy_common() would work fine with multiple 
calls, but I had forgotten about the expected shutdown/close case.

> For no-fallback sockets, it will be unhashed when mptcp_destroy_common()
> is called for the first time. So we can check if it is in use by this.
>

That makes sense, thanks for the details!

- Mat

>>> +         !sk_unhashed(sk))
>>> +             sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>>> +
>>>       /* move all the rx fwd alloc into the sk_mem_reclaim_final in
>>>        * inet_sock_destruct() will dispose it
>>>        */
>>> @@ -3174,6 +3190,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
>>>        */
>>> @@ -3529,6 +3546,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>>>
>>>       mptcp_token_destroy(msk);
>>>       inet_sk_state_store(sk, TCP_SYN_SENT);
>>> +     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>>>       subflow = mptcp_subflow_ctx(ssock->sk);
>>> #ifdef CONFIG_TCP_MD5SIG
>>>       /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
>>> @@ -3661,8 +3679,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);
>>> +     }
>>>
>>> unlock:
>>>       release_sock(sk);
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index 18f866b1afda..374ef75e4bb9 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -116,6 +116,7 @@
>>> #define MPTCP_WORK_EOF                3
>>> #define MPTCP_FALLBACK_DONE   4
>>> #define MPTCP_WORK_CLOSE_SUBFLOW 5
>>> +#define MPTCP_DESTROYED              6
>>>
>>> /* MPTCP socket release cb flags */
>>> #define MPTCP_PUSH_PENDING    1
>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>> index 07dd23d0fe04..da6cfa73a3bd 100644
>>> --- a/net/mptcp/subflow.c
>>> +++ b/net/mptcp/subflow.c
>>> @@ -747,6 +747,9 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>>>                       mptcp_sk(new_msk)->setsockopt_seq = ctx->setsockopt_seq;
>>>                       mptcp_pm_new_connection(mptcp_sk(new_msk), child, 1);
>>>                       mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
>>> +                     sock_prot_inuse_add(sock_net(new_msk),
>>> +                                         new_msk->sk_prot,
>>> +                                         1);
>>>                       ctx->conn = new_msk;
>>>                       new_msk = NULL;
>>>
>>> --
>>> 2.37.2
>>>
>>>
>>
>> --
>> Mat Martineau
>> Intel
>

--
Mat Martineau
Intel

[PATCH mptcp-next v5 3/3] 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.

mptcp_connect command with '-r' flag seems won't exit after flush_pids,
therefore we need consider this additional statistics. But after the
second flush_pids, it will exit.

All tests pass.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 tools/testing/selftests/net/mptcp/diag.sh | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 515859a5168b..d969cb45b8a9 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -141,6 +141,28 @@ chk_msk_listen()
 	nr=$(ss -Ml $filter | wc -l)
 }
 
+chk_msk_inuse()
+{
+	local nr listen_nr
+	local expected=$1
+
+	shift 1
+	msg=$*
+
+	nr=$(ip netns exec $ns awk '$1~/^MPTCP$/{print $3}' /proc/net/protocols)
+	listen_nr=$(ss -N $ns -Ml | grep -c LISTEN)
+	expected=$(($expected+$listen_nr))
+
+	printf "%-50s" "$msg"
+
+	if [ $nr != $expected ]; then
+		echo "[ fail ] expected $expected found $nr"
+		ret=$test_cnt
+	else
+		echo "[  ok  ]"
+	fi
+}
+
 # $1: ns, $2: port
 wait_local_port_listen()
 {
@@ -194,8 +216,11 @@ 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 "msk in use statistics"
 flush_pids
 
+# with '-r' flag, client won't exit after flush_pids
+chk_msk_inuse 1 "msk in use statistics"
 
 echo "a" | \
 	timeout ${timeout_test} \
@@ -231,6 +256,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+1)) "msk in use statistics"
 flush_pids
 
+chk_msk_inuse 0 "msk in use statistics"
+
 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:
  - 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: [PATCH mptcp-next v5 3/3] selftest: mptcp: add test for mptcp socket in use

[Matthieu Baerts]

Hi Menglong,

If you have to edit this commit (see my message on the patch 0/3), I
have some small improvements to suggest.

On 07/10/2022 11:29, menglong8.dong@gmail.com wrote:
> 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.
> 
> mptcp_connect command with '-r' flag seems won't exit after flush_pids,
> therefore we need consider this additional statistics. But after the
> second flush_pids, it will exit.
> 
> All tests pass.
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 515859a5168b..d969cb45b8a9 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -141,6 +141,28 @@ chk_msk_listen()
>  	nr=$(ss -Ml $filter | wc -l)
>  }
>  
> +chk_msk_inuse()
> +{
> +	local nr listen_nr
> +	local expected=$1
> +
> +	shift 1
> +	msg=$*

Can you mark 'msg' as 'local' please?
(we regularly forget to add this keyword, it is possible it is missing
in other places)

> +
> +	nr=$(ip netns exec $ns awk '$1~/^MPTCP$/{print $3}' /proc/net/protocols)
> +	listen_nr=$(ss -N $ns -Ml | grep -c LISTEN)
> +	expected=$(($expected+$listen_nr))
> +
> +	printf "%-50s" "$msg"
> +
> +	if [ $nr != $expected ]; then
> +		echo "[ fail ] expected $expected found $nr"
> +		ret=$test_cnt
> +	else
> +		echo "[  ok  ]"
> +	fi

Could it not be possible to re-use __chk_nr()? (maybe you need to
slightly adapt it?)

> +}
> +
>  # $1: ns, $2: port
>  wait_local_port_listen()
>  {
> @@ -194,8 +216,11 @@ 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 "msk in use statistics"
>  flush_pids
>  
> +# with '-r' flag, client won't exit after flush_pids
> +chk_msk_inuse 1 "msk in use statistics"
>  
>  echo "a" | \
>  	timeout ${timeout_test} \
> @@ -231,6 +256,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+1)) "msk in use statistics"

Why do you expect to see msk + 1?

>  flush_pids
>  
> +chk_msk_inuse 0 "msk in use statistics"

The message "msk in use statistics" is always the same. Maybe clearer if
it is slightly different? e.g. to print something like:

after MPC handshake
....chk remote_key
....chk no fallback
....chk 2 msk in use
....chk 0 msk in use after flush
check fallback
many msk socket present
....chk many msk in use
....chk 0 msk in use after flush

WDYT?

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

Re: [PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[Mat Martineau]

On Fri, 7 Oct 2022, menglong8.dong@gmail.com wrote:

> 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. And in the 3th patch, we add the testing for this commit.
>

I think v5 looks ok for the export branch - we can get more test coverage 
there and update if needed.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> 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 (3):
>  mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()
>  mptcp: add statistics for mptcp socket in use
>  selftest: mptcp: add test for mptcp socket in use
>
> net/mptcp/protocol.c                      | 35 ++++++++++++++++++-----
> net/mptcp/protocol.h                      |  1 +
> net/mptcp/subflow.c                       |  3 ++
> tools/testing/selftests/net/mptcp/diag.sh | 28 ++++++++++++++++++
> 4 files changed, 60 insertions(+), 7 deletions(-)
>
> -- 
> 2.37.2
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[Matthieu Baerts]

Hi Menglong, Mat,

On 12/10/2022 02:53, Mat Martineau wrote:
> On Fri, 7 Oct 2022, menglong8.dong@gmail.com wrote:
> 
>> 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. And in the 3th patch, we add the testing for this commit.
>>
> 
> I think v5 looks ok for the export branch - we can get more test
> coverage there and update if needed.
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for the patch and the review!

I was going to apply it but I just noticed our public CI complained
about the new test during its second test:


 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


The useful bit:

----------------------- 8< -----------------------
not ok 1 test: selftest_diag.tap # exit=7
# no msk on netns creation                          [  ok  ]
# listen match for dport 10000                      [  ok  ]
# listen match for sport 10000                      [  ok  ]
# listen match for saddr and sport                  [  ok  ]
# all listen sockets                                [  ok  ]
# after MPC handshake                               [  ok  ]
# ....chk remote_key                                [  ok  ]
# ....chk no fallback                               [  ok  ]
# msk in use statistics                             [  ok  ]
# msk in use statistics                             [  ok  ]
# check fallback                                    [  ok  ]
# many msk socket present                           [  ok  ]
# msk in use statistics                             [ fail ] expected
249 found 248
# msk in use statistics                             [  ok  ]
----------------------- 8< -----------------------

Do you mind looking at that please?


Please note that the public CI is known to be slow. Here it was failing
with a debug kernel config (KASAN, KMEMLEAK, etc. see
kernel/configs/debug.config file)

If you want to reproduce it in closer conditions, you can do something
like this to build the same kernel and run diag.sh in a loop until it fails:

  $ cd [kernel source code]
  $ echo "run_loop run_selftest_one ./diag.sh" > .virtme-exec-run
  $ 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


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

Re: [PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

On Wed, Oct 12, 2022 at 5:29 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong, Mat,
>
> On 12/10/2022 02:53, Mat Martineau wrote:
> > On Fri, 7 Oct 2022, menglong8.dong@gmail.com wrote:
> >
> >> 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. And in the 3th patch, we add the testing for this commit.
> >>
> >
> > I think v5 looks ok for the export branch - we can get more test
> > coverage there and update if needed.
> >
> > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
> Thank you for the patch and the review!
>
> I was going to apply it but I just noticed our public CI complained
> about the new test during its second test:
>
>
>  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
>
>
> The useful bit:
>
> ----------------------- 8< -----------------------
> not ok 1 test: selftest_diag.tap # exit=7
> # no msk on netns creation                          [  ok  ]
> # listen match for dport 10000                      [  ok  ]
> # listen match for sport 10000                      [  ok  ]
> # listen match for saddr and sport                  [  ok  ]
> # all listen sockets                                [  ok  ]
> # after MPC handshake                               [  ok  ]
> # ....chk remote_key                                [  ok  ]
> # ....chk no fallback                               [  ok  ]
> # msk in use statistics                             [  ok  ]
> # msk in use statistics                             [  ok  ]
> # check fallback                                    [  ok  ]
> # many msk socket present                           [  ok  ]
> # msk in use statistics                             [ fail ] expected
> 249 found 248
> # msk in use statistics                             [  ok  ]
> ----------------------- 8< -----------------------
>
> Do you mind looking at that please?
>
>
> Please note that the public CI is known to be slow. Here it was failing
> with a debug kernel config (KASAN, KMEMLEAK, etc. see
> kernel/configs/debug.config file)
>
> If you want to reproduce it in closer conditions, you can do something
> like this to build the same kernel and run diag.sh in a loop until it fails:
>

Hello,

Seems there is something wrong with the self test that
I added. Maybe it needs some sleep between flush_pids
and chk_msk_fallback_nr().

>   $ cd [kernel source code]
>   $ echo "run_loop run_selftest_one ./diag.sh" > .virtme-exec-run
>   $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
>       --pull always mptcp/mptcp-upstream-virtme-docker:latest \
>       auto-debug
>

Thanks for the command, I'll try to reproduce and solve
it.

Menglong Dong

> For more details:
>
>     https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
>
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

On Wed, Oct 12, 2022 at 5:29 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong, Mat,
>
> On 12/10/2022 02:53, Mat Martineau wrote:
> > On Fri, 7 Oct 2022, menglong8.dong@gmail.com wrote:
> >
> >> 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. And in the 3th patch, we add the testing for this commit.
> >>
> >
> > I think v5 looks ok for the export branch - we can get more test
> > coverage there and update if needed.
> >
> > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
> Thank you for the patch and the review!
>
> I was going to apply it but I just noticed our public CI complained
> about the new test during its second test:
>
>
>  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
>
>
> The useful bit:
>
> ----------------------- 8< -----------------------
> not ok 1 test: selftest_diag.tap # exit=7
> # no msk on netns creation                          [  ok  ]
> # listen match for dport 10000                      [  ok  ]
> # listen match for sport 10000                      [  ok  ]
> # listen match for saddr and sport                  [  ok  ]
> # all listen sockets                                [  ok  ]
> # after MPC handshake                               [  ok  ]
> # ....chk remote_key                                [  ok  ]
> # ....chk no fallback                               [  ok  ]
> # msk in use statistics                             [  ok  ]
> # msk in use statistics                             [  ok  ]
> # check fallback                                    [  ok  ]
> # many msk socket present                           [  ok  ]
> # msk in use statistics                             [ fail ] expected
> 249 found 248
> # msk in use statistics                             [  ok  ]
> ----------------------- 8< -----------------------
>
> Do you mind looking at that please?

Hello,

With the test command on my computer, the 'many msk socket present'
always fails, and bellow is the log:

# no msk on netns creation                          [  ok  ]
# listen match for dport 10000                      [  ok  ]
# listen match for sport 10000                      [  ok  ]
# listen match for saddr and sport                  [  ok  ]
# all listen sockets                                [  ok  ]
# after MPC handshake                               [  ok  ]
# ....chk remote_key                                [  ok  ]
# ....chk no fallback                               [  ok  ]
# msk in use statistics                             [  ok  ]
# msk in use statistics                             [  ok  ]
# check fallback                                    [  ok  ]
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# main_loop_s: timed out
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# connect(): Connection refused
# many msk socket present                           [ fail ] timeout
while expecting 200 max 0 last 0
# msk in use statistics                             [ fail ] expected
201 found 0
# msk in use statistics                             [  ok  ]

Do you have any ideas?

I suspect the failure in your tests has something to do with the '+1'
in 'chk_msk_inuse $((NR_CLIENTS*2+1))'. I add the '+1' as I find
that the 'mptcp_connect' with '-r' won't exit after flush_pids(),
until the second flush_pids(). I'm sure if this opinion is right.

Thanks!
Menglong Dong

>
>
> Please note that the public CI is known to be slow. Here it was failing
> with a debug kernel config (KASAN, KMEMLEAK, etc. see
> kernel/configs/debug.config file)
>
> If you want to reproduce it in closer conditions, you can do something
> like this to build the same kernel and run diag.sh in a loop until it fails:
>
>   $ cd [kernel source code]
>   $ echo "run_loop run_selftest_one ./diag.sh" > .virtme-exec-run
>   $ 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
>
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[Matthieu Baerts]

Hi Menglong,

On 28/10/2022 05:18, Menglong Dong wrote:
> On Wed, Oct 12, 2022 at 5:29 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Menglong, Mat,
>>
>> On 12/10/2022 02:53, Mat Martineau wrote:
>>> On Fri, 7 Oct 2022, menglong8.dong@gmail.com wrote:
>>>
>>>> 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. And in the 3th patch, we add the testing for this commit.
>>>>
>>>
>>> I think v5 looks ok for the export branch - we can get more test
>>> coverage there and update if needed.
>>>
>>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>
>> Thank you for the patch and the review!
>>
>> I was going to apply it but I just noticed our public CI complained
>> about the new test during its second test:

(...)
>> Do you mind looking at that please?
> 
> Hello,
> 
> With the test command on my computer, the 'many msk socket present'
> always fails, and bellow is the log:
> 
> # no msk on netns creation                          [  ok  ]
> # listen match for dport 10000                      [  ok  ]
> # listen match for sport 10000                      [  ok  ]
> # listen match for saddr and sport                  [  ok  ]
> # all listen sockets                                [  ok  ]
> # after MPC handshake                               [  ok  ]
> # ....chk remote_key                                [  ok  ]
> # ....chk no fallback                               [  ok  ]
> # msk in use statistics                             [  ok  ]
> # msk in use statistics                             [  ok  ]
> # check fallback                                    [  ok  ]
> # main_loop_s: timed out
> # connect(): Connection refused
> # main_loop_s: timed out
> # main_loop_s: timed out
> # connect(): Connection refused
> # connect(): Connection refused

(...)

> # many msk socket present                           [ fail ] timeout
> while expecting 200 max 0 last 0
> # msk in use statistics                             [ fail ] expected
> 201 found 0
> # msk in use statistics                             [  ok  ]
> 
> Do you have any ideas?

The test "many msk socket present" creates 100 listening MPTCP sockets,
wait maximum 1 second for them to be ready, then tries to connect to it
and check counters reported by 'ss'.

Could it be possible your environment is quite slow and it needs more
than one second to have the 100 listening MPTCP sockets to be ready? May
you try this patch please?

-------------------- 8< --------------------
diff --git a/tools/testing/selftests/net/mptcp/diag.sh
b/tools/testing/selftests/net/mptcp/diag.sh
index 515859a5168b..72ff61f5dd99 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -150,7 +150,7 @@ wait_local_port_listen()
        local port_hex i

        port_hex="$(printf "%04X" "${port}")"
-       for i in $(seq 10); do
+       for i in $(seq 20); do
                ip netns exec "${listener_ns}" cat /proc/net/tcp | \
                        awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ &&
\$4 ~ /0A/) {rc=0; exit}} END {exit rc}" &&
                        break
-------------------- 8< --------------------

(or even with 'seq 50')


Or is there something limiting the number of sockets being created on
your side? I see that you got 60 connect() errors, maybe limited to 40
somewhere?


> I suspect the failure in your tests has something to do with the '+1'
> in 'chk_msk_inuse $((NR_CLIENTS*2+1))'. I add the '+1' as I find
> that the 'mptcp_connect' with '-r' won't exit after flush_pids(),
> until the second flush_pids(). I'm sure if this opinion is right.

Mmm, flush_pids() sends a 'SIGUSR1' signal to all pids but:

(1) it doesn't wait for mptcp_connect processes to be finished: may you
try with this patch please?

-------------------- 8< --------------------
diff --git a/tools/testing/selftests/net/mptcp/diag.sh
b/tools/testing/selftests/net/mptcp/diag.sh
index 515859a5168b..4ad70ead3bdf 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -16,6 +16,12 @@ 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()
-------------------- 8< --------------------


(2) I think mptcp_connect might not stop in some conditions when SIGUSR1
is received. May you try with this patch please?

-------------------- 8< --------------------
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index e54653ea2ed4..596e3344d5af 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -586,7 +586,7 @@ static int copyfd_io_poll(int infd, int peerfd, int
outfd, bool *in_closed_after
                char rbuf[8192];
                ssize_t len;

-               if (fds.events == 0)
+               if (fds.events == 0 || quit)
                        break;

                switch (poll(&fds, 1, poll_timeout)) {
-------------------- 8< --------------------

If these fixes are needed, best to add them in (a) separated commit(s).
I can also send them if you prefer, they are not directly related to
your series.

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

Re: [PATCH mptcp-next v5 0/3] mptcp: add statistics for mptcp socket in use

[Menglong Dong]

On Tue, Nov 1, 2022 at 1:07 AM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong,
>
> On 28/10/2022 05:18, Menglong Dong wrote:
> > On Wed, Oct 12, 2022 at 5:29 PM Matthieu Baerts
> > <matthieu.baerts@tessares.net> wrote:
> >>
> >> Hi Menglong, Mat,
> >>
> >> On 12/10/2022 02:53, Mat Martineau wrote:
> >>> On Fri, 7 Oct 2022, menglong8.dong@gmail.com wrote:
> >>>
> >>>> 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. And in the 3th patch, we add the testing for this commit.
> >>>>
> >>>
> >>> I think v5 looks ok for the export branch - we can get more test
> >>> coverage there and update if needed.
> >>>
> >>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> >>
> >> Thank you for the patch and the review!
> >>
> >> I was going to apply it but I just noticed our public CI complained
> >> about the new test during its second test:
>
> (...)
> >> Do you mind looking at that please?
> >
> > Hello,
> >
> > With the test command on my computer, the 'many msk socket present'
> > always fails, and bellow is the log:
> >
> > # no msk on netns creation                          [  ok  ]
> > # listen match for dport 10000                      [  ok  ]
> > # listen match for sport 10000                      [  ok  ]
> > # listen match for saddr and sport                  [  ok  ]
> > # all listen sockets                                [  ok  ]
> > # after MPC handshake                               [  ok  ]
> > # ....chk remote_key                                [  ok  ]
> > # ....chk no fallback                               [  ok  ]
> > # msk in use statistics                             [  ok  ]
> > # msk in use statistics                             [  ok  ]
> > # check fallback                                    [  ok  ]
> > # main_loop_s: timed out
> > # connect(): Connection refused
> > # main_loop_s: timed out
> > # main_loop_s: timed out
> > # connect(): Connection refused
> > # connect(): Connection refused
>
> (...)
>
> > # many msk socket present                           [ fail ] timeout
> > while expecting 200 max 0 last 0
> > # msk in use statistics                             [ fail ] expected
> > 201 found 0
> > # msk in use statistics                             [  ok  ]
> >
> > Do you have any ideas?
>
> The test "many msk socket present" creates 100 listening MPTCP sockets,
> wait maximum 1 second for them to be ready, then tries to connect to it
> and check counters reported by 'ss'.
>
> Could it be possible your environment is quite slow and it needs more
> than one second to have the 100 listening MPTCP sockets to be ready? May
> you try this patch please?
>
> -------------------- 8< --------------------
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh
> b/tools/testing/selftests/net/mptcp/diag.sh
> index 515859a5168b..72ff61f5dd99 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -150,7 +150,7 @@ wait_local_port_listen()
>         local port_hex i
>
>         port_hex="$(printf "%04X" "${port}")"
> -       for i in $(seq 10); do
> +       for i in $(seq 20); do
>                 ip netns exec "${listener_ns}" cat /proc/net/tcp | \
>                         awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ &&
> \$4 ~ /0A/) {rc=0; exit}} END {exit rc}" &&
>                         break
> -------------------- 8< --------------------
>
> (or even with 'seq 50')
>
>
> Or is there something limiting the number of sockets being created on
> your side? I see that you got 60 connect() errors, maybe limited to 40
> somewhere?
>

You are right, the qemu that runs the tests is quite slow on
my computer. With increasing the 'timeout_poll', '-w' that passed to
mptcp_connect, etc, finally it works.

Meanwhile, I figure out why the test failed. On a fast computer,
the mptcp_connect with the '-r' won't exit after flush_pids().
However, on a slow computer, such as qemu, it will exit out of the
'timeout' command.

>
> > I suspect the failure in your tests has something to do with the '+1'
> > in 'chk_msk_inuse $((NR_CLIENTS*2+1))'. I add the '+1' as I find
> > that the 'mptcp_connect' with '-r' won't exit after flush_pids(),
> > until the second flush_pids(). I'm sure if this opinion is right.
>
> Mmm, flush_pids() sends a 'SIGUSR1' signal to all pids but:
>
> (1) it doesn't wait for mptcp_connect processes to be finished: may you
> try with this patch please?
>
> -------------------- 8< --------------------
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh
> b/tools/testing/selftests/net/mptcp/diag.sh
> index 515859a5168b..4ad70ead3bdf 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -16,6 +16,12 @@ 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()
> -------------------- 8< --------------------
>
>
> (2) I think mptcp_connect might not stop in some conditions when SIGUSR1
> is received. May you try with this patch please?
>
> -------------------- 8< --------------------
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index e54653ea2ed4..596e3344d5af 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -586,7 +586,7 @@ static int copyfd_io_poll(int infd, int peerfd, int
> outfd, bool *in_closed_after
>                 char rbuf[8192];
>                 ssize_t len;
>
> -               if (fds.events == 0)
> +               if (fds.events == 0 || quit)
>                         break;
>
>                 switch (poll(&fds, 1, poll_timeout)) {
> -------------------- 8< --------------------
>
> If these fixes are needed, best to add them in (a) separated commit(s).
> I can also send them if you prefer, they are not directly related to
> your series.
>

The 'quit' is not used anywhere, which also puzzled me before.
Seems it can exit normally with the second method you raise,
plus the following:

@@ -692,7 +692,7 @@ static int copyfd_io_poll(int infd, int peerfd,
int outfd, bool *in_closed_after
        }

        /* leave some time for late join/announce */
-       if (cfg_remove)
+       if (cfg_remove && !quit)
                usleep(cfg_wait);

I'll add them in a separate commit.

Thanks!
Menglong Dong

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