tcp: Add TCP ROCCET congestion control module.

net/ipv4/tcp_roccet.c line 897

[Severity: Medium]
Could this condition falsely trigger a premature RTT probe if the connection was previously idle?

In roccet_enter_min_rtt_probe(), ca->next_min_rtt_probe is assigned a raw jiffies timestamp to track the probe epoch. Normal state transitions out of probing explicitly reinitialize next_min_rtt_probe to a usecs timestamp.

If the connection then enters ORBITER from LAUNCH, this block compares a usecs timestamp (now) against the residual jiffies timestamp. Since usecs are vastly larger, won't this check instantly fail and incorrectly re-enter probing?

[ ... ]

net/ipv4/tcp_roccet.c line 1126

[Severity: Medium]
When the connection goes idle during probing, roccet_cwnd_event_tx_start() forces an immediate transition to LAUNCH. This cleanup logic handles clearing the probe timers, but does it miss reinitializing next_min_rtt_probe to usecs for that transition?

[ ... ]

net/ipv4/tcp_roccet.c line 1385

[Severity: High]
Does this assertion fail unconditionally because the struct exceeds 104 bytes?

struct roccettcp contains 24 u32 fields (96 bytes) and 3 u16 fields (6 bytes), consuming 102 bytes. The next field is the enum roccet_state bitfield, which forces 2 bytes of padding after the u16 fields, placing the bitfield block at offset 104, pushing the total struct size to 108 bytes.

Since ICSK_CA_PRIV_SIZE is exactly 104 bytes, won't this break compilation on standard architectures?

Sashiko AI review · https://sashiko.dev/#/patchset/asdfdDIQb7Nhm692@volt-roccet-vm?part=1

net/ipv4/tcp_roccet.c line 98

[Severity: Low]
This isn't a bug, but these comments describe per-round samples. roccet_acked() actually shifts curr_rtt into last_rtt on every accepted ACK sample. It does not look for round boundaries:

	ca->last_rtt = ca->curr_rtt;
	ca->curr_rtt = sample->rtt_us;

So the jitter that roccet_orbiter_update() uses for roccet_xj is the difference between consecutive ACK samples, not between rounds.

Should the comments be updated, or was a per-round sample intended?

[ ... ]

net/ipv4/tcp_roccet.c line 184

[Severity: Low]
Do the two parses here agree? The candidate is checked with kstrtoint(val, 10, ...). param_set_int() comes from STANDARD_PARAM_DEF() in kernel/params.c and parses with base 0:

	return strtolfn(val, 0, (type *)kp->arg);

So beta=0717 passes the check as decimal 717 but is stored as octal 463. beta=0x2cd is rejected, even though param_set_int() would accept it.

The stored value stays in range, but it can differ from the value that was checked. Could this store beta_candidate directly, or parse with the same base in both places?

[ ... ]

net/ipv4/tcp_roccet.c line 462

[Severity: Medium]
Can a spurious recovery during RTT_PROBE leave a lasting reduction through this saved window?

Take cwnd_before_min_rtt_probe = 100 and a live cwnd of 50 during the probe. Entering fast recovery then does:

tcp_enter_recovery()

    tcp_init_cwnd_reduction()
        tp->prior_cwnd = 50
        roccet_recalc_ssthresh()   <- uses cwnd_before_min_rtt_probe (100)
    tcp_set_ca_state(sk, TCP_CA_Recovery)
        roccet_state()
            roccet_handle_recovery()
                roccet_min_rtt_probe_ce()
                    cwnd_before_min_rtt_probe = 100 * 717 / 1024 = 70

Suppose DSACK later shows the recovery was spurious. tcp_undo_cwnd_reduction() calls tcp_reno_undo_cwnd(), which returns max(cwnd, prior_cwnd) = 50. It never touches cwnd_before_min_rtt_probe.

When the probe ends, roccet_min_rtt_probe() installs the reduced value:

	tcp_snd_cwnd_set(tp, min(ca->cwnd_before_min_rtt_probe,
				 tp->snd_cwnd_clamp));

So cwnd becomes 70 instead of 100. An undo after the probe ends can't get back to 100 either, because prior_cwnd holds 50.

Should the undo path also restore the saved probe window?

[ ... ]

net/ipv4/tcp_roccet.c line 537

[Severity: Medium]
This stores a jiffies value in a field that otherwise holds a usec deadline. roccet_set_next_min_rtt_probe_ts() won't update it while the state is RTT_PROBE or RTT_PROBE_REFILL.

