All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
@ 2022-09-19 13:21 menglong8.dong
  2022-09-19 15:14 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: menglong8.dong @ 2022-09-19 13:21 UTC (permalink / raw)
  To: pabeni; +Cc: mptcp, Menglong 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>
---
 net/mptcp/protocol.c | 19 +++++++++++++------
 net/mptcp/subflow.c  |  3 +++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 45ed50e9aec9..4da77aa8b070 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2311,6 +2311,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 			tcp_set_state(ssk, TCP_CLOSE);
 			mptcp_subflow_queue_clean(ssk);
 			inet_csk_listen_stop(ssk);
+			sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
 		}
 		__tcp_close(ssk, 0);
 
@@ -3067,6 +3068,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
 	skb_rbtree_purge(&msk->out_of_order_queue);
 	mptcp_data_unlock(sk);
 
+	if (!sk_unhashed(sk) || __mptcp_check_fallback(msk))
+		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
 	 */
@@ -3513,6 +3517,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	mptcp_token_destroy(msk);
 	inet_sk_state_store(sock->sk, TCP_SYN_SENT);
 	subflow = mptcp_subflow_ctx(ssock->sk);
+	sock_prot_inuse_add(sock_net(sock->sk), sock->sk->sk_prot, 1);
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
 	 * TCP option space.
@@ -3547,12 +3552,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;
@@ -3560,16 +3566,17 @@ 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);
+	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 
 	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;
 }
 
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


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: net: mptcp: add statistics for mptcp socket in use: Tests Results
  2022-09-19 13:21 [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use menglong8.dong
@ 2022-09-19 15:14 ` MPTCP CI
  2022-09-19 22:37 ` [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use Mat Martineau
  2022-09-20 10:52 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI
  2 siblings, 0 replies; 9+ messages in thread
From: MPTCP CI @ 2022-09-19 15:14 UTC (permalink / raw)
  To: Menglong Dong; +Cc: mptcp

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

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

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


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)

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
  2022-09-19 13:21 [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use menglong8.dong
  2022-09-19 15:14 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI
@ 2022-09-19 22:37 ` Mat Martineau
  2022-09-20  2:10   ` Menglong Dong
  2022-09-20 10:52 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI
  2 siblings, 1 reply; 9+ messages in thread
From: Mat Martineau @ 2022-09-19 22:37 UTC (permalink / raw)
  To: menglong8.dong; +Cc: pabeni, mptcp, Menglong Dong

On Mon, 19 Sep 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>

Hello Menglong -

Thanks for your patch.

One minor thing: please use the subject line tags listed in 
https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes : 
either "[PATCH mptcp-net]" or "[PATCH mptcp-next]", so it's clear which 
tree the patch is intended for.

> ---
> net/mptcp/protocol.c | 19 +++++++++++++------
> net/mptcp/subflow.c  |  3 +++
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 45ed50e9aec9..4da77aa8b070 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2311,6 +2311,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> 			tcp_set_state(ssk, TCP_CLOSE);
> 			mptcp_subflow_queue_clean(ssk);
> 			inet_csk_listen_stop(ssk);
> +			sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);

The code in this function is for a closing subflow, not a closing MPTCP 
socket. I don't think this call belongs here.

> 		}
> 		__tcp_close(ssk, 0);
>
> @@ -3067,6 +3068,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
> 	skb_rbtree_purge(&msk->out_of_order_queue);
> 	mptcp_data_unlock(sk);
>
> +	if (!sk_unhashed(sk) || __mptcp_check_fallback(msk))
> +		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
> 	 */
> @@ -3513,6 +3517,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> 	mptcp_token_destroy(msk);
> 	inet_sk_state_store(sock->sk, TCP_SYN_SENT);
> 	subflow = mptcp_subflow_ctx(ssock->sk);
> +	sock_prot_inuse_add(sock_net(sock->sk), sock->sk->sk_prot, 1);
> #ifdef CONFIG_TCP_MD5SIG
> 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> 	 * TCP option space.
> @@ -3547,12 +3552,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;

