tcp: Add TCP ROCCET congestion control module.
Subsystems (4)
PATCH v3 1/1 tcp: Add TCP ROCCET congestion control module.
TCP ROCCET is an extension of 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 5 RTT 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 only on the srRTT.
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].
[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
---
Changes since v1:
* Adjust some comments & Rework commit message
* Fix Kconfig format
* Add div-by-0 check variables
* Fix ack summation from +=1 to +=acked (counting now ACKs instead of ACK events)
* Fix wrapping in time comparisons
* Remove most module_params, including 'ignore_loss'
* Remove check that always evaluated to 'true' regarding 'bw_limit_detect'
Changes since v2:
* Improve initialization of roccettcp struct
* Always react to ECE bits and reset flag
* Detect plateaus in ACK-rate
* Fix potential overflow in ACK-rate tracking for high-speed connections
* Reset ACK-counter correctly on new interval
* Fix potential integer overflow in rRTT calculation
--- net/ipv4/Kconfig | 12 + net/ipv4/Makefile | 1 + net/ipv4/tcp_roccet.c | 647 ++++++++++++++++++++++++++++++++++++++++++ net/ipv4/tcp_roccet.h | 46 +++ 4 files changed, 706 insertions(+) create mode 100644 net/ipv4/tcp_roccet.c create mode 100644 net/ipv4/tcp_roccet.h
net/ipv4/Kconfig +12 -0
@@ -663,6 +663,18 @@ config TCP_CONG_CDG delay gradients." In Networking 2011. Preprint: http://caia.swin.edu.au/cv/dahayes/content/networking2011-cdg-preprint.pdf +config TCP_CONG_ROCCET + tristate "ROCCET TCP" + default n + help + TCP ROCCET is a sender-side only modification of the TCP CUBIC + protocol stack/TCP CUBIC congestion control algorithm that + optimizes the performance of TCP congestion control. Especially + for networks with large buffers (wireless, cellular networks), + TCP ROCCET has improved performance by maintaining similar + throughput as CUBIC while reducing the latency. + For more information, see: https://arxiv.org/abs/2510.25281 + config TCP_CONG_BBR tristate "BBR TCP" default n
net/ipv4/Makefile +1 -0
@@ -45,6 +45,7 @@ obj-$(CONFIG_INET_TCP_DIAG) += tcp_diag.o obj-$(CONFIG_INET_UDP_DIAG) += udp_diag.o obj-$(CONFIG_INET_RAW_DIAG) += raw_diag.o obj-$(CONFIG_TCP_CONG_BBR) += tcp_bbr.o +obj-$(CONFIG_TCP_CONG_ROCCET) += tcp_roccet.o obj-$(CONFIG_TCP_CONG_BIC) += tcp_bic.o obj-$(CONFIG_TCP_CONG_CDG) += tcp_cdg.o obj-$(CONFIG_TCP_CONG_CUBIC) += tcp_cubic.o
net/ipv4/tcp_roccet.c new file +647 -0
647 lines not on this page · raw message
net/ipv4/tcp_roccet.h new file +46 -0
46 lines not on this page · raw message
base-commit: 73d587ae684d176fac9db94173f77d78a794ea4f
Signature · 1 line
2.43.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [Critical] Division by zero occurs in `roccettcp_cong_avoid` when an ACK arrives without an RTT sample…
Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Division by zero occurs in roccettcp_cong_avoid when an ACK arrives without an RTT sample, causing a guaranteed kernel panic.
- [High] Double multiplicative decrease of congestion window and ssthresh upon receiving ECN (ECE) marks.
- [High] Initialization of time tracking variables to 0 causes the after() macro to fail, breaking ACK rate tracking.
- [High] Loading the module with specific module parameter values causes a division by zero panic during initialization.
- [High] ROCCET aggressively halves cwnd and exits slow start simply when the socket is not cwnd limited.
- [High] ROCCET completely ignores packet loss during slow start, violating fundamental TCP congestion control principles.
- [High] Permanent tracking of absolute minimum RTT causes perpetual congestion window reduction on network handover.
- [Medium] Static functions are incorrectly decorated with __bpf_kfunc and registered as BTF kfuncs, violating BPF subsystem rules.
- [Medium] Missing zero-filling in ACK rate sampling causes stale rate comparisons and spurious slow-start exits.
- [Medium] The acknowledgment arrival rate is sampled over a fixed 100ms interval, not 5 RTT intervals.
--
commit bedc1f2efa75e431179c08512e7b18a28104cd5a
Author: Tim Fuechsel <[email protected]>
tcp: Add TCP ROCCET congestion control module.
This commit introduces TCP ROCCET, a sender-side extension of CUBIC optimized for cellular networks. It relies on RTT measurements and ACK arrival rates to address bufferbloat during the slow start and congestion avoidance phases.
Quoted 2 lines
The second metric is the acknowledgment arrival rate sampled over 5 RTT intervals.
[Severity: Medium]
Is this description accurate? The commit message specifies that the acknowledgment arrival rate is sampled over 5 RTT intervals, but the implementation in the code uses a static 100ms interval instead.
Quoted 4 lines
net/ipv4/tcp_roccet.c new file +0
-0
[ ... ]
net/ipv4/tcp_roccet.c line 116
Quoted 8 lines
net/ipv4/tcp_roccet.c new file +8
-0+static __always_inline void update_min_rtt(struct sock *sk) +{ +struct roccettcp *ca =inet_csk_ca (sk); + +/* Check if new lower min RTT was found. If so, set it directly */ +if (ca->curr_rtt < ca->curr_min_rtt) + ca->curr_min_rtt =max (ca->curr_rtt,1 ); +}
[Severity: High]
Does this code handle network handovers correctly? Since curr_min_rtt only updates downwards and is never expired, if a connection moves from a low-latency network (like Wi-Fi) to a higher-latency network (like 5G Edge), could curr_min_rtt remain locked to the lower latency? This might cause curr_srrtt to constantly exceed sr_rtt_upper_bound on the new network, potentially triggering endless congestion window reductions.
[ ... ]
net/ipv4/tcp_roccet.c line 135
Quoted 7 lines
net/ipv4/tcp_roccet.c new file +7
-0+static __always_inline void update_ack_rate(struct sock *sk, u32 acked) +{ +struct roccettcp *ca =inet_csk_ca (sk); +u32 now =jiffies_to_usecs (tcp_jiffies32); +u32 interval =USEC_PER_MSEC *100 ; + +if (after (now, ca->ack_rate.last_rate_time + interval)) {
[Severity: High]
Could the after() macro fail here upon system boot? Since ca->ack_rate.last_rate_time is statically initialized to 0 in roccettcp_init(), and tcp_jiffies32 is initialized to a large negative offset that evaluates to a massive positive 32-bit timestamp in usecs, the difference between now and last_rate_time might exceed 2^31. Would this cause after() to incorrectly evaluate to false for a large portion of the system's uptime cycle and break ACK rate tracking?
[Severity: Medium]
Is this hardcoded 100ms interval intentional? As mentioned earlier, the commit message specifies sampling over 5 RTT intervals, but this code uses a static 100ms interval that is completely disconnected from the actual RTT.
net/ipv4/tcp_roccet.c line 139
Quoted 4 lines
net/ipv4/tcp_roccet.c new file +4
-0+ ca->ack_rate.last_rate_time = now; + ca->ack_rate.last_rate = ca->ack_rate.curr_rate; + ca->ack_rate.curr_rate = ca->ack_rate.cnt; + ca->ack_rate.cnt = acked;// start counting for the new interval
[Severity: Medium]
Does this logic safely handle long idle periods? If a connection is idle for several seconds, update_ack_rate() isn't called. When a new burst arrives, this shifts the interval directly adjacent to the previous burst's intervals without zero-filling the idle time. Could this trick ROCCET into spuriously exiting slow start on the first ACK after an idle period because last_rate still reflects traffic from before the idle period?
[ ... ]
Quoted 4 lines
net/ipv4/tcp_roccet.c new file +4
-0+static __always_inline void update_srrtt(struct sock *sk) +{ +struct roccettcp *ca =inet_csk_ca (sk); +
net/ipv4/tcp_roccet.c line 160
Quoted 10 lines
net/ipv4/tcp_roccet.c new file +10
-0+/* Avoid integer overflow in the calculation below. +* This could occur in cases where we have not yet +* received an RTT sample. +*/ +if (ca->curr_rtt < ca->curr_min_rtt) + ca->curr_min_rtt = ca->curr_rtt; + +/* avoid division by zero */ +if (ca->curr_min_rtt ==0 ) +return ;
[Severity: Critical]
Can this sequence cause a division by zero panic in roccettcp_cong_avoid()?
If an ACK arrives without an RTT sample (e.g., for a retransmitted packet), ca->curr_rtt remains 0. update_min_rtt() sets it to max(0, 1) = 1. Then update_srrtt() executes the condition above:
if (ca->curr_rtt < ca->curr_min_rtt)
ca->curr_min_rtt = ca->curr_rtt;Because curr_rtt (0) is less than curr_min_rtt (1), this resets curr_min_rtt back to 0. update_srrtt() then correctly returns early, but execution continues in roccettcp_cong_avoid() where it calculates:
roccet_xj = ((jitter * 100) / ca->curr_min_rtt) + sr_rtt_upper_bound;
Since ca->curr_min_rtt is exactly 0, wouldn't this calculation trigger a kernel panic?
[ ... ]
Quoted 1 line
+__bpf_kfunc static void
roccettcp_init(struct sock *sk)
[Severity: Medium]
Is it supported to mark static functions as __bpf_kfunc? The BPF subsystem requires kfuncs to be global functions so they can be properly resolved by resolve_btfids. Also, does a native TCP congestion control module need to expose its internal functions as BPF kfuncs to operate?
net/ipv4/tcp_roccet.c line 190
Quoted 12 lines
net/ipv4/tcp_roccet.c new file +12
-0+{ +struct roccettcp *ca =inet_csk_ca (sk); + +roccettcp_reset (ca); + +if (initial_ssthresh) +tcp_sk (sk)->snd_ssthresh = initial_ssthresh; + +/* Initial roccet parameters */ + ca->roccet_last_event_time_us =0 ; + ca->ack_rate.last_rate =0 ; + ca->ack_rate.last_rate_time =0 ;
[Severity: High]
As noted above, initializing last_rate_time to 0 here causes issues with the after() macro check in update_ack_rate() due to the initial negative offset of tcp_jiffies32.
[ ... ]
Quoted 3 lines
net/ipv4/tcp_roccet.c new file +3
-0+__bpf_kfunc static void roccettcp_cong_avoid(struct sock *sk, u32 ack, +u32 acked) +{
[ ... ]
net/ipv4/tcp_roccet.c line 413
Quoted 11 lines
net/ipv4/tcp_roccet.c new file +11
-0+/* LAUNCH: Detect an exit point for tcp slow start +* in networks with large buffers of multiple BDP +* Like in cellular networks (5G, ...). +* Or exit LAUNCH if cwnd is too large for application layer +* data rate (tcp cwnd validation). +*/ + +if ((tcp_in_slow_start (tp) && ca->curr_srrtt > sr_rtt_upper_bound && +get_ack_rate_diff (ca) <= ack_rate_diff_ss) || + (!tcp_is_cwnd_limited (sk) &&tcp_in_slow_start (tp))) { + ca->epoch_start =0 ;
[Severity: High]
Does this cause severe throughput degradation for interactive applications? The condition !tcp_is_cwnd_limited(sk) indicates the application is not currently sending enough data to fill the window, such as during small bursts or brief idle periods. Using this condition to aggressively halve cwnd and ssthresh during slow start seems like it would heavily penalize bursty applications.
[ ... ]
net/ipv4/tcp_roccet.c line 457
Quoted 7 lines
net/ipv4/tcp_roccet.c new file +7
-0+/* Respects the jitter of the connection and add it on top of +* the upper bound for the srRTT. +*/ + roccet_xj = ((jitter *100 ) / ca->curr_min_rtt) + + sr_rtt_upper_bound; +if (roccet_xj < sr_rtt_upper_bound) + roccet_xj = sr_rtt_upper_bound;
[Severity: Critical]
As discussed in update_srrtt(), if an ACK arrives without an RTT sample, ca->curr_min_rtt can be 0 here, which will cause a division by zero panic.
Quoted 11 lines
net/ipv4/tcp_roccet.c new file +11
-0+/* The srRTT exceeds the upper bound if bufferbloat happens. +* Here, we want to reduce the cwnd and drain the buffer. +*/ +if ((ca->curr_srrtt > roccet_xj && evaluate_srrtt) || + ca->ece_received) { +if (ca->ece_received) + ca->ece_received = false; + ca->epoch_start =0 ; + ca->roccet_last_event_time_us = now; + ca->cnt =100 *tcp_snd_cwnd (tp); +
net/ipv4/tcp_roccet.c line 479
Quoted 10 lines
net/ipv4/tcp_roccet.c new file +10
-0+/* Set Wmax if cwnd is larger than the old Wmax */ +if (tcp_snd_cwnd (tp) > ca->last_max_cwnd) + ca->last_max_cwnd =tcp_snd_cwnd (tp); + +tcp_snd_cwnd_set (tp,min (tp->snd_cwnd_clamp, +max ((tcp_snd_cwnd (tp) * beta) / +BICTCP_BETA_SCALE ,2U ))); + tp->snd_ssthresh =tcp_snd_cwnd (tp); +return ; + }
[Severity: High]
Could this result in a double penalty for a single ECE event?
When an ECE flag is seen, roccet_in_ack_event() sets ca->ece_received to true. The core TCP stack handles this by entering the CWR state, adjusting ssthresh, and lowering cwnd via PRR. During this time, roccettcp_cong_avoid() is bypassed.
When the stack returns to the Open state and resumes calling roccettcp_cong_avoid(), ca->ece_received is still true. Does this cause an unintended second multiplicative decrease of the congestion window and ssthresh?
[ ... ]
net/ipv4/tcp_roccet.c line 504
Quoted 10 lines
net/ipv4/tcp_roccet.c new file +10
-0+__bpf_kfunc static u32 roccettcp_recalc_ssthresh(struct sock *sk) +{ +const struct tcp_sock *tp =tcp_sk (sk); +struct roccettcp *ca =inet_csk_ca (sk); + +/* On loss in slow start enter congestion avoidance +* without a cwnd reduction. +*/ +if (tcp_in_slow_start (tp)) +return tcp_snd_cwnd (tp);
[Severity: High]
Does ignoring packet loss during slow start violate standard TCP congestion control safety principles (like RFC 5681)? Refusing to back off in response to dropped packets can lead to network congestion collapse and unfairly starve competing flows.
[ ... ]
Quoted 16 lines
net/ipv4/tcp_roccet.c new file +16
-0+static int __init roccettcp_register (void ) +{ +int ret; + +BUILD_BUG_ON (sizeof (struct roccettcp ) >ICSK_CA_PRIV_SIZE ); + +/* Precompute a bunch of the scaling factors that are used per-packet +* based on SRTT of 100ms +*/ + + beta_scale = +8 * (BICTCP_BETA_SCALE + beta) /3 / (BICTCP_BETA_SCALE - beta); + + cube_rtt_scale = (bic_scale *10 );/* 1024*c/rtt */ + +/* calculate the "K" for (wmax-cwnd) = c/rtt * K^3
[ ... ]
net/ipv4/tcp_roccet.c line 627
Quoted 5 lines
net/ipv4/tcp_roccet.c new file +5
-0+/* 1/c * 2^2*bictcp_HZ * srtt */ + cube_factor =1ull << (10 +3 *BICTCP_HZ );/* 2^40 */ + +/* divide by bic_scale and by constant Srtt (100ms) */ +do_div (cube_factor, bic_scale *10 );
[Severity: High]
Can a user panic the kernel during module initialization by passing specific module parameters?
The beta and bic_scale variables are module parameters that are not bounded or validated. If a user loads the module with beta=1024 or bic_scale=0, the expressions (BICTCP_BETA_SCALE - beta) and bic_scale * 10 will evaluate to 0, leading to a division by zero panic here.
net/ipv4/tcp_roccet.c line 630
Quoted 2 lines
net/ipv4/tcp_roccet.c new file +2
-0+ ret =register_btf_kfunc_id_set (BPF_PROG_TYPE_STRUCT_OPS , + &tcp_roccet_kfunc_set);
[Severity: Medium]
As mentioned earlier, static functions cannot be reliably registered as BPF kfuncs.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/ah_ZFYB_tiw45_fg@volt-roccet-vm?part=1
Jakub Kicinski
maintainerThis is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
tcp: Add TCP ROCCET congestion control module.
This patch adds a new TCP congestion control algorithm derived from TCP CUBIC, called ROCCET, which adds bufferbloat detection based on srRTT and ACK arrival rate, and replaces HyStart with a custom slow start phase called LAUNCH. A few observations follow.
[Low]
The commit message says:
The second metric is the acknowledgment arrival rate sampled over 5 RTT intervals.
Looking at update_ack_rate(), the sampling window is a fixed 100 ms wall-clock interval (u32 interval = USEC_PER_MSEC * 100), not a multiple of the connection's RTT. The 5*curr_rtt window only governs ca->next_srrtt_check. Should the commit message match the implementation, or should the implementation be changed to actually sample over 5 RTT intervals?
[Low]
Could the commit message also discuss why ROCCET cannot be implemented as a BPF struct_ops program on top of CUBIC, given that this same patch already registers BTF kfuncs for the struct_ops interface and that most of the file is a verbatim copy of CUBIC?
Quoted 9 lines
net/ipv4/Kconfig +1
-0@@ -663,6 +663,18 @@ config TCP_CONG_CDG delay gradients." In Networking 2011. Preprint: http://caia.swin.edu.au/cv/dahayes/content/networking2011-cdg-preprint.pdf +config TCP_CONG_ROCCET
net/ipv4/Kconfig config TCP_CONG_CDG · line 676
Quoted 10 lines
net/ipv4/Kconfig +10
-0+ tristate "ROCCET TCP" + default n + help + TCP ROCCET is a sender-side only modification of the TCP CUBIC + protocol stack/TCP CUBIC congestion control algorithm that + optimizes the performance of TCP congestion control. Especially + for networks with large buffers (wireless, cellular networks), + TCP ROCCET has improved performance by maintaining similar + throughput as CUBIC while reducing the latency. + For more information, see: https://arxiv.org/abs/2510.25281
[Low]
The help text describes ROCCET as a "modification of the TCP CUBIC protocol stack/TCP CUBIC congestion control algorithm", but CUBIC itself is not modified by this patch; ROCCET is a new sibling module that copies large parts of CUBIC. The acronym ROCCET is also never expanded here. Should the help text be reworded?
net/ipv4/tcp_roccet.h
Quoted 6 lines
net/ipv4/tcp_roccet.h new file +0
-0@@ -0,0 +1,46 @@
[Low]
Is the separate header net/ipv4/tcp_roccet.h needed? It only declares struct ack_rate and struct roccettcp, both of which are used solely inside tcp_roccet.c. Other in-tree CCs (tcp_cubic.c, tcp_bbr.c, tcp_vegas.c, tcp_dctcp.c, tcp_bic.c, tcp_cdg.c, ...) keep their per-socket state in the .c file.
Quoted 2 lines
net/ipv4/tcp_roccet.h new file +2
-0+struct roccettcp { +u32 cnt;/* increase cwnd by 1 after ACKs */
[ ... ]
net/ipv4/tcp_roccet.h line 33
Quoted 1 line
net/ipv4/tcp_roccet.h new file +1
-0+u32 curr_rtt;/* the minimum rtt of current round */
[Low]
The comment says "the minimum rtt of current round", but in roccettcp_acked() curr_rtt is assigned the latest sample (ca->curr_rtt = sample->rtt_us;) without any min-of-round logic. Was the comment intended to be copied from tcp_cubic.c, where curr_rtt has different semantics?
Quoted 5 lines
net/ipv4/tcp_roccet.c new file +0
-0
[ ... ]
Quoted 5 lines
net/ipv4/tcp_roccet.c new file +5
-0+/* Parameters that are specific to the ROCCET-Algorithm */ +static int sr_rtt_upper_bound__read_mostly =100 ; +static int ack_rate_diff_ss__read_mostly =10 ; + +module_param (sr_rtt_upper_bound, int,0644 );
net/ipv4/tcp_roccet.c line 81
Quoted 10 lines
net/ipv4/tcp_roccet.c new file +10
-0+MODULE_PARM_DESC (sr_rtt_upper_bound,"ROCCET's upper bound for srRTT." ); +module_param (ack_rate_diff_ss, int,0644 ); +MODULE_PARM_DESC (ack_rate_diff_ss, +"ROCCET's threshold to exit slow start if ACK-rate defer by given amount of segments." ); + +static int fast_convergence__read_mostly =1 ; +static int beta__read_mostly =717 ;/* = 717/1024 (BICTCP_BETA_SCALE) */ +static int initial_ssthresh__read_mostly ; +static int bic_scale__read_mostly =41 ; +static int tcp_friendliness__read_mostly =1 ;
[Low]sr_rtt_upper_bound and ack_rate_diff_ss are declared signed int and exposed as 0644 module params, but they are compared to and added to u32 quantities (curr_srrtt, get_ack_rate_diff(), roccet_xj). If a negative value is written, the signed-to-unsigned promotion turns it into a very large unsigned, effectively disabling the bufferbloat detection branch. Should these be unsigned int with a setter that rejects 0 / negative values?
[Low]
Is there a way to avoid adding seven new module_param tunables to a new in-tree CC? Five of them duplicate CUBIC's identically named knobs (fast_convergence, beta, initial_ssthresh, bic_scale, tcp_friendliness), which makes it hard for an admin to reason about which knob applies to which CC, and these are global (not per-netns) unlike the sysctls used by other modern CCs.
net/ipv4/tcp_roccet.c line 116
Quoted 8 lines
net/ipv4/tcp_roccet.c new file +8
-0+static __always_inline void update_min_rtt(struct sock *sk) +{ +struct roccettcp *ca =inet_csk_ca (sk); + +/* Check if new lower min RTT was found. If so, set it directly */ +if (ca->curr_rtt < ca->curr_min_rtt) + ca->curr_min_rtt =max (ca->curr_rtt,1 ); +}
[High]
Can update_min_rtt() and update_srrtt() leave curr_min_rtt at 0 and crash the kernel via divide-by-zero in roccettcp_cong_avoid()?
After roccettcp_reset() (called from roccettcp_init() and from roccettcp_state(TCP_CA_Loss)) curr_rtt is 0 and curr_min_rtt is ~0U. On the next ACK that reaches roccettcp_cong_avoid() before roccettcp_acked() has set curr_rtt to a non-zero value (the early return in roccettcp_acked() suppresses updates for HZ jiffies after fast recovery), update_min_rtt() does:
if (ca->curr_rtt < ca->curr_min_rtt) ca->curr_min_rtt = max(ca->curr_rtt, 1);
so curr_min_rtt is clamped to 1. Then update_srrtt() runs:
if (ca->curr_rtt < ca->curr_min_rtt) ca->curr_min_rtt = ca->curr_rtt;
which overwrites curr_min_rtt with 0 (curr_rtt). update_srrtt() returns early on its own divide-by-zero guard, but cong_avoid keeps running and reaches:
roccet_xj = ((jitter * 100) / ca->curr_min_rtt) + sr_rtt_upper_bound;
and divides by 0.
Once curr_min_rtt becomes 0, the unsigned compare (ca->curr_rtt < ca->curr_min_rtt) is false for any later positive curr_rtt, so curr_min_rtt stays 0 for the lifetime of the socket. Is that intended?
Quoted 4 lines
net/ipv4/tcp_roccet.c new file +4
-0+/* Compute srRTT. +*/ +static __always_inline void update_srrtt(struct sock *sk) +{
[ ... ]
net/ipv4/tcp_roccet.c line 171
Quoted 2 lines
net/ipv4/tcp_roccet.c new file +2
-0+u32 rrtt = (100 * (ca->curr_rtt - ca->curr_min_rtt)) / + ca->curr_min_rtt;
[Medium]
Can this overflow? curr_rtt and curr_min_rtt are microseconds and can reach values close to TCP_RTO_MAX (~120 s). 100 * (curr_rtt - curr_min_rtt) is computed in u32 and wraps once the difference exceeds about 42.9 ms. The wrapped rrtt is then folded into curr_srrtt and drives the LAUNCH/ORBITER decisions. Should this be promoted to u64 with div_u64()?
[ ... ]
Quoted 11 lines
net/ipv4/tcp_roccet.c new file +11
-0+__bpf_kfunc static void roccettcp_cwnd_event(struct sock *sk, +enum tcp_ca_event event) +{ +if (event ==CA_EVENT_TX_START ) { +struct roccettcp *ca =inet_csk_ca (sk); +u32 now = tcp_jiffies32; +s32 delta; + + delta = now -tcp_sk (sk)->lsndtime; + +/* We were application limited (idle) for a while.
net/ipv4/tcp_roccet.c line 215
Quoted 10 lines
net/ipv4/tcp_roccet.c new file +10
-0+* Shift epoch_start to keep cwnd growth to cubic curve. +*/ +if (ca->epoch_start && delta >0 ) { + ca->epoch_start += delta; +if (after (ca->epoch_start, now)) + ca->epoch_start = now; + } +return ; + } +}
[High]
Is this body reachable? In include/net/tcp.h tcp_ca_event() dispatches CA_EVENT_TX_START exclusively to .cwnd_event_tx_start and returns before calling .cwnd_event:
if (event == CA_EVENT_TX_START) {
if (icsk->icsk_ca_ops->cwnd_event_tx_start)
icsk->icsk_ca_ops->cwnd_event_tx_start(sk);
return;
}tcp_cubic.c handles this by registering cubictcp_cwnd_event_tx_start in .cwnd_event_tx_start. With the registration here only setting .cwnd_event = roccettcp_cwnd_event, the entire idle-period epoch_start adjustment is dead code, and on a transition out of application-limited idle bictcp_update() will compute t with a stale epoch_start, which then propagates through:
delta = (cube_rtt_scale * offs * offs * offs) >> (10 + 3 * BICTCP_HZ);
producing a large bic_target/cnt. Should this be wired to .cwnd_event_tx_start instead?
[ ... ]
Quoted 5 lines
net/ipv4/tcp_roccet.c new file +5
-0+/* calculate the cubic root of x using a table lookup followed by one +* Newton-Raphson iteration. +* Avg err ~= 0.195% +*/ +static u32 cubic_root (u64 a)
[ ... ]
net/ipv4/tcp_roccet.c line 266
Quoted 4 lines
net/ipv4/tcp_roccet.c new file +4
-0+/* Compute congestion window to use. +*/ +static __always_inline void bictcp_update(struct roccettcp *ca, u32 cwnd, +u32 acked)
[Low]cubic_root(), bictcp_update(), the module_param block, and the precomputation in roccettcp_register() appear to be byte-for-byte copies of tcp_cubic.c. Should these be factored as shared helpers in a common file (or ROCCET implemented as a BPF struct_ops on top of CUBIC) so that future numerical fixes to CUBIC propagate to ROCCET?
[ ... ]
Quoted 5 lines
net/ipv4/tcp_roccet.c new file +5
-0+__bpf_kfunc static void roccettcp_cong_avoid(struct sock *sk, u32 ack, +u32 acked) +{ +struct tcp_sock *tp =tcp_sk (sk); +struct roccettcp *ca =inet_csk_ca (sk);
net/ipv4/tcp_roccet.c line 389
Quoted 10 lines
net/ipv4/tcp_roccet.c new file +10
-0+ +u32 now =jiffies_to_usecs (tcp_jiffies32); +bool evaluate_srrtt = false; +u32 roccet_xj; +u32 jitter; + +if (after (ca->last_rtt, ca->curr_rtt)) + jitter = ca->last_rtt - ca->curr_rtt; +else + jitter = ca->curr_rtt - ca->last_rtt;
[Low]after() is the TCP sequence-number macro from include/net/tcp.h. Using it here for an RTT-microsecond comparison works (it expands to a signed 32-bit subtraction), but it is a sequence-space helper and reads as a layering mistake. The same file uses raw subtraction with <= a few lines below ("now - ca->roccet_last_event_time_us <= 100 * USEC_PER_MSEC"), so the time arithmetic is internally inconsistent. Could the after() uses be replaced with explicit (s32) subtractions?
[Low]
On the very first call after a reset, ca->last_rtt is 0, so this computes jitter = |curr_rtt - 0| = curr_rtt, i.e. an entire RTT worth of "jitter". That value then feeds roccet_xj and loosens the bufferbloat-detection threshold during the LAUNCH phase the algorithm is supposed to keep tightest. Should the first sample be suppressed (e.g., skip jitter computation while last_rtt == 0)?
[ ... ]
net/ipv4/tcp_roccet.c line 457
Quoted 4 lines
net/ipv4/tcp_roccet.c new file +4
-0+ roccet_xj = ((jitter *100 ) / ca->curr_min_rtt) + + sr_rtt_upper_bound; +if (roccet_xj < sr_rtt_upper_bound) + roccet_xj = sr_rtt_upper_bound;
[Medium]
Same overflow class as update_srrtt(): jitter * 100 is computed in u32. With jitter above ~42.9 ms in microseconds the multiplication wraps, and the subsequent clamp "if (roccet_xj < sr_rtt_upper_bound)" only catches a subset of the wrap cases. The wrapped value drives whether the cwnd-reduction branch fires. Should jitter * 100 be performed in u64?
[ ... ]
Quoted 5 lines
net/ipv4/tcp_roccet.c new file +5
-0+static int __init roccettcp_register (void ) +{ +int ret; + +BUILD_BUG_ON (sizeof (struct roccettcp ) >ICSK_CA_PRIV_SIZE );
[ ... ]
Quoted 2 lines
net/ipv4/tcp_roccet.c new file +2
-0+ beta_scale = +8 * (BICTCP_BETA_SCALE + beta) /3 / (BICTCP_BETA_SCALE - beta);
[ ... ]
net/ipv4/tcp_roccet.c line 627
Quoted 2 lines
net/ipv4/tcp_roccet.c new file +2
-0+/* divide by bic_scale and by constant Srtt (100ms) */ +do_div (cube_factor, bic_scale *10 );
[Low]
Can these divides panic at module load if bic_scale=0 or beta=1024 is passed via modprobe? do_div(cube_factor, bic_scale * 10) divides by 0 when bic_scale is 0, and beta_scale divides by (BICTCP_BETA_SCALE - beta) which is 0 when beta is 1024. The same shape exists in tcp_cubic.c, but should the new file sanity-check these inputs before computing the precomputed factors?
[Low]MODULE_VERSION("1.0") is more typical of out-of-tree DKMS modules than of in-tree code; should it be dropped here?
Signature · 1 line
pw-bot: cr