[PATCH net-next] tcp: fix tcp_should_expand_sndbuf() to use tcp_packets_in_flight()

tcp_should_expand_sndbuf() does not expand the send buffer if we have
filled the congestion window.

However, it should use tcp_packets_in_flight() instead of
tp->packets_out to make this check.

Testing has established that the difference matters a lot if there are
many SACKed packets, causing a needless performance shortfall.

Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Yuchung Cheng <redacted>
Signed-off-by: Eric Dumazet <redacted>
Signed-off-by: Nandita Dukkipati <redacted>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 8fdd27b..fb4cf8b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -4770,7 +4770,7 @@ static bool tcp_should_expand_sndbuf(const struct sock *sk)
 		return false;
 
 	/* If we filled the congestion window, do not expand.  */
-	if (tp->packets_out >= tp->snd_cwnd)
+	if (tcp_packets_in_flight(tp) >= tp->snd_cwnd)
 		return false;
 
 	return true;

-- 
2.2.0.rc0.207.ga3a616c

On Fri, Feb 20, 2015 at 10:33 AM, Neal Cardwell [off-list ref] wrote:

tcp_should_expand_sndbuf() does not expand the send buffer if we have
filled the congestion window.

However, it should use tcp_packets_in_flight() instead of
tp->packets_out to make this check.

oy. is this a candidate for -stable?

Testing has established that the difference matters a lot if there are
many SACKed packets, causing a needless performance shortfall.

One of the things I have been meaning to do for a while is go back to
testing some of the original circumstances of bufferbloat,
to see to what extent all the enormous changes to linux tcp itself
since 2010 have made a difference. I vividly remember the sort of
dup ack/sack stuff, and tcp cubic losing it´s mind, we saw back in the
day on the simple tests here:

https://gettys.wordpress.com/2010/12/06/whose-house-is-of-glasse-must-not-throw-stones-at-another/

Running without fq_codel in place on my gateways is not something that
I do willingly. I am sad to report that the latest comcast services
running at nearly 200mbit down, still exhibit 1.2 seconds of latency
under load on the rrul test, *but* my primary targets for those sorts
of tests in the cloud have not kept up with the latest linuxes.
(hoping to deploy new stuff soon)

I had thought this guy´s dissertation on BBR was pretty promising, also:

http://cpsc.yale.edu/event/dissertation-defensemichael-nowlan

and keep wondering if that code has been more thoroughly evaluated and
tested somewhere.

-- 
Dave Täht

thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks

From: Neal Cardwell <ncardwell@google.com>
Date: Fri, 20 Feb 2015 13:33:16 -0500

tcp_should_expand_sndbuf() does not expand the send buffer if we have
filled the congestion window.

However, it should use tcp_packets_in_flight() instead of
tp->packets_out to make this check.

Testing has established that the difference matters a lot if there are
many SACKed packets, causing a needless performance shortfall.

Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Yuchung Cheng <redacted>
Signed-off-by: Eric Dumazet <redacted>
Signed-off-by: Nandita Dukkipati <redacted>

This is a bug in my view, thus applying to 'net'.

Thanks Neal.