Re: [PATCH mptcp] net: mptcp: add statistics for mptcp socket in use

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

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