Re: Re: [PATCHv5 net-next] tcp: Add TCP ROCCET congestion control module.

[Lukas Prause <lukas.prause@ikt.uni-hannover.de>]

[-- Attachment #1: Type: text/plain, Size: 7454 bytes --]

Hi Eric,
thank you for the review. On a few points, I’d have some comments. We
think that this AI review is, at some point, a little bit misleading.
We are using the cong_control functionality to implement stricter packet
pacing than the default value.
With this, we aim to implement our own alternative to PRR.

> 1. Concurrency data race on module parameters in socket init
> ------------------------------------------------------------
> In roccettcp_init():
>     param_check(true);
>     param_precompute();
>
> Both functions mutate global static variables (beta, bic_scale,
beta_scale,
> cube_rtt_scale, cube_factor) that are marked __read_mostly.
> Calling this in roccettcp_init() on every socket creation causes
unsynchronized
> concurrent writes across CPUs. Furthermore, param_check() calls pr_err() /
> pr_info(), which will spam the kernel log on every connection creation
if an
> invalid parameter was configured.
>
> Global scale factors should only be precomputed at module init time
> (roccettcp_register), or validated using kernel_param_ops.
I agree with you. We will fix that.

> 2. Bypassing PRR and broken loss recovery via .cong_control
> ----------------------------------------------------------
> By defining .cong_control = roccet_control, TCP core completely delegates
> congestion control and skips PRR (tcp_cwnd_reduction()).
>
> However, in roccet_control():
>     if (!ca->is_in_recovery)
>         roccettcp_cong_avoid(sk, ack, rs->acked_sacked);
>
> During recovery (TCP_CA_Recovery), ROCCET does nothing to regulate cwnd or
> clock out retransmissions according to RFC 6937. Unless you have a full
> custom recovery engine (like BBR), you should stick to .cong_avoid so the
> stack handles PRR and loss recovery properly.
If we are in recovery we still do packet pacing to 100 percent of the
cwnd instead of 120.

> 3. Broken .ssthresh callback and ignored ECN (TCP_CA_CWR)
> ---------------------------------------------------------
> roccettcp_ssthresh() simply returns tp->snd_ssthresh without
recalculation:
>     static u32 roccettcp_ssthresh(struct sock *sk)
>     {
>         return tcp_sk(sk)->snd_ssthresh;
>     }
>
> The recalculation was moved to roccettcp_state(..., TCP_CA_Recovery).
> This breaks the TCP stack contract:
> a) tcp_init_cwnd_reduction() initializes PRR state using the return
value of
>    icsk_ca_ops->ssthresh(sk). Returning the unreduced ssthresh
corrupts PRR.
> b) When ECN CE marks arrive, tcp_enter_cwr() transitions to TCP_CA_CWR
and calls
>    icsk_ca_ops->ssthresh(sk). Since roccettcp_state() only checks for
Loss and
>    Recovery, TCP_CA_CWR is completely ignored. Consequently, neither
ssthresh
>    nor cwnd is ever reduced on ECN.
We are not using the PRR from the TCP stack, because we are implementing
cong_control.
I agree that ECNs are ignored here. We will fix that.

> 4. Congestion avoidance starvation in ORBITER
> ---------------------------------------------
> In roccettcp_cong_avoid():
>     send = tp->snd_nxt - ca->interval_snd_seq_start;
>     received = tp->snd_una - ca->interval_una_seq_start;
>     send_more_than_acked =
>         send > received + ((tcp_snd_cwnd(tp) * tp->mss_cache) / 100);
>     ...
>     if (!tcp_is_cwnd_limited(sk) || send_more_than_acked)
>         return;
>
> During steady-state transmission, the in-flight byte count (send -
received)
> is approximately cwnd * mss. This is ~100x larger than the 1% guard space
> ((cwnd * mss) / 100). Thus, send_more_than_acked evaluates to true on
almost
> every ACK, returning before bictcp_update() or tcp_cong_avoid_ai() can
run.
> Window growth in congestion avoidance is completely starved.
That is not true. Here we are monitoring the bytes sent and received
over a time interval of 5 RTT

