[mptcp-next 0/2] Fix mptcp connection hangs after link failover

[mptcp-next 0/2] Fix mptcp connection hangs after link failover

[Florian Westphal]

First patch is preparation work: tx_pending_data counter is unreliable.
Second patch fixes premature stop of the retransmit timer.

Florian Westphal (2):
  mptcp: remove tx_pending_data
  mptcp: re-set push-pending bit on retransmit failure

 net/mptcp/protocol.c | 32 +++++++++++++++++++++++++-------
 net/mptcp/protocol.h |  1 -
 2 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.32.0

[mptcp-next 1/2] mptcp: remove tx_pending_data

[Florian Westphal]

The update on recovery is not correct.

msk->tx_pending_data += msk->snd_nxt - rtx_head->data_seq;

will update tx_pending_data multiple times when a subflow is declared
stale while earlier recovery is still in progress.
This means that tx_pending_data will still be positive even after
all data as has been transmitted.

Rather than fix it, remove this field: there are no consumers.
The outstanding data byte count can be computed either via

 "msk->write_seq - rtx_head->data_seq" or
 "msk->write_seq - msk->snd_una".

The latter is more recent/accurate estimate as rtx_head adjustment
is deferred until mptcp lock can be acquired.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/mptcp/protocol.c | 4 ----
 net/mptcp/protocol.h | 1 -
 2 files changed, 5 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2a525c7ae920..c0e0ee4cb24f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1531,7 +1531,6 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
 	dfrag->already_sent += sent;
 
 	msk->snd_burst -= sent;
-	msk->tx_pending_data -= sent;
 
 	snd_nxt_new += dfrag->already_sent;
 
@@ -1761,7 +1760,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		frag_truesize += psize;
 		pfrag->offset += frag_truesize;
 		WRITE_ONCE(msk->write_seq, msk->write_seq + psize);
-		msk->tx_pending_data += psize;
 
 		/* charge data on mptcp pending queue to the msk socket
 		 * Note: we charge such data both to sk and ssk
@@ -2254,7 +2252,6 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
 	mptcp_data_unlock(sk);
 
 	msk->first_pending = rtx_head;
-	msk->tx_pending_data += msk->snd_nxt - rtx_head->data_seq;
 	msk->snd_burst = 0;
 
 	/* be sure to clear the "sent status" on all re-injected fragments */
@@ -2525,7 +2522,6 @@ static int __mptcp_init_sock(struct sock *sk)
 	msk->first_pending = NULL;
 	msk->wmem_reserved = 0;
 	WRITE_ONCE(msk->rmem_released, 0);
