[PATCH net 1/2] tcp: fix tcp_cong_avoid_ai() credit accumulation bug with decreases in w

[PATCH net 1/2] tcp: fix tcp_cong_avoid_ai() credit accumulation bug with decreases in w

[Neal Cardwell]

The recent change to tcp_cong_avoid_ai() to handle stretch ACKs
introduced a bug where snd_cwnd_cnt could accumulate a very large
value while w was large, and then if w was reduced snd_cwnd could be
incremented by a large delta, leading to a large burst and high packet
loss. This was tickled when CUBIC's bictcp_update() sets "ca->cnt =
100 * cwnd".

This bug crept in while preparing the upstream version of
814d488c6126.

Testing: This patch has been tested in datacenter netperf transfers
and live youtube.com and google.com servers.

Fixes: 814d488c6126 ("tcp: fix the timid additive increase on stretch ACKs")
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_cong.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index d694088..62856e1 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -378,6 +378,12 @@ EXPORT_SYMBOL_GPL(tcp_slow_start);
  */
 void tcp_cong_avoid_ai(struct tcp_sock *tp, u32 w, u32 acked)
 {
+	/* If credits accumulated at a higher w, apply them gently now. */
+	if (tp->snd_cwnd_cnt >= w) {
+		tp->snd_cwnd_cnt = 0;
+		tp->snd_cwnd++;
+	}
+
 	tp->snd_cwnd_cnt += acked;
 	if (tp->snd_cwnd_cnt >= w) {
 		u32 delta = tp->snd_cwnd_cnt / w;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH net 2/2] tcp: restore 1.5x per RTT limit to CUBIC cwnd growth in congestion avoidance

[Neal Cardwell]

Commit 814d488c6126 ("tcp: fix the timid additive increase on stretch
ACKs") fixed a bug where tcp_cong_avoid_ai() would either credit a
connection with an increase of snd_cwnd_cnt, or increase snd_cwnd, but
not both, resulting in cwnd increasing by 1 packet on at most every
alternate invocation of tcp_cong_avoid_ai().

Although the commit correctly implemented the CUBIC algorithm, which
can increase cwnd by as much as 1 packet per 1 packet ACKed (2x per
RTT), in practice that could be too aggressive: in tests on network
paths with small buffers, YouTube server retransmission rates nearly
doubled.

This commit restores CUBIC to a maximum cwnd growth rate of 1 packet
per 2 packets ACKed (1.5x per RTT). In YouTube tests this restored
retransmit rates to low levels.

Testing: This patch has been tested in datacenter netperf transfers
and live youtube.com and google.com servers.

Fixes: 9cd981dcf174 ("tcp: fix stretch ACK bugs in CUBIC")
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_cubic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index 4b276d1..06d3d66 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -306,8 +306,10 @@ tcp_friendliness:
 		}
 	}
 
-	if (ca->cnt == 0)			/* cannot be zero */
-		ca->cnt = 1;
+	/* The maximum rate of cwnd increase CUBIC allows is 1 packet per
+	 * 2 packets ACKed, meaning cwnd grows at 1.5x per RTT.
+	 */
+	ca->cnt = max(ca->cnt, 2U);
 }
 
 static void bictcp_cong_avoid(struct sock *sk, u32 ack, u32 acked)
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net 1/2] tcp: fix tcp_cong_avoid_ai() credit accumulation bug with decreases in w

[David Miller]

From: Neal Cardwell <ncardwell@google.com>
Date: Tue, 10 Mar 2015 17:17:03 -0400

> The recent change to tcp_cong_avoid_ai() to handle stretch ACKs
> introduced a bug where snd_cwnd_cnt could accumulate a very large
> value while w was large, and then if w was reduced snd_cwnd could be
> incremented by a large delta, leading to a large burst and high packet
> loss. This was tickled when CUBIC's bictcp_update() sets "ca->cnt =
> 100 * cwnd".
> 
> This bug crept in while preparing the upstream version of
> 814d488c6126.
> 
> Testing: This patch has been tested in datacenter netperf transfers
> and live youtube.com and google.com servers.
> 
> Fixes: 814d488c6126 ("tcp: fix the timid additive increase on stretch ACKs")
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