roccet_handle_state_transitions() converts it back to a usec deadline in only two cases: RTT_PROBE or RTT_PROBE_REFILL to ORBITER, and RTT_PROBE_REFILL to DRAIN.

What happens if the application is idle for longer than an RTO during the probe or refill window? roccet_cwnd_event_tx_start() then moves straight to LAUNCH:

		ca->state = LAUNCH;

Only the probe timers are cleared on that transition, so next_min_rtt_probe keeps the jiffies value.

Later LAUNCH to ORBITER or DRAIN to ORBITER transitions don't take the prev_state == RTT_PROBE* branch. roccet_orbiter_update() then compares a usec now against a jiffies deadline:

	if (!time_between32(now, ca->next_min_rtt_probe
				 - 2 * ROCCET_NEXT_MIN_RTT_PROBE_MS
				 * USEC_PER_MSEC, ca->next_min_rtt_probe) &&
	    ca->curr_rtt != U32_MAX) {
		ca->state = RTT_PROBE_ENTER;

This is true for nearly every value of now. Wouldn't ROCCET then enter RTT_PROBE_ENTER right away and cut cwnd for at least 200 ms with no need to probe? The only way out seems to be update_min_rtt() finding a new minimum first.

On the same RTT_PROBE to LAUNCH path, ssthresh is also set from the reduced probe cwnd, and cwnd_before_min_rtt_probe is never applied. Is that intended?

[ ... ]

net/ipv4/tcp_roccet.c line 565

[Severity: Medium]
Can this raise cwnd during the drain probe?

In ORBITER, cwnd can be anywhere from 2 to 9 after roccet_congestion_event() or roccet_handle_recovery() reduce it to max(cwnd * beta / 1024, 2). probe_cwnd is not limited by the current cwnd, so it becomes TCP_INIT_CWND (10). The probe then sends more than before for at least 200 ms.

curr_min_rtt has just been reset to U32_MAX, so the new minimum is measured with more data in flight than before the probe. That can inflate curr_min_rtt and lower srRTT for the following probe period.

For 10 < cwnd < 20, the reduction is also smaller than the halving described in the commit message.

[ ... ]

net/ipv4/tcp_roccet.c line 1153

[Severity: Medium]
ROCCET sets .cong_control, so tcp_slow_start_after_idle_check() returns early:

	if (!READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_slow_start_after_idle) ||
	    tp->packets_out || ca_ops->cong_control)
		return;

That makes this the only restart-after-idle logic that runs. Should it follow what tcp_cwnd_restart() does?

	u32 restart_cwnd = tcp_init_cwnd(tp, __sk_dst_get(sk));
	...
	WRITE_ONCE(tp->snd_ssthresh, tcp_current_ssthresh(sk));
	restart_cwnd = min(restart_cwnd, cwnd);

Compared to that, this code differs in a few ways:

There is no min() against the current cwnd. A cwnd of 2 to 9, which is common after the beta reductions, is raised to 10 after idle.

The per-route initcwnd from tcp_init_cwnd() is ignored.

ssthresh is set to the old cwnd instead of tcp_current_ssthresh().

The trigger is delta >= icsk_rto, while the core uses delta > icsk_rto.

The comment says this goes into LAUNCH with slow start. But if the old cwnd is at most 10, ssthresh ends up at or below the new cwnd. Then tcp_in_slow_start() is false, and roccet_launch_update() leaves LAUNCH right away through its !tcp_in_slow_start(tp) branch. Is that intended?

[Severity: Medium]
Can this also fire on the RTO retransmission itself, and not only after a real idle period?

tcp_retransmit_timer()
tcp_enter_loss()
tcp_timeout_mark_lost() <- in_flight drops to 0
->ssthresh()
tcp_set_ca_state(sk, TCP_CA_Loss)
roccet_state() <- cwnd = 10, ssthresh = max(ssthresh, 10)
tcp_retransmit_skb()
__tcp_transmit_skb()
tcp_event_data_sent()
tcp_ca_event(sk, CA_EVENT_TX_START)
roccet_cwnd_event_tx_start()

tcp_event_data_sent() fires CA_EVENT_TX_START before it updates lsndtime:

	if (tcp_packets_in_flight(tp) == 0)
		tcp_ca_event(sk, CA_EVENT_TX_START);

	tp->lsndtime = now;

At that point lsndtime is the previous send, for example the TLP probe or the previous retransmission. The RTO timer was armed for icsk_rto from about that time, so delta >= icsk_rto holds.

With the default slow_start_after_idle=1, this replaces the ssthresh that roccet_state(TCP_CA_Loss) just kept with 10, and sets LAUNCH.

