Re: [PATCH mptcp-next v16 3/8] Squash to "mptcp: add mptcpi_subflows_total counter"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

Thank you for your replies!

On 13/10/2023 17:35, Paolo Abeni wrote:
> On Fri, 2023-10-13 at 12:46 +0200, Matthieu Baerts wrote:
>> On 13/10/2023 12:32, Paolo Abeni wrote:
>>> Hi,
>>>
>>> On Fri, 2023-10-13 at 13:46 +0800, Geliang Tang wrote:
>>>> Update __mptcp_has_initial_subflow().
>>>>
>>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>>> ---
>>>>  net/mptcp/protocol.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>> index 6508179e94a6..1fb4ac3727c4 100644
>>>> --- a/net/mptcp/protocol.h
>>>> +++ b/net/mptcp/protocol.h
>>>> @@ -1081,7 +1081,7 @@ static inline bool __mptcp_has_initial_subflow(const struct mptcp_sock *msk)
>>>>  {
>>>>  	struct sock *ssk = READ_ONCE(msk->first);
>>>>  
>>>> -	return ssk && inet_sk_state_load(ssk) != TCP_CLOSE;
>>>> +	return ssk && inet_sk_state_load(ssk) == TCP_ESTABLISHED;
>>>
>>> I think the above is not correct, __mptcp_has_initial_subflow() will
>>> return false before connect completes and/or for listener sockets.
>>
>> Please note that __mptcp_has_initial_subflow() is there just to count
>> the number of subflows. It is being used with 'msk->pm.subflows' and it
>> is supposed to have the same "behaviour". Then I don't think we should
>> increment the subflow counter for listener sockets, no?
> 
> If the goal is giving an accurate count of the total number of
> subflows, I think we should: the msk listener has 1 subflow: the tcp
> listener.

Good point! I was thinking about subflows that were visible on the wire,
but yes, there is a subflow there.

If we look at 'ss' output, we will also see one TCP subflow (with ulp
mptcp) for this listener socket, so displaying 1 for the total number of
subflows would make more sense.

So yes, we should add TCPF_LISTEN.

>>> You can list explicitly add the valid states with something alike:
>>>
>>> 	(1 << inet_sk_state_load(ssk)) & (TCPF_ESTABLISHED |
>>> TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_LISTEN | TCPF_CLOSE_WAIT)
>>>
>>> I'm unsure if we should include CLOSE_WAIT here: the remote has shut
>>> down, but this end can still send data...
>>
>> Maybe better, no? As long as the behaviour is similar to the one with
>> 'msk->pm.subflows'.
> 
> If we keep the way we account for MPJ subflows as a reference
> CLOSE_WAIT status must be excluded. I agree/now see it's the better
> option.

OK, so no CLOSE_WAIT, right?

>>> Side important note: you are too fast :) There are a lot of in-flight
>>> patches, and it's difficult to follow each series consistently. I
>>> suggest to focus on a small subset - possibly on a single series at the
>>> time.
>>>
>>> e.g. The first 2 patches in this series are IMHO ready to be merged
>>> [*]. If Mat could apply them, you could follow-up with the remaining
>>> bits of this series.
>>
>> Sure, I can do that.
>>
>> Regarding patch 1/8, do you think we should send that to "-net"? The
>> patch looks OK to me but on the other hand, it is not a big issue to
>> reset the initial subflow (but not ideal) if we fear regressions due to
>> this patch. WDYT?
> 
> I think both patch 1 & 2 should go via -net, but I'm not 110% sure
> there will be not regressions free. Perhaps we can let stage a bit in
> our tree?

Sure, good idea!

I can apply these patches now.

>>> [*] modulo some expansion to the changelog of patch 1, but that could
>>> happen even after merging IMHO.
>>
>> Indeed. But we might forget :)
>> So if you have any suggestions, do not hesitate to share them :-)
> 
> I would re-phrase the commit message roughly as follow:
> 
> """
> When closing the first subflow, the MPTCP protocol unconditionally
> calls tcp_disconnect(), which in turn generates a reset if the subflow
> is established. 
> 
> That is unexpected and different from what MPTCP does with MPJ
> subflows, where resets are generated only on FASTCLOSE and other edge
> scenarios.
> 
> We can't reuse for the first subflow the same code in place for MPJ
> subflows, as MPTCP clean them up completely via a tcp_close() call,
> while must keep the first subflow socket alive for later re-usage, due
> to implementation constraints.
> 
> This patch adds a new helper __mptcp_subflow_disconnect() that
> encapsulates, a logic similar to tcp_close, issuing a reset only when
> the MPTCP_CF_FASTCLOSE flag is set, and performing a clean shutdown
> otherwise.
> """

I suggest to also modify the title, something like:

    mptcp: avoid sending RST when closing the initial subflow

And this Fixes tag:

    Fixes: c2b2ae3925b6 ("mptcp: handle correctly disconnect() failures")

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net