Patchwork tcp: frto should not set snd_cwnd to 0

login
register
mail settings
Submitter Ilpo Järvinen
Date Feb. 4, 2013, 12:14 p.m.
Message ID <alpine.DEB.2.00.1302041327210.4561@wel-95.cs.helsinki.fi>
Download mbox | patch
Permalink /patch/217890/
State Accepted
Delegated to: David Miller
Headers show

Comments

Ilpo Järvinen - Feb. 4, 2013, 12:14 p.m.
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(-)
Eric Dumazet - Feb. 4, 2013, 3:07 p.m.
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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Feb. 5, 2013, 7:44 p.m.
On Mon, 2013-02-04 at 07:07 -0800, Eric Dumazet wrote:

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

Sorry for the delay.

The patch survived my tests ;)

Tested-by: Eric Dumazet <edumazet@google.com>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Neal Cardwell - Feb. 5, 2013, 7:49 p.m.
On Mon, Feb 4, 2013 at 7:14 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
> [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));
> --

Acked-by: Neal Cardwell <ncardwell@google.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 6, 2013, 8:55 p.m.
From: Neal Cardwell <ncardwell@google.com>
Date: Tue, 5 Feb 2013 14:49:04 -0500

> On Mon, Feb 4, 2013 at 7:14 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
>> [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>
 ...
> Acked-by: Neal Cardwell <ncardwell@google.com>

Applied, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pasi Kärkkäinen - Feb. 6, 2013, 9:13 p.m.
On Wed, Feb 06, 2013 at 03:55:04PM -0500, David Miller wrote:
> From: Neal Cardwell <ncardwell@google.com>
> Date: Tue, 5 Feb 2013 14:49:04 -0500
> 
> > On Mon, Feb 4, 2013 at 7:14 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
> >> [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>
>  ...
> > Acked-by: Neal Cardwell <ncardwell@google.com>
> 
> Applied, thanks everyone.
>

Hmm.. are we missing CC stable@kernel.org in these patches? 
I guess 3.6.x is already EOL, but it'd be nice to get this bug fixed also in 3.7.x ..

Thanks,

-- Pasi

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 6, 2013, 9:19 p.m.
From: Pasi Kärkkäinen <pasik@iki.fi>
Date: Wed, 6 Feb 2013 23:13:43 +0200

> On Wed, Feb 06, 2013 at 03:55:04PM -0500, David Miller wrote:
>> From: Neal Cardwell <ncardwell@google.com>
>> Date: Tue, 5 Feb 2013 14:49:04 -0500
>> 
>> > On Mon, Feb 4, 2013 at 7:14 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
>> >> [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>
>>  ...
>> > Acked-by: Neal Cardwell <ncardwell@google.com>
>> 
>> Applied, thanks everyone.
>>
> 
> Hmm.. are we missing CC stable@kernel.org in these patches? 
> I guess 3.6.x is already EOL, but it'd be nice to get this bug fixed also in 3.7.x ..

We never CC: stable on networking patches, I queue them up manually
and submit them at a time of my own choosing.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pasi Kärkkäinen - Feb. 6, 2013, 9:21 p.m.
On Wed, Feb 06, 2013 at 04:19:19PM -0500, David Miller wrote:
> From: Pasi Kärkkäinen <pasik@iki.fi>
> Date: Wed, 6 Feb 2013 23:13:43 +0200
> 
> > On Wed, Feb 06, 2013 at 03:55:04PM -0500, David Miller wrote:
> >> From: Neal Cardwell <ncardwell@google.com>
> >> Date: Tue, 5 Feb 2013 14:49:04 -0500
> >> 
> >> > On Mon, Feb 4, 2013 at 7:14 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
> >> >> [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>
> >>  ...
> >> > Acked-by: Neal Cardwell <ncardwell@google.com>
> >> 
> >> Applied, thanks everyone.
> >>
> > 
> > Hmm.. are we missing CC stable@kernel.org in these patches? 
> > I guess 3.6.x is already EOL, but it'd be nice to get this bug fixed also in 3.7.x ..
> 
> We never CC: stable on networking patches, I queue them up manually
> and submit them at a time of my own choosing.
>

OK, I didn't know that. Thanks :)

-- Pasi

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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));