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

[Paolo Abeni <pabeni@redhat.com>]

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.

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

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

> > [*] 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.
"""

Cheers,

Paolo