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

[Paolo Abeni <pabeni@redhat.com>]

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