On the next ACK, roccet_launch_update() sees cwnd == ssthresh == 10 and leaves LAUNCH at once. CUBIC then grows again from 10 with last_max_cwnd = 0 (cleared by roccet_reset()), instead of slow starting back toward the old ssthresh.

The core tcp_slow_start_after_idle_check() avoids this case with its tp->packets_out test. Should this path also check packets_out or icsk_ca_state?

[Severity: Low]
Does the restart window apply too late for the first skb after idle?

This runs from CA_EVENT_TX_START inside __tcp_transmit_skb(). By that point, tcp_write_xmit() has already sized the skb using the pre-idle cwnd:

tcp_write_xmit()

    cwnd_quota = tcp_cwnd_test(tp);
    ...
    limit = tcp_mss_split_point(sk, skb, mss_now, cwnd_quota, nonagle);
    ...
    tcp_transmit_skb(sk, skb, 1, gfp)
        tcp_event_data_sent()
            roccet_cwnd_event_tx_start()   <- cwnd cut to 10 here

The core restart, tcp_skb_entail()->tcp_slow_start_after_idle_check(), runs before transmission, but it is skipped for cong_control modules.

So the first GSO packet after idle can carry far more than 10 segments. It is bounded only by tcp_tso_autosize() at the old pacing rate and by gso_max_segs. The pacing rate is not updated until the next ACK either.

[Severity: Low]
Should the ssthresh store here be
WRITE_ONCE(tp->snd_ssthresh, tcp_snd_cwnd(tp))?

tcp_get_timestamping_opt_stats() reads the field without a lock:

	nla_put_u32(stats, TCP_NLA_SND_SSTHRESH, READ_ONCE(tp->snd_ssthresh));

Every other snd_ssthresh store in this module uses WRITE_ONCE(), and KCSAN would likely report this plain store.

[ ... ]

net/ipv4/tcp_roccet.c line 1172

[Severity: Medium]
What happens to the ROCCET state if this RTO later turns out to be spurious?

roccet_reset() clears the private state, including the last_max_cwnd that roccet_recalc_ssthresh() just computed, and forces state = LAUNCH.

.undo_cwnd is tcp_reno_undo_cwnd. So after F-RTO or tcp_try_undo_loss(), tcp_undo_cwnd_reduction() restores only cwnd and ssthresh:

	tcp_snd_cwnd_set(tp, icsk->icsk_ca_ops->undo_cwnd(sk));
	if (tp->prior_ssthresh > tp->snd_ssthresh) {
		WRITE_ONCE(tp->snd_ssthresh, tp->prior_ssthresh);

The next set_state(TCP_CA_Open) matches none of the branches in roccet_state(), so ROCCET stays in LAUNCH.

tcp_fastretrans_alert() runs before tcp_cong_control() in tcp_ack(). So on the same ACK, roccet_launch_update() sees cwnd >= ssthresh. For a flow that was in ORBITER, prior_ssthresh is max(ssthresh, 3/4 cwnd), which is at most cwnd. It then takes the !tcp_in_slow_start(tp) exit:

		new_cwnd = tcp_snd_cwnd(tp) - (tcp_snd_cwnd(tp) / 3);

Doesn't this cut cwnd by a third right after the core undid the reduction?

[ ... ]

net/ipv4/tcp_roccet.c line 1219

[Severity: Medium]
Does this freeze curr_rtt and last_rtt for one second after every epoch start, not just after fast recovery?

In cubictcp_acked(), the same test only gates delay_min and HyStart:

	if (ca->epoch_start && (s32)(tcp_jiffies32 - ca->epoch_start) < HZ)
		return;

Here it also gates the curr_rtt and last_rtt updates. Only RTT_PROBE is exempt.

epoch_start is set to 0 on every LAUNCH exit, ROCCET congestion event, CWR and Recovery. The next bictcp_update() in ORBITER then sets it to tcp_jiffies32. roccet_cwnd_event_tx_start() also clamps it to now after idle. Each of these starts a one second window with no new RTT samples.

During that window, roccet_control() still runs the EWMA on every ACK using the same stale curr_rtt:

	if (rs->acked_sacked > 0) {
		update_min_rtt(ca);
		update_srrtt(ca);
	}

srRTT then converges toward the rRTT of a single stale sample, often the inflated RTT that caused the reduction. Jitter is frozen as well.

next_srrtt_check_ts is at most one second away. Wouldn't the next srRTT evaluation and roccet_xj in roccet_orbiter_update() be computed from this frozen data?

[ ... ]

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/asdfdDIQb7Nhm692%40volt-roccet-vm