Changing all of the "sock->sk" text in this function to "sk" creates a lot 
of diffs that aren't related to maintaining the inuse statistics. If you'd 
like to do that refactoring change, please split that into a separate 
patch for mptcp-next.

-Mat

> 	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;
> @@ -3560,16 +3566,17 @@ 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);
> +	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>
> 	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;
> }
>
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
  2022-09-19 22:37 ` [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use Mat Martineau
@ 2022-09-20  2:10   ` Menglong Dong
  2022-09-20 11:44     ` Matthieu Baerts
  2022-09-20 22:36     ` Mat Martineau
  0 siblings, 2 replies; 9+ messages in thread
From: Menglong Dong @ 2022-09-20  2:10 UTC (permalink / raw)
  To: Mat Martineau; +Cc: pabeni, mptcp, Menglong Dong

On Tue, Sep 20, 2022 at 6:37 AM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Mon, 19 Sep 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>
>
> Hello Menglong -
>
> Thanks for your patch.
>
> One minor thing: please use the subject line tags listed in
> https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes :
> either "[PATCH mptcp-net]" or "[PATCH mptcp-next]", so it's clear which
> tree the patch is intended for.
>

Thanks for your remind, I'll send the next version with
mptcp-next tag.

> > ---
> > net/mptcp/protocol.c | 19 +++++++++++++------
> > net/mptcp/subflow.c  |  3 +++
> > 2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 45ed50e9aec9..4da77aa8b070 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2311,6 +2311,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >                       tcp_set_state(ssk, TCP_CLOSE);
> >                       mptcp_subflow_queue_clean(ssk);
> >                       inet_csk_listen_stop(ssk);
> > +                     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>
> The code in this function is for a closing subflow, not a closing MPTCP
> socket. I don't think this call belongs here.
>

I think this is the path of the listening mptcp close.
The mptcp socket in listening status is not hashed
to the token hash table, or has 'MPTCP_FALLBACK_DONE'
flags. Therefore, the use statistics of it is not freed in
mptcp_destroy_common().

Hmm...maybe there is a better code path for
this part?

> >               }
> >               __tcp_close(ssk, 0);
> >
> > @@ -3067,6 +3068,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
> >       skb_rbtree_purge(&msk->out_of_order_queue);
> >       mptcp_data_unlock(sk);
> >
> > +     if (!sk_unhashed(sk) || __mptcp_check_fallback(msk))
> > +             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
> >        */
> > @@ -3513,6 +3517,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> >       mptcp_token_destroy(msk);
> >       inet_sk_state_store(sock->sk, TCP_SYN_SENT);
> >       subflow = mptcp_subflow_ctx(ssock->sk);
> > +     sock_prot_inuse_add(sock_net(sock->sk), sock->sk->sk_prot, 1);
> > #ifdef CONFIG_TCP_MD5SIG
> >       /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> >        * TCP option space.
> > @@ -3547,12 +3552,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;
>
> Changing all of the "sock->sk" text in this function to "sk" creates a lot
> of diffs that aren't related to maintaining the inuse statistics. If you'd
> like to do that refactoring change, please split that into a separate
> patch for mptcp-next.
>

Ok, I'll split it into a separate patch.

Thanks!
Menglong Dong

> -Mat
>
> >       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;
> > @@ -3560,16 +3566,17 @@ 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);
> > +     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> >
> >       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;
> > }
> >
> > 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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: net: mptcp: add statistics for mptcp socket in use: Tests Results
  2022-09-19 13:21 [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use menglong8.dong
  2022-09-19 15:14 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI
  2022-09-19 22:37 ` [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use Mat Martineau
@ 2022-09-20 10:52 ` MPTCP CI
  2 siblings, 0 replies; 9+ messages in thread
From: MPTCP CI @ 2022-09-20 10:52 UTC (permalink / raw)
  To: Menglong Dong; +Cc: mptcp

Hi Menglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

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

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

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


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)

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
  2022-09-20  2:10   ` Menglong Dong
@ 2022-09-20 11:44     ` Matthieu Baerts
  2022-09-20 13:47       ` Menglong Dong
  2022-09-20 22:36     ` Mat Martineau
  1 sibling, 1 reply; 9+ messages in thread
