[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 <ycheng@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Nandita Dukkipati <nanditad@google.com>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Fri, Feb 20, 2015 at 10:33 AM, Neal Cardwell <ncardwell@google.com> 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.

> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Nandita Dukkipati <nanditad@google.com>
> ---
>  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
>
> --
> 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

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 <ycheng@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Nandita Dukkipati <nanditad@google.com>

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

Thanks Neal.
--
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