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

[Eric Dumazet]

On Mon, Aug 31, 2026 at 7:04 PM Tim Fuechsel <t.fuechsel@xxxxxx> wrote:
>
> TCP ROCCET is an new congestion control algorithm based on TCP CUBIC
> that improves its overall performance in cellular networks. By its mode
> of function, CUBIC causes bufferbloat while it tries to detect the
> available throughput of a network path. This is particularly a problem
> with large buffers in mobile networks. A more detailed description and
> analysis of this problem caused by TCP CUBIC can be found in [1].
> TCP ROCCET addresses the bufferbloat problem by adding two additional
> metrics to detect bufferbloat. The first metric is the relative increase
> in RTT from its minimum, the srRTT. Here, bufferbloat can be detected
> when RTTs increase due to buffer filling. The second metric is the
> acknowledgment arrival rate sampled over 100ms intervals. If CUBIC
> increases the send rate or congestion window, and the acknowledgment
> arrival rate stays on the same level, the connection is limited by the
> bottleneck link's capacity. In such cases, ROCCET reduces the send rate
> to prevent bufferbloat. ROCCET uses a modified version of slow start
> rather than HyStart because HyStart is known to enter the congestion
> avoidance phase too early when used in cellular networks [2]. Therefore,
> ROCCET uses a combination of srRTT and the acknowledgment arrival rate to
> determine when to exit slow start. For the congestion avoidance phase,
> ROCCET relies on the srRTT and monitoring the send and received Bytes
> to detect the filling of the bottleneck buffer.
>
> In real-world mobile 5G NR measurements, TCP ROCCET achieves better
> performance than CUBIC and BBRv3, by maintaining similar throughput
> while reducing the latency. In stationary 5G NR scenarios, the performance
> is similar to that of BBRv3. More information about TCP ROCCET and
> measurement evaluations can be found here [3].
> For the version (2026-07-21) we provide additional performance
> evaluation regarding throughput, latency and bandwidth share [4].
>
> [1] https://doi.org/10.1109/VTC2023-Fall60731.2023.10333357
> [2] https://doi.org/10.1109/WMNC.2016.7543932
> [3] https://doi.org/10.23919/WONS68803.2026.11501781
> [4] http://go.lu-h.de/roccet-2026-07-21
>
> Signed-off-by: Lukas Prause <lukas.prause@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Tim Fuechsel <t.fuechsel@xxxxxx>

Hi Lukas, Tim,

Here is a AI review of your patch. There are several architectural issues,
race conditions, and logic bugs that need to be addressed.

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.


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.


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.


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.


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.


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.


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.


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.


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").

Thanks.