From: Matthieu Baerts @ 2022-09-20 11:44 UTC (permalink / raw)
  To: Menglong Dong, Mat Martineau; +Cc: pabeni, mptcp, Menglong Dong

Hi Menglong,

On 20/09/2022 04:10, Menglong Dong wrote:
> On Tue, Sep 20, 2022 at 6:37 AM Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Mon, 19 Sep 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>
>>
>> Hello Menglong -
>>
>> Thanks for your patch.
>>
>> One minor thing: please use the subject line tags listed in
>> https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes :
>> either "[PATCH mptcp-net]" or "[PATCH mptcp-next]", so it's clear which
>> tree the patch is intended for.
>>
> 
> Thanks for your remind, I'll send the next version with
> mptcp-next tag.

Thank you for the your patch.

If you don't mind, can you also strip "net:" prefix from the commit
title? Having 'mptcp:' is enough and that's what was done in most
commits changing files from net/mptcp directory so far.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
  2022-09-20 11:44     ` Matthieu Baerts
@ 2022-09-20 13:47       ` Menglong Dong
  0 siblings, 0 replies; 9+ messages in thread
From: Menglong Dong @ 2022-09-20 13:47 UTC (permalink / raw)
  To: Matthieu Baerts; +Cc: Mat Martineau, pabeni, mptcp, Menglong Dong

On Tue, Sep 20, 2022 at 7:44 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong,
>
> On 20/09/2022 04:10, Menglong Dong wrote:
> > On Tue, Sep 20, 2022 at 6:37 AM Mat Martineau
> > <mathew.j.martineau@linux.intel.com> wrote:
> >>
> >> On Mon, 19 Sep 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>
> >>
> >> Hello Menglong -
> >>
> >> Thanks for your patch.
> >>
> >> One minor thing: please use the subject line tags listed in
> >> https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes :
> >> either "[PATCH mptcp-net]" or "[PATCH mptcp-next]", so it's clear which
> >> tree the patch is intended for.
> >>
> >
> > Thanks for your remind, I'll send the next version with
> > mptcp-next tag.
>
> Thank you for the your patch.
>
> If you don't mind, can you also strip "net:" prefix from the commit
> title? Having 'mptcp:' is enough and that's what was done in most
> commits changing files from net/mptcp directory so far.
>

Oops, I forgot it again. Sure, I'll send the next version
with 'mptcp:' prefix.

Thanks!
Menglong Dong

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
  2022-09-20  2:10   ` Menglong Dong
  2022-09-20 11:44     ` Matthieu Baerts
@ 2022-09-20 22:36     ` Mat Martineau
  2022-09-21  3:25       ` Menglong Dong
  1 sibling, 1 reply; 9+ messages in thread
From: Mat Martineau @ 2022-09-20 22:36 UTC (permalink / raw)
  To: Menglong Dong; +Cc: pabeni, mptcp, Menglong Dong

On Tue, 20 Sep 2022, Menglong Dong wrote:

> On Tue, Sep 20, 2022 at 6:37 AM Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Mon, 19 Sep 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>
>>
>> Hello Menglong -
>>
>> Thanks for your patch.
>>
>> One minor thing: please use the subject line tags listed in
>> https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes :
>> either "[PATCH mptcp-net]" or "[PATCH mptcp-next]", so it's clear which
>> tree the patch is intended for.
>>
>
> Thanks for your remind, I'll send the next version with
> mptcp-next tag.
>
>>> ---
>>> net/mptcp/protocol.c | 19 +++++++++++++------
>>> net/mptcp/subflow.c  |  3 +++
>>> 2 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 45ed50e9aec9..4da77aa8b070 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2311,6 +2311,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>>                       tcp_set_state(ssk, TCP_CLOSE);
>>>                       mptcp_subflow_queue_clean(ssk);
>>>                       inet_csk_listen_stop(ssk);
>>> +                     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>>
>> The code in this function is for a closing subflow, not a closing MPTCP
>> socket. I don't think this call belongs here.
>>
>
> I think this is the path of the listening mptcp close.
> The mptcp socket in listening status is not hashed
> to the token hash table, or has 'MPTCP_FALLBACK_DONE'
> flags. Therefore, the use statistics of it is not freed in
> mptcp_destroy_common().
>
> Hmm...maybe there is a better code path for
> this part?