> 5. curr_min_rtt poisoning and premature slow-start exit
> -------------------------------------------------------
> In roccettcp_reset(), ca->curr_rtt is 0 and ca->curr_min_rtt is ~0U.
> In update_min_rtt():
>     if (ca->curr_rtt < ca->curr_min_rtt) {
>         ca->curr_min_rtt = max(ca->curr_rtt, 1);
>         ...
>
> If an ACK arrives before an RTT sample is collected (e.g.
sample->rtt_us <= 0),
> ca->curr_rtt is 0. ca->curr_min_rtt is set to max(0, 1) = 1 us.
> Because 1 us is smaller than any real path RTT, ca->curr_min_rtt
remains stuck
> at 1 us.
> Then in update_srrtt():
>     u32 rrtt = div_u64(100 * (u64)(ca->curr_rtt - ca->curr_min_rtt),
>                        ca->curr_min_rtt);
> For a 20 ms RTT (20000 us), rrtt evaluates to 1,999,900. curr_srrtt
immediately
> exceeds sr_rtt_upper_bound and terminates slow start on the very first
sample.
That could be a problem, and we will implement a fix for it.

> 6. Perpetual min-RTT probe loop
> -------------------------------
> In roccet_min_rtt_probe(), when the probe finishes:
>     ca->probe_min_rtt_until = 0;
>     ca->state = ORBITER;
>
> ca->next_min_rtt_probe is not updated when exiting the probe. If no
new lower
> RTT was found during the probe, next_min_rtt_probe remains in the past.
> On the next ACK, after(now, ca->next_min_rtt_probe) immediately
evaluates to
> true, trapping the flow in a continuous probe loop and repeatedly
halving cwnd
> down to TCP_INIT_CWND.
That is not true. Every time a new minimum RTT is found, we calculate
ca->next_min_rtt_probe, and before we enter probing, the current minimum
RTT is set to u32 max.
This means the next RTT is the new minimum RTT.

> 7. 71-minute timestamp wrap and permanent DRAIN state
> -----------------------------------------------------
> now is calculated via jiffies_to_usecs(tcp_jiffies32), which wraps every
> ~71.5 minutes (2^32 us).
>
> In roccet_control():
>     else if ((s32)now - ca->roccet_last_event_time_us <= 100 *
USEC_PER_MSEC)
>         ca->state = DRAIN;
>
> ca->roccet_last_event_time_us is initialized to 0. When bit 31 of now
is set
> (for 35.7 minutes out of every 71.5 minutes), (s32)now is negative, so
> (s32)now - 0 is negative and always <= 100000. Newly created
connections during
> that 35-minute window will immediately enter and remain permanently
stuck in
> DRAIN state.
>
> Also, before() and after() from <net/tcp.h> are sequence number comparison
> macros (32-bit TCP seqno), not time comparison macros.
This is a problem, and we will fix it.
In the past, we got a hint that we should use the before and after
macros here.

> 8. Re-implementing tcp_update_pacing_rate()
> -------------------------------------------
> roccet_control() duplicates the pacing rate calculation from
tcp_input.c and
> hardcodes 100% pacing instead of honoring sysctl_tcp_pacing_ca_ratio.
If you
> switch to .cong_avoid, the core stack will manage pacing updates
automatically.
We do this as an alternative to PRR with stricter pacing.

> 9. Coding style & formatting
> ----------------------------
> - Declarations after statements (ISO C90 violations) in roccettcp_init(),
>   update_srrtt(), roccettcp_acked(), and roccettcp_register().
> - C++ style comments (//) instead of /* ... */ in multiple places.
> - Commented-out dead code: //WRITE_ONCE(sk->sk_pacing_rate, 0);
> - Typos in comments ("sRrTT", "probeing", "has was idle", "defer", etc.)
>   and references to non-existent struct fields ("curr_min_rtt_timed.rtt").
Of course, we will correct that.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4930 bytes --]