Re: [PATCH] tcp: frto should not set snd_cwnd to 0

[Eric Dumazet <eric.dumazet@gmail.com>]

On Mon, 2013-02-04 at 14:14 +0200, Ilpo Järvinen wrote:
> On Sun, 3 Feb 2013, Eric Dumazet wrote:
> 
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Commit 9dc274151a548 (tcp: fix ABC in tcp_slow_start())
> > uncovered a bug in FRTO code :
> > tcp_process_frto() is setting snd_cwnd to 0 if the number
> > of in flight packets is 0.
> > 
> > As Neal pointed out, if no packet is in flight we lost our
> > chance to disambiguate whether a loss timeout was spurious.
> > 
> > We should assume it was a proper loss.
> > 
> > Reported-by: Pasi Kärkkäinen <pasik@iki.fi>
> > Signed-off-by: Neal Cardwell <ncardwell@google.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> > Cc: Yuchung Cheng <ycheng@google.com>
> > ---
> >  net/ipv4/tcp_input.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 8aca4ee..680c422 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -3484,7 +3484,8 @@ static bool tcp_process_frto(struct sock *sk, int flag)
> >  	    ((tp->frto_counter >= 2) && (flag & FLAG_RETRANS_DATA_ACKED)))
> >  		tp->undo_marker = 0;
> >  
> > -	if (!before(tp->snd_una, tp->frto_highmark)) {
> > +	if (!before(tp->snd_una, tp->frto_highmark) ||
> > +	    !tcp_packets_in_flight(tp)) {
> 
> I think this condition becomes now too broad because there is transient
> during FRTO. I think the patch below would be enough to resolve this,
> what do you think?
> 
> --
> [PATCH 1/1] tcp: fix for zero packets_in_flight was too broad
> 
> There are transients during normal FRTO procedure during which
> the packets_in_flight can go to zero between write_queue state
> updates and firing the resulting segments out. As FRTO processing
> occurs during that window the check must be more precise to
> not match "spuriously" :-). More specificly, e.g., when
> packets_in_flight is zero but FLAG_DATA_ACKED is true the problematic
> branch that set cwnd into zero would not be taken and new segments
> might be sent out later.
> 
> Only compile tested.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Pasi Kärkkäinen <pasik@iki.fi>
> ---
>  net/ipv4/tcp_input.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 680c422..500c2da 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3484,8 +3484,7 @@ static bool tcp_process_frto(struct sock *sk, int flag)
>  	    ((tp->frto_counter >= 2) && (flag & FLAG_RETRANS_DATA_ACKED)))
>  		tp->undo_marker = 0;
>  
> -	if (!before(tp->snd_una, tp->frto_highmark) ||
> -	    !tcp_packets_in_flight(tp)) {
> +	if (!before(tp->snd_una, tp->frto_highmark)) {
>  		tcp_enter_frto_loss(sk, (tp->frto_counter == 1 ? 2 : 3), flag);
>  		return true;
>  	}
> @@ -3505,6 +3504,11 @@ static bool tcp_process_frto(struct sock *sk, int flag)
>  		}
>  	} else {
>  		if (!(flag & FLAG_DATA_ACKED) && (tp->frto_counter == 1)) {
> +			if (!tcp_packets_in_flight(tp)) {
> +				tcp_enter_frto_loss(sk, 2, flag);
> +				return true;
> +			}
> +				
>  			/* Prevent sending of new data. */
>  			tp->snd_cwnd = min(tp->snd_cwnd,
>  					   tcp_packets_in_flight(tp));

Thanks Ilpo.

I'll be able to test your patch under load only in ~8 hours.