Ah, right: the above block of code only runs once for the single listener 
subflow.

mptcp_destroy_common() still seems like the better place to adjust the 
counter for the TCP_LISTEN case. Does it work to check the sk_state of 
__mptcp_nmpc_socket(msk) for TCP_LISTEN before the loop in 
mptcp_destroy_common() that calls __mptcp_close_ssk(), like this?

ssock = __mptcp_nmpc_socket(msk);
if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
 	listener = true;

>
>>>               }
>>>               __tcp_close(ssk, 0);
>>>
>>> @@ -3067,6 +3068,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
>>>       skb_rbtree_purge(&msk->out_of_order_queue);
>>>       mptcp_data_unlock(sk);
>>>
>>> +     if (!sk_unhashed(sk) || __mptcp_check_fallback(msk))

then you could change this to

if (!sk_unhashed(sk) || __mptcp_check_fallback(msk) || listener)

?


Thanks,

Mat


>>> +             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
>>>        */
>>> @@ -3513,6 +3517,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>>>       mptcp_token_destroy(msk);
>>>       inet_sk_state_store(sock->sk, TCP_SYN_SENT);
>>>       subflow = mptcp_subflow_ctx(ssock->sk);
>>> +     sock_prot_inuse_add(sock_net(sock->sk), sock->sk->sk_prot, 1);
>>> #ifdef CONFIG_TCP_MD5SIG
>>>       /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
>>>        * TCP option space.
>>> @@ -3547,12 +3552,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;
>>
>> Changing all of the "sock->sk" text in this function to "sk" creates a lot
>> of diffs that aren't related to maintaining the inuse statistics. If you'd
>> like to do that refactoring change, please split that into a separate
>> patch for mptcp-next.
>>
>
> Ok, I'll split it into a separate patch.
>
> Thanks!
> Menglong Dong
>
>> -Mat
>>
>>>       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;
>>> @@ -3560,16 +3566,17 @@ 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);
>>> +     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>>>
>>>       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;
>>> }
>>>
>>> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use
  2022-09-20 22:36     ` Mat Martineau
@ 2022-09-21  3:25       ` Menglong Dong
  0 siblings, 0 replies; 9+ messages in thread
From: Menglong Dong @ 2022-09-21  3:25 UTC (permalink / raw)
  To: Mat Martineau; +Cc: pabeni, mptcp, Menglong Dong

On Wed, Sep 21, 2022 at 6:36 AM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Tue, 20 Sep 2022, Menglong Dong wrote:
>
> > On Tue, Sep 20, 2022 at 6:37 AM Mat Martineau
> > <mathew.j.martineau@linux.intel.com> wrote:
> >>
> >> On Mon, 19 Sep 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>
> >>
> >> Hello Menglong -
> >>
> >> Thanks for your patch.
> >>
> >> One minor thing: please use the subject line tags listed in
> >> https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes :
> >> either "[PATCH mptcp-net]" or "[PATCH mptcp-next]", so it's clear which
> >> tree the patch is intended for.
> >>
> >
> > Thanks for your remind, I'll send the next version with
> > mptcp-next tag.
> >
> >>> ---
> >>> net/mptcp/protocol.c | 19 +++++++++++++------
> >>> net/mptcp/subflow.c  |  3 +++
> >>> 2 files changed, 16 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> >>> index 45ed50e9aec9..4da77aa8b070 100644
> >>> --- a/net/mptcp/protocol.c
> >>> +++ b/net/mptcp/protocol.c
> >>> @@ -2311,6 +2311,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >>>                       tcp_set_state(ssk, TCP_CLOSE);
> >>>                       mptcp_subflow_queue_clean(ssk);
> >>>                       inet_csk_listen_stop(ssk);
> >>> +                     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> >>
> >> The code in this function is for a closing subflow, not a closing MPTCP
> >> socket. I don't think this call belongs here.
> >>
> >
> > I think this is the path of the listening mptcp close.
> > The mptcp socket in listening status is not hashed
> > to the token hash table, or has 'MPTCP_FALLBACK_DONE'
> > flags. Therefore, the use statistics of it is not freed in
> > mptcp_destroy_common().
> >
> > Hmm...maybe there is a better code path for
> > this part?
>
> Ah, right: the above block of code only runs once for the single listener
> subflow.
>
> mptcp_destroy_common() still seems like the better place to adjust the
> counter for the TCP_LISTEN case. Does it work to check the sk_state of
> __mptcp_nmpc_socket(msk) for TCP_LISTEN before the loop in
> mptcp_destroy_common() that calls __mptcp_close_ssk(), like this?
>

