Re: [PATCH mptcp-net v2 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed"

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2022-06-15 at 17:59 -0700, Mat Martineau wrote:
> On Wed, 15 Jun 2022, Paolo Abeni wrote:
> 
> > This tries to address a few issues outstanding in the mentioned
> > patch:
> > - we explicitly need to reset the timeout timer for mp_fail's sake
> > - we need to explicitly generate a tcp ack for mp_fail, otherwise
> >  there are no guarantees for suck option being sent out
> > - the timeout timer needs handling need some caring, as it's still
> >  shared between mp_fail and msk socket timeout.
> > - we can re-use msk->first for msk->fail_ssk, as only the first/mpc
> >  subflow can fail without reset. That additionally avoid the need
> >  to clear fail_ssk on the relevant subflow close.
> > - fail_tout would need some additional annotation. Just to be on the
> >  safe side move its manipulaiton under the ssk socket lock.
> > 
> > Last 2 paragraph of the squash to commit should be replaced with:
> > 
> > """
> > It leverages the fact that only the MPC/first subflow can gracefully
> > fail to avoid unneeded subflows traversal: the failing subflow can
> > be only msk->first.
> > 
> > A new 'fail_tout' field is added to the subflow context to record the
> > MP_FAIL response timeout and use such field to reliably share the
> > timeout timer between the MP_FAIL event and the MPTCP socket close timeout.
> > 
> > Finally, a new ack is generated to send out MP_FAIL notification as soon
> > as we hit the relevant condition, instead of waiting a possibly unbound
> > time for the next data packet.
> > """
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/pm.c       |  4 +++-
> > net/mptcp/protocol.c | 54 ++++++++++++++++++++++++++++++++++++--------
> > net/mptcp/protocol.h |  4 ++--
> > net/mptcp/subflow.c  | 24 ++++++++++++++++++--
> > 4 files changed, 72 insertions(+), 14 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 3c7f07bb124e..45e2a48397b9 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -305,13 +305,15 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> > 	if (!READ_ONCE(msk->allow_infinite_fallback))
> > 		return;
> > 
> > -	if (!msk->fail_ssk) {
> > +	if (!subflow->fail_tout) {
> > 		pr_debug("send MP_FAIL response and infinite map");
> > 
> > 		subflow->send_mp_fail = 1;
> > 		subflow->send_infinite_map = 1;
> > +		tcp_send_ack(sk);
> > 	} else {
> > 		pr_debug("MP_FAIL response received");
> > +		WRITE_ONCE(subflow->fail_tout, 0);
> > 	}
> > }
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index a0f9f3831509..50026b8da625 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -500,7 +500,7 @@ static void mptcp_set_timeout(struct sock *sk)
> > 	__mptcp_set_timeout(sk, tout);
> > }
> > 
> > -static bool tcp_can_send_ack(const struct sock *ssk)
> > +static inline bool tcp_can_send_ack(const struct sock *ssk)
> > {
> > 	return !((1 << inet_sk_state_load(ssk)) &
> > 	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
> > @@ -2490,24 +2490,56 @@ static void __mptcp_retrans(struct sock *sk)
> > 		mptcp_reset_timer(sk);
> > }
> > 
> > +/* schedule the timeout timer for the nearest relevant event: either
> > + * close timeout or mp_fail timeout. Both of them could be not
> > + * scheduled yet
> > + */
> > +void mptcp_reset_timeout(struct mptcp_sock *msk, unsigned long fail_tout)
> > +{
> > +	struct sock *sk = (struct sock *)msk;
> > +	unsigned long timeout, close_timeout;
> > +
> > +	if (!fail_tout && !sock_flag(sk, SOCK_DEAD))
> > +		return;
> > +
> > +	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies + TCP_TIMEWAIT_LEN;
> > +
> > +	/* the following is basically time_min(close_timeout, fail_tout) */
> > +	if (!fail_tout)
> > +		timeout = close_timeout;
> > +	else if (!sock_flag(sk, SOCK_DEAD))
> > +		timeout = fail_tout;
> > +	else if (time_after(close_timeout, fail_tout))
> > +		timeout = fail_tout;
> > +	else
> > +		timeout = close_timeout;
> > +
> > +	sk_reset_timer(sk, &sk->sk_timer, timeout);
> > +}
> 
> Hi Paolo -
> 
> The above function seems more complex than needed. If mptcp_close() has 
> been called, the fail timeout should be considered canceled and the 
> "close" has exclusive use of sk_timer. subflow->fail_tout can be set to 0 
> and sk_timer can be set only based on TCP_TIMEWAIT_LEN.

I agree the above function is non trivial, on the flip side, it helps
keeping all the timeout logic in a single place.

Note that msk->first->fail_tout is under the subflow socket lock
protection, and this function is not always invoked under such lock, so
we will need to either add more read/write once annotation or split the
logic in the caller.

Side note: I agree a bit with David Laight wrt *_once preoliferation,
possibly with less divisive language: 

https://lore.kernel.org/netdev/cea2c2c39d0e4f27b2e75cdbc8fce09d@AcuMS.aculab.com/

;)

*_ONCE are a bit hard to track and difficult to verify formally, so I
tried hard to reduce their usage. Currently we have only a single READ
operation outside the relevant lock, which is AFAIK the 'safer'
possible scenario with such annotations. All the write operations are
under the subflow socket lock.

> TCP_RTO_MAX for the MP_FAIL timeout was kind of arbitrary - the spec 
> doesn't say what exactly to do. We didn't want the connection to be in the 
> "unacknowledged" MP_FAIL state forever, but also didn't want to give up 
> too fast.

This function don't make any assumption on the relative timeout length
and should fit the case if we will make them configurable someday

> Given that, do you think the complexity is justified to possibly reset the 
> subflow earlier in this "unacknowledged MP_FAIL followed by a close" 
> scenario?

IMHO yes ;) But if you have strong opinion otherwise I can refactor the
code...

Cheers,

Paolo