Re: [PATCH net 2/2] tcp: restore 1.5x per RTT limit to CUBIC cwnd growth in congestion avoidance

[David Miller]

From: Neal Cardwell <ncardwell@google.com>
Date: Tue, 10 Mar 2015 17:17:04 -0400

> Commit 814d488c6126 ("tcp: fix the timid additive increase on stretch
> ACKs") fixed a bug where tcp_cong_avoid_ai() would either credit a
> connection with an increase of snd_cwnd_cnt, or increase snd_cwnd, but
> not both, resulting in cwnd increasing by 1 packet on at most every
> alternate invocation of tcp_cong_avoid_ai().
> 
> Although the commit correctly implemented the CUBIC algorithm, which
> can increase cwnd by as much as 1 packet per 1 packet ACKed (2x per
> RTT), in practice that could be too aggressive: in tests on network
> paths with small buffers, YouTube server retransmission rates nearly
> doubled.
> 
> This commit restores CUBIC to a maximum cwnd growth rate of 1 packet
> per 2 packets ACKed (1.5x per RTT). In YouTube tests this restored
> retransmit rates to low levels.
> 
> Testing: This patch has been tested in datacenter netperf transfers
> and live youtube.com and google.com servers.
> 
> Fixes: 9cd981dcf174 ("tcp: fix stretch ACK bugs in CUBIC")
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

Re: [PATCH net 2/2] tcp: restore 1.5x per RTT limit to CUBIC cwnd growth in congestion avoidance

[Neal Cardwell]

On Wed, Mar 11, 2015 at 4:52 PM, David Miller <davem@davemloft.net> wrote:
> From: Neal Cardwell <ncardwell@google.com>
> Date: Tue, 10 Mar 2015 17:17:04 -0400
>
>> Commit 814d488c6126 ("tcp: fix the timid additive increase on stretch
>> ACKs") fixed a bug where tcp_cong_avoid_ai() would either credit a
>> connection with an increase of snd_cwnd_cnt, or increase snd_cwnd, but
>> not both, resulting in cwnd increasing by 1 packet on at most every
>> alternate invocation of tcp_cong_avoid_ai().
>>
>> Although the commit correctly implemented the CUBIC algorithm, which
>> can increase cwnd by as much as 1 packet per 1 packet ACKed (2x per
>> RTT), in practice that could be too aggressive: in tests on network
>> paths with small buffers, YouTube server retransmission rates nearly
>> doubled.
>>
>> This commit restores CUBIC to a maximum cwnd growth rate of 1 packet
>> per 2 packets ACKed (1.5x per RTT). In YouTube tests this restored
>> retransmit rates to low levels.
>>
>> Testing: This patch has been tested in datacenter netperf transfers
>> and live youtube.com and google.com servers.
>>
>> Fixes: 9cd981dcf174 ("tcp: fix stretch ACK bugs in CUBIC")
>> Signed-off-by: Neal Cardwell <ncardwell@google.com>
>> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Applied.

Thanks!

My sense is that both of these fixes are appropriate for the stable
queue, since they fix serious packet loss issues that folks running
3.19.x will run into:

d578e18 tcp: restore 1.5x per RTT limit to CUBIC cwnd growth in
congestion avoidance
9949afa tcp: fix tcp_cong_avoid_ai() credit accumulation bug with decreases in w

neal

Re: [PATCH net 2/2] tcp: restore 1.5x per RTT limit to CUBIC cwnd growth in congestion avoidance

[David Miller]

From: Neal Cardwell <ncardwell@google.com>
Date: Wed, 11 Mar 2015 18:10:42 -0400

> My sense is that both of these fixes are appropriate for the stable
> queue, since they fix serious packet loss issues that folks running
> 3.19.x will run into:

Ok, queued up, thanks.