* [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: [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
* 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
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.