net/mptcp/protocol.c | 19 +++++++++++++------ net/mptcp/subflow.c | 3 +++ 2 files changed, 16 insertions(+), 6 deletions(-)
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
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)
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)
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.