Message ID | 1424457196-3497-1-git-send-email-ncardwell@google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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
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;