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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

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?

> 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'.

> 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?

> [*] 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 :-)

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