I'm afraid that this can't work, as msk->subflow will be disposed
in mptcp_destroy() before mptcp_destroy_common().

Does it work if we move mptcp_dispose_initial_subflow()
behind mptcp_destroy_common()?

Otherwise, how about we pass a MPTCP_CF_LISTEN flag
to mptcp_destroy_common(), like this:

    ssock = __mptcp_nmpc_socket(msk);
    if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
        listener = true;

    mptcp_dispose_initial_subflow(msk);
    mptcp_destroy_common(msk, listener ? MPTCP_CF_LISTEN : 0);

Thanks!
Menglong Dong

> ssock = __mptcp_nmpc_socket(msk);
> if (ssock && inet_sk_state_load(ssock->sk) == TCP_LISTEN)
>         listener = true;
>
> >
> >>>               }
> >>>               __tcp_close(ssk, 0);
> >>>
> >>> @@ -3067,6 +3068,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)
> >>>       skb_rbtree_purge(&msk->out_of_order_queue);
> >>>       mptcp_data_unlock(sk);
> >>>
> >>> +     if (!sk_unhashed(sk) || __mptcp_check_fallback(msk))
>
> then you could change this to
>
> if (!sk_unhashed(sk) || __mptcp_check_fallback(msk) || listener)
>
> ?
>
>
> Thanks,
>
> Mat
>
>
> >>> +             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
> >>>        */
> >>> @@ -3513,6 +3517,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> >>>       mptcp_token_destroy(msk);
> >>>       inet_sk_state_store(sock->sk, TCP_SYN_SENT);
> >>>       subflow = mptcp_subflow_ctx(ssock->sk);
> >>> +     sock_prot_inuse_add(sock_net(sock->sk), sock->sk->sk_prot, 1);
> >>> #ifdef CONFIG_TCP_MD5SIG
> >>>       /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> >>>        * TCP option space.
> >>> @@ -3547,12 +3552,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;
> >>
> >> Changing all of the "sock->sk" text in this function to "sk" creates a lot
> >> of diffs that aren't related to maintaining the inuse statistics. If you'd
> >> like to do that refactoring change, please split that into a separate
> >> patch for mptcp-next.
> >>
> >
> > Ok, I'll split it into a separate patch.
> >
> > Thanks!
> > Menglong Dong
> >
> >> -Mat
> >>
> >>>       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;
> >>> @@ -3560,16 +3566,17 @@ 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);
> >>> +     sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> >>>
> >>>       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;
> >>> }
> >>>
> >>> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-21  3:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-19 13:21 [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use menglong8.dong
2022-09-19 15:14 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI
2022-09-19 22:37 ` [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use Mat Martineau
2022-09-20  2:10   ` Menglong Dong
2022-09-20 11:44     ` Matthieu Baerts
2022-09-20 13:47       ` Menglong Dong
2022-09-20 22:36     ` Mat Martineau
2022-09-21  3:25       ` Menglong Dong
2022-09-20 10:52 ` net: mptcp: add statistics for mptcp socket in use: Tests Results MPTCP CI

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.