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

Stefan Bader stefan.bader at canonical.com
Fri Sep 15 10:04:20 UTC 2017


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.

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;
>  }
>  
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20170915/3e49087a/attachment.sig>


More information about the kernel-team mailing list