ACK/cmnt: [SRU][Zesty][PATCH 1/5] tcp_bbr: cut pacing rate only if filled pipe

[Vinson Lee]

On Fri, Sep 15, 2017 at 3:04 AM, Stefan Bader
<stefan.bader at canonical.com> wrote:
> On 13.09.2017 01:12, Vinson Lee wrote:
>> From: Neal Cardwell <ncardwell at google.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1708604
>>
>> 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 at google.com>
>> Signed-off-by: Yuchung Cheng <ycheng at google.com>
>> Signed-off-by: Soheil Hassas Yeganeh <soheil at google.com>
>> Signed-off-by: David S. Miller <davem at davemloft.net>
>> (cherry picked from commit 4aea287e90dd61a48268ff2994b56f9799441b62)
>> Signed-off-by: Vinson Lee <vlee at freedesktop.org>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
>
>> ---
>
> Since the same set is part of 4.9.42 (and 4.12.6) the set looks sensible. Just a
> few formal notes:
>
> First the bug report should have or follow SRU guidelines. Which mean be
> formatted to look like
>
> [SRU Justification]
>
> Impact: <what wrong now>
>
> Fix: <refer to patches, where do those come from>
>
> Testcase: <if possible how can the fix be checked quickly>
>
> Regression Risk: <how likely / how much impact>
>
>
> That text could be used for a cover email. Personally I prefer a quick technical
> background about the patches to follow. But repeating SRU justification is fine,
> too. But having some sort of intro helps to group the actual patches together.
>
> Looking at the bug report there is also a Xenial task which I believe is
> incorrect. If you could either change it to invalid if that is true or make a
> comment asking for this. That would help to clarify.
>

Please apply these patches to Ubuntu 4.10 kernels in both Xenial and Zesty.

> Thanks,
> Stefan
>
>
>>  net/ipv4/tcp_bbr.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
>> index b89bce4c721e..b152e1d3b754 100644
>> --- a/net/ipv4/tcp_bbr.c
>> +++ b/net/ipv4/tcp_bbr.c
>> @@ -221,12 +221,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;
>>  }
>>
>>
>
>