[PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When/if sysctl_tcp_abc > 1, we expect to increase cwnd by 2 if the
received ACK acknowledges more than 2*MSS bytes, in tcp_slow_start()

Problem is this RFC 3465 statement is not correctly coded, as
the while () loop increases snd_cwnd one by one.

Add a new variable to avoid this off-by one error.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Nandita Dukkipati <nanditad@google.com>
Cc: John Heffner <johnwheffner@gmail.com>
Cc: Stephen Hemminger <shemminger@vyatta.com>
---
v2: added John suggestion

 net/ipv4/tcp_cong.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index 04dbd7a..4d4db16 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -307,6 +307,7 @@ EXPORT_SYMBOL_GPL(tcp_is_cwnd_limited);
 void tcp_slow_start(struct tcp_sock *tp)
 {
 	int cnt; /* increase in packets */
+	unsigned int delta = 0;
 
 	/* RFC3465: ABC Slow start
 	 * Increase only after a full MSS of bytes is acked
@@ -333,9 +334,9 @@ void tcp_slow_start(struct tcp_sock *tp)
 	tp->snd_cwnd_cnt += cnt;
 	while (tp->snd_cwnd_cnt >= tp->snd_cwnd) {
 		tp->snd_cwnd_cnt -= tp->snd_cwnd;
-		if (tp->snd_cwnd < tp->snd_cwnd_clamp)
-			tp->snd_cwnd++;
+		delta++;
 	}
+	tp->snd_cwnd = min(tp->snd_cwnd + delta, tp->snd_cwnd_clamp);
 }
 EXPORT_SYMBOL_GPL(tcp_slow_start);
 

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Yuchung Cheng]

On Fri, Jul 20, 2012 at 8:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> When/if sysctl_tcp_abc > 1, we expect to increase cwnd by 2 if the
> received ACK acknowledges more than 2*MSS bytes, in tcp_slow_start()
>
> Problem is this RFC 3465 statement is not correctly coded, as
> the while () loop increases snd_cwnd one by one.
>
> Add a new variable to avoid this off-by one error.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Nandita Dukkipati <nanditad@google.com>
> Cc: John Heffner <johnwheffner@gmail.com>
> Cc: Stephen Hemminger <shemminger@vyatta.com>
> ---
> v2: added John suggestion
>
>  net/ipv4/tcp_cong.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> index 04dbd7a..4d4db16 100644
> --- a/net/ipv4/tcp_cong.c
> +++ b/net/ipv4/tcp_cong.c
> @@ -307,6 +307,7 @@ EXPORT_SYMBOL_GPL(tcp_is_cwnd_limited);
>  void tcp_slow_start(struct tcp_sock *tp)
>  {
>         int cnt; /* increase in packets */
> +       unsigned int delta = 0;
>
>         /* RFC3465: ABC Slow start
>          * Increase only after a full MSS of bytes is acked
> @@ -333,9 +334,9 @@ void tcp_slow_start(struct tcp_sock *tp)
>         tp->snd_cwnd_cnt += cnt;
>         while (tp->snd_cwnd_cnt >= tp->snd_cwnd) {
>                 tp->snd_cwnd_cnt -= tp->snd_cwnd;
> -               if (tp->snd_cwnd < tp->snd_cwnd_clamp)
> -                       tp->snd_cwnd++;
> +               delta++;
Nice! this also removes wasteful iteration when clamp << cwnd_cnt.
>         }
> +       tp->snd_cwnd = min(tp->snd_cwnd + delta, tp->snd_cwnd_clamp);
>  }
>  EXPORT_SYMBOL_GPL(tcp_slow_start);
>
>
>

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Neal Cardwell]

On Fri, Jul 20, 2012 at 8:07 AM, Yuchung Cheng <ycheng@google.com> wrote:
> On Fri, Jul 20, 2012 at 8:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>         tp->snd_cwnd_cnt += cnt;
>>         while (tp->snd_cwnd_cnt >= tp->snd_cwnd) {

Nice catch, Eric.

One thing that's always bothered me about the tp->snd_cwnd_cnt code is
that the slow start and congestion avoidance use different criteria
for incrementing snd_cwnd_cnt. tcp_slow_start() increments
snd_cwnd_cnt by snd_cwnd for each ACKed packet, and congestion
avoidance increases snd_cwnd_cnt by just 1 for each packet.

This means that if we exit slow start and enter congestion avoidance,
then we think we can have a "credit" for a bunch of ACKs that never
happened (up to snd_cwnd-1), so we can conceivably do our first
additive increase in congestion avoidance up to almost 1RTT too
early. Can we just get rid of the use of snd_cwnd_cnt in slow start,
and just use local variables in tcp_slow_start() rather than trying to
carry state between ACKs?

neal

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Eric Dumazet]

On Fri, 2012-07-20 at 09:03 -0700, Neal Cardwell wrote:
> On Fri, Jul 20, 2012 at 8:07 AM, Yuchung Cheng <ycheng@google.com> wrote:
> > On Fri, Jul 20, 2012 at 8:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>         tp->snd_cwnd_cnt += cnt;
> >>         while (tp->snd_cwnd_cnt >= tp->snd_cwnd) {
> 
> Nice catch, Eric.
> 
> One thing that's always bothered me about the tp->snd_cwnd_cnt code is
> that the slow start and congestion avoidance use different criteria
> for incrementing snd_cwnd_cnt. tcp_slow_start() increments
> snd_cwnd_cnt by snd_cwnd for each ACKed packet, and congestion
> avoidance increases snd_cwnd_cnt by just 1 for each packet.
> 
> This means that if we exit slow start and enter congestion avoidance,
> then we think we can have a "credit" for a bunch of ACKs that never
> happened (up to snd_cwnd-1), so we can conceivably do our first
> additive increase in congestion avoidance up to almost 1RTT too
> early. Can we just get rid of the use of snd_cwnd_cnt in slow start,
> and just use local variables in tcp_slow_start() rather than trying to
> carry state between ACKs?

Apparently tcp_slow_start() needs the snd_cwnd_cnt in case 
"limited slow start"  is used :

cnt = sysctl_tcp_max_ssthresh >> 1;

So to address your point, maybe we should clear  snd_cwnd_cnt
when leaving slow start for congestion avoidance phase ?

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Neal Cardwell]

On Fri, Jul 20, 2012 at 9:08 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> So to address your point, maybe we should clear  snd_cwnd_cnt
> when leaving slow start for congestion avoidance phase ?

Sounds good. That can be a separate commit to add the new logic to the
end of tcp_slow_start() to check to see if we've bumped into ssthresh
and reset snd_cwnd_cnt.

neal

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Neal Cardwell]

On Fri, Jul 20, 2012 at 8:07 AM, Yuchung Cheng <ycheng@google.com> wrote:
> On Fri, Jul 20, 2012 at 8:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> When/if sysctl_tcp_abc > 1, we expect to increase cwnd by 2 if the
>> received ACK acknowledges more than 2*MSS bytes, in tcp_slow_start()
>>
>> Problem is this RFC 3465 statement is not correctly coded, as
>> the while () loop increases snd_cwnd one by one.
>>
>> Add a new variable to avoid this off-by one error.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>

Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[David Miller]

From: Neal Cardwell <ncardwell@google.com>
Date: Fri, 20 Jul 2012 10:58:27 -0700

> On Fri, Jul 20, 2012 at 8:07 AM, Yuchung Cheng <ycheng@google.com> wrote:
>> On Fri, Jul 20, 2012 at 8:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> From: Eric Dumazet <edumazet@google.com>
>>>
>>> When/if sysctl_tcp_abc > 1, we expect to increase cwnd by 2 if the
>>> received ACK acknowledges more than 2*MSS bytes, in tcp_slow_start()
>>>
>>> Problem is this RFC 3465 statement is not correctly coded, as
>>> the while () loop increases snd_cwnd one by one.
>>>
>>> Add a new variable to avoid this off-by one error.
>>>
>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Acked-by: Yuchung Cheng <ycheng@google.com>
> 
> Acked-by: Neal Cardwell <ncardwell@google.com>

Applied.

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Stephen Hemminger]

TCP ABC was an experiment that failed. It solves a problem that does not exist on Linux. I did the implementation mostly to prove that.

Perhaps it abc should just be removed?

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Fri, 20 Jul 2012 11:01:43 -0700

> TCP ABC was an experiment that failed. It solves a problem that does
> not exist on Linux. I did the implementation mostly to prove that.
> 
> Perhaps it abc should just be removed?

This is my impression as well.

Re: [PATCH v2 net-next] tcp: fix ABC in tcp_slow_start()

[Neal Cardwell]

On Fri, Jul 20, 2012 at 11:02 AM, David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Fri, 20 Jul 2012 11:01:43 -0700
>
>> TCP ABC was an experiment that failed. It solves a problem that does
>> not exist on Linux. I did the implementation mostly to prove that.
>>
>> Perhaps it abc should just be removed?
>
> This is my impression as well.

Removing ABC sounds good to me as well.

neal