| Message ID | 20170714214925.30720-1-ncardwell@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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?
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
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.
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
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; }
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(-)