[net,1/5] tcp_bbr: cut pacing rate only if filled pipe

Message ID 20170714214925.30720-1-ncardwell@google.com
State Accepted
Delegated to: David Miller
Headers show

Commit Message

Neal Cardwell July 14, 2017, 9:49 p.m.
In bbr_set_pacing_rate(), which decides whether to cut the pacing
rate, there was some code that considered exiting STARTUP to be
equivalent to the notion of filling the pipe (i.e.,
bbr_full_bw_reached()). Specifically, as the code was structured,
exiting STARTUP and going into PROBE_RTT could cause us to cut the
pacing rate down to something silly and low, based on whatever
bandwidth samples we've had so far, when it's possible that all of
them have been small app-limited bandwidth samples that are not
representative of the bandwidth available in the path. (The code was
correct at the time it was written, but the state machine changed
without this spot being adjusted correspondingly.)

Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
---
 net/ipv4/tcp_bbr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Stephen Hemminger July 14, 2017, 10:36 p.m. | #1
On Fri, 14 Jul 2017 17:49:21 -0400
Neal Cardwell <ncardwell@google.com> wrote:

> In bbr_set_pacing_rate(), which decides whether to cut the pacing
> rate, there was some code that considered exiting STARTUP to be
> equivalent to the notion of filling the pipe (i.e.,
> bbr_full_bw_reached()). Specifically, as the code was structured,
> exiting STARTUP and going into PROBE_RTT could cause us to cut the
> pacing rate down to something silly and low, based on whatever
> bandwidth samples we've had so far, when it's possible that all of
> them have been small app-limited bandwidth samples that are not
> representative of the bandwidth available in the path. (The code was
> correct at the time it was written, but the state machine changed
> without this spot being adjusted correspondingly.)
> 
> Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>

Looks good, but net-next is closed at this time. Please resubmit later.

http://vger.kernel.org/~davem/net-next.html
Neal Cardwell July 14, 2017, 10:54 p.m. | #2
On Fri, Jul 14, 2017 at 6:36 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Fri, 14 Jul 2017 17:49:21 -0400
> Neal Cardwell <ncardwell@google.com> wrote:
>
>> In bbr_set_pacing_rate(), which decides whether to cut the pacing
>> rate, there was some code that considered exiting STARTUP to be
>> equivalent to the notion of filling the pipe (i.e.,
>> bbr_full_bw_reached()). Specifically, as the code was structured,
>> exiting STARTUP and going into PROBE_RTT could cause us to cut the
>> pacing rate down to something silly and low, based on whatever
>> bandwidth samples we've had so far, when it's possible that all of
>> them have been small app-limited bandwidth samples that are not
>> representative of the bandwidth available in the path. (The code was
>> correct at the time it was written, but the state machine changed
>> without this spot being adjusted correspondingly.)
>>
>> Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
>> Signed-off-by: Neal Cardwell <ncardwell@google.com>
>> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
>
> Looks good, but net-next is closed at this time. Please resubmit later.
>
> http://vger.kernel.org/~davem/net-next.html

Thanks, Stephen. We see your point on the net-next patch "tcp: adjust
tail loss probe timeout"; we'll resubmit that patch when net-next
opens. Sorry about that!

But for the tcp_bbr patch series, those are bug fixes, and we were
marking them as being for "net" with Fixes: footers in the hopes that
they could go into the "net" branch and be queued up for inclusion in
-stable releases. Are you saying that in your estimation the substance
of the fixes doesn't rise to the level of "net" material? If that is
the consensus then we can resubmit for net-next when that opens.

thanks,
neal
Stephen Hemminger July 14, 2017, 11:15 p.m. | #3
On Fri, 14 Jul 2017 18:54:02 -0400
Neal Cardwell <ncardwell@google.com> wrote:

