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

[Matthieu Baerts <matttbe@kernel.org>]

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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net