diff mbox series

[mptcp-next,v14,3/7] mptcp: add __mptcp_subflow_disconnect helper

Message ID e75487344cde69c5b754ee1fb3bd8c730d54e5de.1697031190.git.geliang.tang@suse.com (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series userspace pm remove id 0 subflow & address | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 46 lines checked
matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ warning Unstable: 1 failed test(s): selftest_mptcp_join

Commit Message

Geliang Tang Oct. 11, 2023, 1:34 p.m. UTC 
When closing the msk->first socket in __mptcp_close_ssk(), if there's
another subflow available, it's better to avoid resetting it, just shut
down it.

This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
we invoke tcp_shutdown() instead of tcp_disconnect().

Co-developed-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Matthieu Baerts (NGI0) Oct. 11, 2023, 4:53 p.m. UTC | #1 
Hi Geliang, Paolo,

On 11/10/2023 15:34, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), if there's
> another subflow available, it's better to avoid resetting it, just shut
> down it.
> 
> This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
> flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
> we invoke tcp_shutdown() instead of tcp_disconnect().
> 
> Co-developed-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30e0c29ae0a4..1a54d55f8bb2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
>  #define MPTCP_CF_PUSH		BIT(1)
>  #define MPTCP_CF_FASTCLOSE	BIT(2)
>  
> +/* be sure to send a reset only if the caller asked for it, also
> + * clean completely the subflow status when the subflow reaches
> + * TCP_CLOSE state
> + */
> +static void __mptcp_subflow_disconnect(struct sock *ssk,
> +				       struct mptcp_subflow_context *subflow,
> +				       unsigned int flags)
> +{
> +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
> +	    (flags & MPTCP_CF_FASTCLOSE)) {
> +		/* The MPTCP code never wait on the subflow sockets, TCP-level
> +		 * disconnect should never fail
> +		 */
> +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> +		mptcp_subflow_ctx_reset(subflow);
> +	} else {
> +		tcp_shutdown(ssk, SEND_SHUTDOWN);

I didn't check in detail but should we not also call
__mptcp_subflow_error_report()?

And maybe other helpers? What is done between the 'goto out' and the
'out' label in __mptcp_close_ssk() here below? → what we do when other
subflows are being closed.

Cheers,
Matt

> +	}
> +}
> +
>  /* subflow sockets can be either outgoing (connect) or incoming
>   * (accept).
>   *
> @@ -2403,7 +2423,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
>  
>  	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
> -		/* be sure to force the tcp_disconnect() path,
> +		/* be sure to force the tcp_close path
>  		 * to generate the egress reset
>  		 */
>  		ssk->sk_lingertime = 0;
> @@ -2413,11 +2433,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  
>  	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
>  	if (!dispose_it) {
> -		/* The MPTCP code never wait on the subflow sockets, TCP-level
> -		 * disconnect should never fail
> -		 */
> -		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> -		mptcp_subflow_ctx_reset(subflow);
> +		__mptcp_subflow_disconnect(ssk, subflow, flags);
>  		release_sock(ssk);
>  
>  		goto out;
Geliang Tang Oct. 12, 2023, 9:03 a.m. UTC | #2 
Hi Matt,

On Wed, Oct 11, 2023 at 06:53:17PM +0200, Matthieu Baerts wrote:
> Hi Geliang, Paolo,
> 
> On 11/10/2023 15:34, Geliang Tang wrote:
> > When closing the msk->first socket in __mptcp_close_ssk(), if there's
> > another subflow available, it's better to avoid resetting it, just shut
> > down it.
> > 
> > This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
> > flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
> > we invoke tcp_shutdown() instead of tcp_disconnect().
> > 
> > Co-developed-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 30e0c29ae0a4..1a54d55f8bb2 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
> >  #define MPTCP_CF_PUSH		BIT(1)
> >  #define MPTCP_CF_FASTCLOSE	BIT(2)
> >  
> > +/* be sure to send a reset only if the caller asked for it, also
> > + * clean completely the subflow status when the subflow reaches
> > + * TCP_CLOSE state
> > + */
> > +static void __mptcp_subflow_disconnect(struct sock *ssk,
> > +				       struct mptcp_subflow_context *subflow,
> > +				       unsigned int flags)
> > +{
> > +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
> > +	    (flags & MPTCP_CF_FASTCLOSE)) {
> > +		/* The MPTCP code never wait on the subflow sockets, TCP-level
> > +		 * disconnect should never fail
> > +		 */
> > +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> > +		mptcp_subflow_ctx_reset(subflow);
> > +	} else {
> > +		tcp_shutdown(ssk, SEND_SHUTDOWN);
> 
> I didn't check in detail but should we not also call
> __mptcp_subflow_error_report()?

When the socket shuts down, it's state is TCPF_FIN_WAIT2.
__mptcp_subflow_error_report will return and do nothing in this case:

      if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
              return false;

So no need to call __mptcp_subflow_error_report.

> 
> And maybe other helpers? What is done between the 'goto out' and the
> 'out' label in __mptcp_close_ssk() here below? → what we do when other
> subflows are being closed.

No need to go the following path too:

        sock_put(ssk);

        if (ssk == msk->first)
                WRITE_ONCE(msk->first, NULL);

So we can keep this patch as it.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> 
> > +	}
> > +}
> > +
> >  /* subflow sockets can be either outgoing (connect) or incoming
> >   * (accept).
> >   *
> > @@ -2403,7 +2423,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
> >  
> >  	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
> > -		/* be sure to force the tcp_disconnect() path,
> > +		/* be sure to force the tcp_close path
> >  		 * to generate the egress reset
> >  		 */
> >  		ssk->sk_lingertime = 0;
> > @@ -2413,11 +2433,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  
> >  	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
> >  	if (!dispose_it) {
> > -		/* The MPTCP code never wait on the subflow sockets, TCP-level
> > -		 * disconnect should never fail
> > -		 */
> > -		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> > -		mptcp_subflow_ctx_reset(subflow);
> > +		__mptcp_subflow_disconnect(ssk, subflow, flags);
> >  		release_sock(ssk);
> >  
> >  		goto out;
> 
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
Matthieu Baerts (NGI0) Oct. 12, 2023, 2:15 p.m. UTC | #3 
Hi Geliang,

On 12/10/2023 11:03, Geliang Tang wrote:
> Hi Matt,
> 
> On Wed, Oct 11, 2023 at 06:53:17PM +0200, Matthieu Baerts wrote:
>> Hi Geliang, Paolo,
>>
>> On 11/10/2023 15:34, Geliang Tang wrote:
>>> When closing the msk->first socket in __mptcp_close_ssk(), if there's
>>> another subflow available, it's better to avoid resetting it, just shut
>>> down it.
>>>
>>> This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
>>> flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
>>> we invoke tcp_shutdown() instead of tcp_disconnect().
>>>
>>> Co-developed-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>>  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 30e0c29ae0a4..1a54d55f8bb2 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
>>>  #define MPTCP_CF_PUSH		BIT(1)
>>>  #define MPTCP_CF_FASTCLOSE	BIT(2)
>>>  
>>> +/* be sure to send a reset only if the caller asked for it, also
>>> + * clean completely the subflow status when the subflow reaches
>>> + * TCP_CLOSE state
>>> + */
>>> +static void __mptcp_subflow_disconnect(struct sock *ssk,
>>> +				       struct mptcp_subflow_context *subflow,
>>> +				       unsigned int flags)
>>> +{
>>> +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
>>> +	    (flags & MPTCP_CF_FASTCLOSE)) {
>>> +		/* The MPTCP code never wait on the subflow sockets, TCP-level
>>> +		 * disconnect should never fail
>>> +		 */
>>> +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
>>> +		mptcp_subflow_ctx_reset(subflow);
>>> +	} else {
>>> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
>>
>> I didn't check in detail but should we not also call
>> __mptcp_subflow_error_report()?
> 
> When the socket shuts down, it's state is TCPF_FIN_WAIT2.
> __mptcp_subflow_error_report will return and do nothing in this case:
> 
>       if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
>               return false;
> 
> So no need to call __mptcp_subflow_error_report.
> 
>>
>> And maybe other helpers? What is done between the 'goto out' and the
>> 'out' label in __mptcp_close_ssk() here below? → what we do when other
>> subflows are being closed.
> 
> No need to go the following path too:
> 
>         sock_put(ssk);
> 
>         if (ssk == msk->first)
>                 WRITE_ONCE(msk->first, NULL);
> 
> So we can keep this patch as it.

Thank you for having check the two cases!

All good then!

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 30e0c29ae0a4..1a54d55f8bb2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2366,6 +2366,26 @@  bool __mptcp_retransmit_pending_data(struct sock *sk)
 #define MPTCP_CF_PUSH		BIT(1)
 #define MPTCP_CF_FASTCLOSE	BIT(2)
 
+/* be sure to send a reset only if the caller asked for it, also
+ * clean completely the subflow status when the subflow reaches
+ * TCP_CLOSE state
+ */
+static void __mptcp_subflow_disconnect(struct sock *ssk,
+				       struct mptcp_subflow_context *subflow,
+				       unsigned int flags)
+{
+	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
+	    (flags & MPTCP_CF_FASTCLOSE)) {
+		/* The MPTCP code never wait on the subflow sockets, TCP-level
+		 * disconnect should never fail
+		 */
+		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
+		mptcp_subflow_ctx_reset(subflow);
+	} else {
+		tcp_shutdown(ssk, SEND_SHUTDOWN);
+	}
+}
+
 /* subflow sockets can be either outgoing (connect) or incoming
  * (accept).
  *
@@ -2403,7 +2423,7 @@  static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
 
 	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
-		/* be sure to force the tcp_disconnect() path,
+		/* be sure to force the tcp_close path
 		 * to generate the egress reset
 		 */
 		ssk->sk_lingertime = 0;
@@ -2413,11 +2433,7 @@  static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
 	if (!dispose_it) {
-		/* The MPTCP code never wait on the subflow sockets, TCP-level
-		 * disconnect should never fail
-		 */
-		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
-		mptcp_subflow_ctx_reset(subflow);
+		__mptcp_subflow_disconnect(ssk, subflow, flags);
 		release_sock(ssk);
 
 		goto out;