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

[Geliang Tang <geliang.tang@suse.com>]

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