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/20/24 07:53, Eric Dumazet wrote:
> On Mon, Aug 19, 2024 at 10:36 PM Mingrui Zhang <mrzhang97@gmail.com> wrote:
>> 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?
> I was referring to this line : Fixes: ac35f562203a ("tcp: bic, cubic:
> use tcp_jiffies32 instead of tcp_time_stamp")
>
> Commit ac35f562203a did not change the behavior at all.
>
> I see no particular reason to mention it, this is confusing.
>
>
>>>> 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
> No problem, there is no hurry.

Thank you, Eric, for your suggestion (switching ca->last_time from jiffies to usec)!
We thought about it and feel that it is more complicated and beyond the scope of this patch.

There are two blocks of code in bictcp_update().
* Block 1: cubic calculation, which is computationally intensive.
* Block 2: tcp friendliness, which emulates RENO.

There are two if statements to control how often these two blocks are called to reduce CPU overhead.
 * If statement 1:  if the condition is true, none of the two blocks are called.
if (ca->last_cwnd == cwnd &&
                    (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32)
                                return;

* If statement 2: If the condition is true, block 1 is not called. Intuitively, block 1 is called at most once per jiffy.
if (ca->epoch_start && tcp_jiffies32 == ca->last_time)
                                goto tcp_friendliness;


This patch changes only the first if statement. If we switch ca->last_time from jiffies to usec,
we need to change not only the first if statement but also the second if statement, as well as block 1.
* change the first if statement from jiffies to usec.
* change the second if statement from jiffies to usec. Need to determine how often (in usec) block 1 is called
* change block 1 from jiffies to usec. Should be fine, but need to make sure no calculation overflow.

Therefore, it might be better to keep the current patch as it is, and address the switch from jiffies to usec in future patches.

Thank you!
Lisong (Mingrui Send On Behalf of)