Hi Sasha,
When backporting this 0927520dbae ("tcp_cubic: better follow cubic
curve after idle period") commit to 3.18 or 4.1 it is important to
also backport the following follow-on fix for that commit:
commit c2e7204d180f8efc80f27959ca9cf16fa17f67db
Author: Eric Dumazet <edumazet@xxxxxxxxxx>
Date: Thu Sep 17 08:38:00 2015 -0700
tcp_cubic: do not set epoch_start in the future
Tracking idle time in bictcp_cwnd_event() is imprecise, as epoch_start
is normally set at ACK processing time, not at send time.
Doing a proper fix would need to add an additional state variable,
and does not seem worth the trouble, given CUBIC bug has been there
forever before Jana noticed it.
Let's simply not set epoch_start in the future, otherwise
bictcp_update() could overflow and CUBIC would again
grow cwnd too fast.
This was detected thanks to a packetdrill test Neal wrote that was flaky
before applying this fix.
Fixes: 30927520dbae ("tcp_cubic: better follow cubic curve after
idle period")
Signed-off-by: Eric Dumazet <edumazet@xxxxxxxxxx>
Signed-off-by: Neal Cardwell <ncardwell@xxxxxxxxxx>
Signed-off-by: Yuchung Cheng <ycheng@xxxxxxxxxx>
Cc: Jana Iyengar <jri@xxxxxxxxxx>
Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
Thanks,
neal
On Fri, Apr 22, 2016 at 10:00 PM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote:
> From: Eric Dumazet <edumazet@xxxxxxxxxx>
>
> This patch has been added to the 3.18 stable tree. If you have any
> objections, please let us know.
>
> ===============
>
> [ Upstream commit 30927520dbae297182990bb21d08762bcc35ce1d ]
>
> Jana Iyengar found an interesting issue on CUBIC :
>
> The epoch is only updated/reset initially and when experiencing losses.
> The delta "t" of now - epoch_start can be arbitrary large after app idle
> as well as the bic_target. Consequentially the slope (inverse of
> ca->cnt) would be really large, and eventually ca->cnt would be
> lower-bounded in the end to 2 to have delayed-ACK slow-start behavior.
>
> This particularly shows up when slow_start_after_idle is disabled
> as a dangerous cwnd inflation (1.5 x RTT) after few seconds of idle
> time.
>
> Jana initial fix was to reset epoch_start if app limited,
> but Neal pointed out it would ask the CUBIC algorithm to recalculate the
> curve so that we again start growing steeply upward from where cwnd is
> now (as CUBIC does just after a loss). Ideally we'd want the cwnd growth
> curve to be the same shape, just shifted later in time by the amount of
> the idle period.
>
> Reported-by: Jana Iyengar <jri@xxxxxxxxxx>
> Signed-off-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> Signed-off-by: Yuchung Cheng <ycheng@xxxxxxxxxx>
> Signed-off-by: Neal Cardwell <ncardwell@xxxxxxxxxx>
> Cc: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
> Cc: Sangtae Ha <sangtae.ha@xxxxxxxxx>
> Cc: Lawrence Brakmo <lawrence@xxxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> Signed-off-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
> ---
> net/ipv4/tcp_cubic.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
> index 20de011..2bacf93 100644
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -154,6 +154,21 @@ static void bictcp_init(struct sock *sk)
> tcp_sk(sk)->snd_ssthresh = initial_ssthresh;
> }
>
> +static void bictcp_cwnd_event(struct sock *sk, enum tcp_ca_event event)
> +{
> + if (event == CA_EVENT_TX_START) {
> + s32 delta = tcp_time_stamp - tcp_sk(sk)->lsndtime;
> + struct bictcp *ca = inet_csk_ca(sk);
> +
> + /* We were application limited (idle) for a while.
> + * Shift epoch_start to keep cwnd growth to cubic curve.
> + */
> + if (ca->epoch_start && delta > 0)
> + ca->epoch_start += delta;
> + return;
> + }
> +}
> +
> /* calculate the cubic root of x using a table lookup followed by one
> * Newton-Raphson iteration.
> * Avg err ~= 0.195%
> @@ -440,6 +455,7 @@ static struct tcp_congestion_ops cubictcp __read_mostly = {
> .cong_avoid = bictcp_cong_avoid,
> .set_state = bictcp_state,
> .undo_cwnd = bictcp_undo_cwnd,
> + .cwnd_event = bictcp_cwnd_event,
> .pkts_acked = bictcp_acked,
> .owner = THIS_MODULE,
> .name = "cubic",
> --
> 2.5.0
>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html