-	msk->tx_pending_data = 0;
 	msk->timer_ival = TCP_RTO_MIN;
 
 	msk->first = NULL;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 99a23fff7b03..8416810afa8e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -254,7 +254,6 @@ struct mptcp_sock {
 	struct sk_buff  *ooo_last_skb;
 	struct rb_root  out_of_order_queue;
 	struct sk_buff_head receive_queue;
-	int		tx_pending_data;
 	struct list_head conn_list;
 	struct list_head rtx_queue;
 	struct mptcp_data_frag *first_pending;
-- 
2.32.0

Re: [mptcp-next 1/2] mptcp: remove tx_pending_data

[Paolo Abeni]

On Mon, 2021-09-06 at 08:06 +0200, Florian Westphal wrote:
> The update on recovery is not correct.
> 
> msk->tx_pending_data += msk->snd_nxt - rtx_head->data_seq;
> 
> will update tx_pending_data multiple times when a subflow is declared
> stale while earlier recovery is still in progress.
> This means that tx_pending_data will still be positive even after
> all data as has been transmitted.
> 
> Rather than fix it, remove this field: there are no consumers.

Yup, I kept it around just to eventually dumping it someday via e.g.
the diag interface...

> The outstanding data byte count can be computed either via
> 
>  "msk->write_seq - rtx_head->data_seq" or
>  "msk->write_seq - msk->snd_una".
> 
> The latter is more recent/accurate estimate as rtx_head adjustment
> is deferred until mptcp lock can be acquired.

... but the latter will suffice for that goal so...
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Paolo Abeni <pabeni@redhat.com>

:)
> ---
>  net/mptcp/protocol.c | 4 ----
>  net/mptcp/protocol.h | 1 -
>  2 files changed, 5 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2a525c7ae920..c0e0ee4cb24f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1531,7 +1531,6 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
>  	dfrag->already_sent += sent;
>  
>  	msk->snd_burst -= sent;
> -	msk->tx_pending_data -= sent;
>  
>  	snd_nxt_new += dfrag->already_sent;
>  
> @@ -1761,7 +1760,6 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		frag_truesize += psize;
>  		pfrag->offset += frag_truesize;
>  		WRITE_ONCE(msk->write_seq, msk->write_seq + psize);
> -		msk->tx_pending_data += psize;
>  
>  		/* charge data on mptcp pending queue to the msk socket
>  		 * Note: we charge such data both to sk and ssk
> @@ -2254,7 +2252,6 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
>  	mptcp_data_unlock(sk);
>  
>  	msk->first_pending = rtx_head;
> -	msk->tx_pending_data += msk->snd_nxt - rtx_head->data_seq;
>  	msk->snd_burst = 0;
>  
>  	/* be sure to clear the "sent status" on all re-injected fragments */
> @@ -2525,7 +2522,6 @@ static int __mptcp_init_sock(struct sock *sk)
>  	msk->first_pending = NULL;
>  	msk->wmem_reserved = 0;
>  	WRITE_ONCE(msk->rmem_released, 0);
> -	msk->tx_pending_data = 0;
>  	msk->timer_ival = TCP_RTO_MIN;
>  
>  	msk->first = NULL;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 99a23fff7b03..8416810afa8e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -254,7 +254,6 @@ struct mptcp_sock {
>  	struct sk_buff  *ooo_last_skb;
>  	struct rb_root  out_of_order_queue;
>  	struct sk_buff_head receive_queue;
> -	int		tx_pending_data;
>  	struct list_head conn_list;
>  	struct list_head rtx_queue;
>  	struct mptcp_data_frag *first_pending;

[mptcp-next 2/2] mptcp: re-set push-pending bit on retransmit failure

[Florian Westphal]

The retransmit head will be NULL in case there is no in-flight data
(meaning all data injected into network has been acked).

In that case the retransmit timer is stopped.

This is only correct if there is no more pending, not-yet-sent data.

If there is, the retransmit timer needs to set the PENDING bit again so
that mptcp tries to send the remaining (new) data once a subflow can accept
more data.

Also, mptcp_subflow_get_retrans() has to be called unconditionally.
This function checks for subflows that have become unresponsive and marks
them as stale, so in the case where the rtx queue is empty, subflows
will never be marked stale which prevents available backup subflows from
becoming eligible for transmit.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/mptcp/protocol.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c0e0ee4cb24f..b88a9b61025b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1105,7 +1105,8 @@ static void __mptcp_clean_una(struct sock *sk)
 	if (cleaned && tcp_under_memory_pressure(sk))
 		__mptcp_mem_reclaim_partial(sk);
 
-	if (snd_una == READ_ONCE(msk->snd_nxt) && !msk->recovery) {
+	if (snd_una == READ_ONCE(msk->snd_nxt) &&
+	    snd_una == READ_ONCE(msk->write_seq)) {
 		if (mptcp_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
 			mptcp_stop_timer(sk);
 	} else {
@@ -1547,6 +1548,13 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
 		msk->snd_nxt = snd_nxt_new;
 }
 
+static void mptcp_check_and_set_pending(struct sock *sk)
+{
+	if (mptcp_send_head(sk) &&
+	    !test_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags))
+		set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags);
+}
+
 void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 {
 	struct sock *prev_ssk = NULL, *ssk = NULL;
@@ -1603,6 +1611,13 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 		mptcp_push_release(sk, ssk, &info);
 
 out:
+	/* Set PENDING again in case we found an ssk
+	 * that could not accept more data
+	 */
+	if (unlikely(copied == 0) &&
+	    READ_ONCE(msk->snd_una) == msk->snd_nxt && ssk)
+		mptcp_check_and_set_pending(sk);
+
 	/* ensure the rtx timer is running */
 	if (!mptcp_timer_pending(sk))
 		mptcp_reset_timer(sk);
@@ -2414,6 +2429,9 @@ static void __mptcp_retrans(struct sock *sk)
 	int ret;
 
 	mptcp_clean_una_wakeup(sk);
+
+	/* first check ssk: need to kick "stale" logic */
+	ssk = mptcp_subflow_get_retrans(msk);
 	dfrag = mptcp_rtx_head(sk);
 	if (!dfrag) {
 		if (mptcp_data_fin_enabled(msk)) {
@@ -2426,10 +2444,12 @@ static void __mptcp_retrans(struct sock *sk)
 			goto reset_timer;
 		}
 
-		return;
+		if (!mptcp_send_head(sk))
+			return;
+
+		goto reset_timer;
 	}
 
-	ssk = mptcp_subflow_get_retrans(msk);
 	if (!ssk)
 		goto reset_timer;
 
@@ -2456,6 +2476,8 @@ static void __mptcp_retrans(struct sock *sk)
 	release_sock(ssk);
 
 reset_timer:
+	mptcp_check_and_set_pending(sk);
+
 	if (!mptcp_timer_pending(sk))
 		mptcp_reset_timer(sk);
 }
-- 
2.32.0

Re: [mptcp-next 2/2] mptcp: re-set push-pending bit on retransmit failure

[Paolo Abeni]

On Mon, 2021-09-06 at 08:06 +0200, Florian Westphal wrote:
> The retransmit head will be NULL in case there is no in-flight data
> (meaning all data injected into network has been acked).
> 
> In that case the retransmit timer is stopped.
> 
> This is only correct if there is no more pending, not-yet-sent data.
> 
> If there is, the retransmit timer needs to set the PENDING bit again so
> that mptcp tries to send the remaining (new) data once a subflow can accept
> more data.
> 
> Also, mptcp_subflow_get_retrans() has to be called unconditionally.
> This function checks for subflows that have become unresponsive and marks
> them as stale, so in the case where the rtx queue is empty, subflows
> will never be marked stale which prevents available backup subflows from
> becoming eligible for transmit.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Looks great, but I'm struggling to follow the code in a couple of
points, see below...

> ---
>  net/mptcp/protocol.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c0e0ee4cb24f..b88a9b61025b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1105,7 +1105,8 @@ static void __mptcp_clean_una(struct sock *sk)
>  	if (cleaned && tcp_under_memory_pressure(sk))
>  		__mptcp_mem_reclaim_partial(sk);
>  
> -	if (snd_una == READ_ONCE(msk->snd_nxt) && !msk->recovery) {
> +	if (snd_una == READ_ONCE(msk->snd_nxt) &&
> +	    snd_una == READ_ONCE(msk->write_seq)) {
>  		if (mptcp_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
>  			mptcp_stop_timer(sk);

@Mat: I'm wild guessing the above could possibly address also
issues/230. Could you please give it a spin in the CI? How frequently
do you observe the mentioned issue in the CI?

> } else {
> @@ -1547,6 +1548,13 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
>  		msk->snd_nxt = snd_nxt_new;
>  }
>  
> +static void mptcp_check_and_set_pending(struct sock *sk)
> +{
> +	if (mptcp_send_head(sk) &&
> +	    !test_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags))
> +		set_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags);
> +}
> +
>  void __mptcp_push_pending(struct sock *sk, unsigned int flags)
>  {
>  	struct sock *prev_ssk = NULL, *ssk = NULL;
> @@ -1603,6 +1611,13 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
>  		mptcp_push_release(sk, ssk, &info);
>  
>  out:
> +	/* Set PENDING again in case we found an ssk
> +	 * that could not accept more data
> +	 */
> +	if (unlikely(copied == 0) &&
> +	    READ_ONCE(msk->snd_una) == msk->snd_nxt && ssk)
> +		mptcp_check_and_set_pending(sk);

@Florian: Why we need to check for ssk != NULL?
Do we need something similar for __mptcp_subflow_push_pending(), too? 
I'm wondering if this will cause a sort of 'spinning'/busy loop on
above condition inside mptcp_release_cb(). Could we just ensure that
the rtx timer is harmed instead? We likely have to wait a bit for the
subflow to become ready, I think.

Cheers,

Paolo

Re: [mptcp-next 2/2] mptcp: re-set push-pending bit on retransmit failure

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> > +	/* Set PENDING again in case we found an ssk
> > +	 * that could not accept more data
> > +	 */
> > +	if (unlikely(copied == 0) &&
> > +	    READ_ONCE(msk->snd_una) == msk->snd_nxt && ssk)
> > +		mptcp_check_and_set_pending(sk);
> 
> @Florian: Why we need to check for ssk != NULL?

Because I don't think it makes sense to re-try ASAP if we could
not find a ssk, and instead defer until next firing of rtx timer.

> Do we need something similar for __mptcp_subflow_push_pending(), too?

I don't think so, its thats not invoked from the mptcp-release cb; also:

> I'm wondering if this will cause a sort of 'spinning'/busy loop on
> above condition inside mptcp_release_cb().

Hmm, right, that might indeed happen.
It should be sae to remove the above clause and unconditioally wait
for the reworked rtx timer to assert pending bit again.

Re: [mptcp-next 2/2] mptcp: re-set push-pending bit on retransmit failure

[Paolo Abeni]

On Mon, 2021-09-06 at 09:55 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > > +	/* Set PENDING again in case we found an ssk
> > > +	 * that could not accept more data
> > > +	 */
> > > +	if (unlikely(copied == 0) &&
> > > +	    READ_ONCE(msk->snd_una) == msk->snd_nxt && ssk)
> > > +		mptcp_check_and_set_pending(sk);
> > 
> > @Florian: Why we need to check for ssk != NULL?
> 
> Because I don't think it makes sense to re-try ASAP if we could
> not find a ssk, and instead defer until next firing of rtx timer.

Ok
> 
> > Do we need something similar for __mptcp_subflow_push_pending(), too?
> 
> I don't think so, its thats not invoked from the mptcp-release cb; 

yup, right. To be clear, with "something similar" I also referred to
ensuring the rtx timer is armed.

> also:
> 
> > I'm wondering if this will cause a sort of 'spinning'/busy loop on
> > above condition inside mptcp_release_cb().
> 
> Hmm, right, that might indeed happen.
> It should be sae to remove the above clause and unconditioally wait
> for the reworked rtx timer to assert pending bit again.

+1 :) (assuming s/sae/safe/ :)

Thanks!

Paolo

Re: [mptcp-next 2/2] mptcp: re-set push-pending bit on retransmit failure

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> On Mon, 2021-09-06 at 09:55 +0200, Florian Westphal wrote:
> > Because I don't think it makes sense to re-try ASAP if we could
> > not find a ssk, and instead defer until next firing of rtx timer.
> 
> Ok
> > 
> > > Do we need something similar for __mptcp_subflow_push_pending(), too?
> > 
> > I don't think so, its thats not invoked from the mptcp-release cb; 
> 
> yup, right. To be clear, with "something similar" I also referred to
> ensuring the rtx timer is armed.

__mptcp_subflow_push_pending() is only called when we already have
pending xmit, i.e. the timer is already running and with the change in
this patch the timer is guaranteed to re-arm itself unless there is
nothing to transmit anymore.

__mptcp_push_pending is different because that is called when new data
is passed in via sendmsg, i.e. timer might not be running (yet).

Makes sense to you?

> > > I'm wondering if this will cause a sort of 'spinning'/busy loop on
> > > above condition inside mptcp_release_cb().
> > 
> > Hmm, right, that might indeed happen.
> > It should be sae to remove the above clause and unconditioally wait
> > for the reworked rtx timer to assert pending bit again.
> 
> +1 :) (assuming s/sae/safe/ :)

