Re: [PATCH net v4 1/3] tcp_cubic: fix to run bictcp_update() at least once per RTT

[Mingrui Zhang <mrzhang97@gmail.com>]

On 8/19/24 04:00, Eric Dumazet wrote:
> On Sat, Aug 17, 2024 at 6:35 PM Mingrui Zhang <mrzhang97@gmail.com> wrote:
>> The original code bypasses bictcp_update() under certain conditions
>> to reduce the CPU overhead. Intuitively, when last_cwnd==cwnd,
>> bictcp_update() is executed 32 times per second. As a result,
>> it is possible that bictcp_update() is not executed for several
>> RTTs when RTT is short (specifically < 1/32 second = 31 ms and
>> last_cwnd==cwnd which may happen in small-BDP networks),
>> thus leading to low throughput in these RTTs.
>>
>> The patched code executes bictcp_update() 32 times per second
>> if RTT > 31 ms or every RTT if RTT < 31 ms, when last_cwnd==cwnd.
>>
>> Fixes: df3271f3361b ("[TCP] BIC: CUBIC window growth (2.0)")
>> Fixes: ac35f562203a ("tcp: bic, cubic: use tcp_jiffies32 instead of tcp_time_stamp")
> I do not understand this Fixes: tag ?
>
> Commit  ac35f562203a was essentially a nop at that time...
>
I may misunderstood the use of Fixes tag and choose the latest commit of that line.
 
Shall it supposed to be the very first commit with that behavior?
That is, the very first commit (df3271f3361b ("[TCP] BIC: CUBIC window growth (2.0)")) when the code was first introduced?
>> Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com>
>> Signed-off-by: Lisong Xu <xu@unl.edu>
>> ---
>> v3->v4: Replace min() with min_t()
>> v2->v3: Correct the "Fixes:" footer content
>> v1->v2: Separate patches
>>
>>  net/ipv4/tcp_cubic.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
>> index 5dbed91c6178..00da7d592032 100644
>> --- a/net/ipv4/tcp_cubic.c
>> +++ b/net/ipv4/tcp_cubic.c
>> @@ -218,8 +218,12 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
>>
>>         ca->ack_cnt += acked;   /* count the number of ACKed packets */
>>
>> +       /* Update 32 times per second if RTT > 1/32 second,
>> +        * or every RTT if RTT < 1/32 second even when last_cwnd == cwnd
>> +        */
>>         if (ca->last_cwnd == cwnd &&
>> -           (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32)
>> +           (s32)(tcp_jiffies32 - ca->last_time) <=
>> +           min_t(s32, HZ / 32, usecs_to_jiffies(ca->delay_min)))
> This looks convoluted to me and still limited if HZ=250 (some distros
> still use 250 jiffies per second :/ )
>
> I would suggest switching to usec right away.
Thank you for the suggestion, however, I may need more time to discuss with another author for this revision. :)
Thank you
>
> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
> index 5dbed91c6178257df8d2ccd1c8690a10bdbaf56a..fae000a57bf7d3803c5dd854af64b6933c4e26dd
> 100644
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -211,26 +211,27 @@ static u32 cubic_root(u64 a)
>  /*
>   * Compute congestion window to use.
>   */
> -static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked)
> +static inline void bictcp_update(struct tcp_sock *tp, struct bictcp *ca,
> +                                u32 cwnd, u32 acked)
>  {
>         u32 delta, bic_target, max_cnt;
>         u64 offs, t;
>
>         ca->ack_cnt += acked;   /* count the number of ACKed packets */
>
> -       if (ca->last_cwnd == cwnd &&
> -           (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32)
> +       delta = tp->tcp_mstamp - ca->last_time;
> +       if (ca->last_cwnd == cwnd && delta <= ca->delay_min)
>                 return;
>
> -       /* The CUBIC function can update ca->cnt at most once per jiffy.
> +       /* The CUBIC function can update ca->cnt at most once per ms.
>          * On all cwnd reduction events, ca->epoch_start is set to 0,
>          * which will force a recalculation of ca->cnt.
>          */
> -       if (ca->epoch_start && tcp_jiffies32 == ca->last_time)
> +       if (ca->epoch_start && delta < USEC_PER_MSEC)
>                 goto tcp_friendliness;
>
>         ca->last_cwnd = cwnd;
> -       ca->last_time = tcp_jiffies32;
> +       ca->last_time = tp->tcp_mstamp;
>
>         if (ca->epoch_start == 0) {
>                 ca->epoch_start = tcp_jiffies32;        /* record beginning */
> @@ -334,7 +335,7 @@ __bpf_kfunc static void cubictcp_cong_avoid(struct
> sock *sk, u32 ack, u32 acked)
>                 if (!acked)
>                         return;
>         }
> -       bictcp_update(ca, tcp_snd_cwnd(tp), acked);
> +       bictcp_update(tp, ca, tcp_snd_cwnd(tp), acked);
>         tcp_cong_avoid_ai(tp, ca->cnt, acked);
>  }