> On Fri, Jul 14, 2017 at 6:36 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> > On Fri, 14 Jul 2017 17:49:21 -0400
> > Neal Cardwell <ncardwell@google.com> wrote:
> >  
> >> In bbr_set_pacing_rate(), which decides whether to cut the pacing
> >> rate, there was some code that considered exiting STARTUP to be
> >> equivalent to the notion of filling the pipe (i.e.,
> >> bbr_full_bw_reached()). Specifically, as the code was structured,
> >> exiting STARTUP and going into PROBE_RTT could cause us to cut the
> >> pacing rate down to something silly and low, based on whatever
> >> bandwidth samples we've had so far, when it's possible that all of
> >> them have been small app-limited bandwidth samples that are not
> >> representative of the bandwidth available in the path. (The code was
> >> correct at the time it was written, but the state machine changed
> >> without this spot being adjusted correspondingly.)
> >>
> >> Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
> >> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> >> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> >> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>  
> >

You are correct, these look more like bug fixes. I was a little concerned
that the changes would be visible but they really aren't user visible.

Should they go to stable as well?
Neal Cardwell July 15, 2017, 12:29 a.m. | #4
On Fri, Jul 14, 2017 at 7:15 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Fri, 14 Jul 2017 18:54:02 -0400
> Neal Cardwell <ncardwell@google.com> wrote:
>
>> On Fri, Jul 14, 2017 at 6:36 PM, Stephen Hemminger
>> <stephen@networkplumber.org> wrote:
>> > On Fri, 14 Jul 2017 17:49:21 -0400
>> > Neal Cardwell <ncardwell@google.com> wrote:
>> >
>> >> In bbr_set_pacing_rate(), which decides whether to cut the pacing
>> >> rate, there was some code that considered exiting STARTUP to be
>> >> equivalent to the notion of filling the pipe (i.e.,
>> >> bbr_full_bw_reached()). Specifically, as the code was structured,
>> >> exiting STARTUP and going into PROBE_RTT could cause us to cut the
>> >> pacing rate down to something silly and low, based on whatever
>> >> bandwidth samples we've had so far, when it's possible that all of
>> >> them have been small app-limited bandwidth samples that are not
>> >> representative of the bandwidth available in the path. (The code was
>> >> correct at the time it was written, but the state machine changed
>> >> without this spot being adjusted correspondingly.)
>> >>
>> >> Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
>> >> Signed-off-by: Neal Cardwell <ncardwell@google.com>
>> >> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>> >> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
>> >
>
> You are correct, these look more like bug fixes. I was a little concerned
> that the changes would be visible but they really aren't user visible.

Yes, exactly.

> Should they go to stable as well?

Yes, please. The intention was for this whole 5-patch BBR pacing
bug-fix  series to go into "net" and into the -stable queue together.

thanks,
neal
David Miller July 15, 2017, 9:44 p.m. | #5
Series applied and queued up for -stable.

Please provide a proper "[PATCH net 0/N] " header posting next time.
All patch series should have one.
Neal Cardwell July 16, 2017, 5:18 a.m. | #6
On Sat, Jul 15, 2017 at 5:44 PM, David Miller <davem@davemloft.net> wrote:
>
> Series applied and queued up for -stable.
>
> Please provide a proper "[PATCH net 0/N] " header posting next time.
> All patch series should have one.

Sorry about that. Will do!

Thanks,
neal

Patch

diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index dbcc9352a48f..743e97511dc8 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -220,12 +220,11 @@  static u64 bbr_rate_bytes_per_sec(struct sock *sk, u64 rate, int gain)
  */
 static void bbr_set_pacing_rate(struct sock *sk, u32 bw, int gain)
 {
-	struct bbr *bbr = inet_csk_ca(sk);
 	u64 rate = bw;
 
 	rate = bbr_rate_bytes_per_sec(sk, rate, gain);
 	rate = min_t(u64, rate, sk->sk_max_pacing_rate);
-	if (bbr->mode != BBR_STARTUP || rate > sk->sk_pacing_rate)
+	if (bbr_full_bw_reached(sk) || rate > sk->sk_pacing_rate)
 		sk->sk_pacing_rate = rate;
 }