Yep, safe.  Test has been running for a few hours now with no failures,
will send v2 shortly.

Re: [mptcp-next 2/2] mptcp: re-set push-pending bit on retransmit failure

[Paolo Abeni]

On Mon, 2021-09-06 at 13:35 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > On Mon, 2021-09-06 at 09:55 +0200, Florian Westphal wrote:
> > > Because I don't think it makes sense to re-try ASAP if we could
> > > not find a ssk, and instead defer until next firing of rtx timer.
> > 
> > Ok
> > > > Do we need something similar for __mptcp_subflow_push_pending(), too?
> > > 
> > > I don't think so, its thats not invoked from the mptcp-release cb; 
> > 
> > yup, right. To be clear, with "something similar" I also referred to
> > ensuring the rtx timer is armed.
> 
> __mptcp_subflow_push_pending() is only called when we already have
> pending xmit, i.e. the timer is already running and with the change in
> this patch the timer is guaranteed to re-arm itself unless there is
> nothing to transmit anymore.
> 
> __mptcp_push_pending is different because that is called when new data
> is passed in via sendmsg, i.e. timer might not be running (yet).
> 
> Makes sense to you?

Yes, thanks for the explaination!

> > > > I'm wondering if this will cause a sort of 'spinning'/busy loop on
> > > > above condition inside mptcp_release_cb().
> > > 
> > > Hmm, right, that might indeed happen.
> > > It should be sae to remove the above clause and unconditioally wait
> > > for the reworked rtx timer to assert pending bit again.
> > 
> > +1 :) (assuming s/sae/safe/ :)
> 
> Yep, safe.  Test has been running for a few hours now with no failures,
> will send v2 shortly.

Excellent!

Cheeers,

Paolo