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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tcp: ROCCET stalls Fast Retransmit by bypassing PRR in TCP_CA_Recovery
- [High] tcp: integer promotion bug falsely triggers congestion events on ROCCET
- [Low] build: struct roccettcp exceeds ICSK_CA_PRIV_SIZE limit
--

commit 1022dd98954ad71bd010ccb13665b66dd4141109
Author: Tim Fuechsel <t.fuechsel@gmx.de>

tcp: Add TCP ROCCET congestion control module.

TCP ROCCET is a new congestion control algorithm based on TCP CUBIC that
improves overall performance in cellular networks. It introduces additional
metrics like relative increase in RTT and acknowledgment arrival rate to
address bufferbloat and manage window scaling.

> diff --git a/net/ipv4/tcp_roccet.c b/net/ipv4/tcp_roccet.c
> new file mode 100644
> --- /dev/null
> +++ b/net/ipv4/tcp_roccet.c
> @@ -0,0 +1,1400 @@
[ ... ]
> +struct roccettcp {
> +	u32 cnt;	/* increase cwnd by 1 after ACKs */
[ ... ]
> +	u16 ack_rate_cnt;	/* Used for counting acks */
> +
> +	enum roccet_state state : 3; /* Current operating state of roccet */

[Severity: Low]
Will this bitfield layout cause a compile-time failure?

Because the preceding field is u16 (which ends at offset 102), the compiler
inserts 2 bytes of padding to align the enum allocation unit, which needs
4-byte alignment, at offset 104. This expands the total struct size to 108
bytes.

> +	bool initial_limit_reached: 1;	/* Set to true after the connection
[ ... ]
> +static int __init roccet_register(void)
> +{
> +	int param_err;
> +
> +	BUILD_BUG_ON(sizeof(struct roccettcp) > ICSK_CA_PRIV_SIZE);

Since ICSK_CA_PRIV_SIZE is statically defined as 104 bytes in
include/net/inet_connection_sock.h, it seems this check will unconditionally
fail and break the module build.

[ ... ]
> +static void roccet_orbiter_update(struct sock *sk, u32 acked)
> +{
[ ... ]
> +	/* Check sent and received bytes from the previous interval.
> +	 * Here we use a guard space of 1% of the current cwnd.
> +	 * We do this to avoid a false positive evaluation due
> +	 * to delays caused by jitter or scheduling.
> +	 */
> +	sent_more_than_acked = (s32)(sent - received) >
> +			       div_u64((u64)tcp_snd_cwnd(tp) *
> +			       (u64)tp->mss_cache, 100);

[Severity: High]
Does this comparison trigger false congestion events when ACKs outpace outgoing
data?

If received exceeds sent, sent - received wraps to a very large u32. When
cast to s32, it correctly represents a negative difference.

However, because div_u64() returns a u64, standard C arithmetic conversions
cause the negative s32 to be sign-extended and promoted to u64.

This results in a huge positive number, making the condition incorrectly
evaluate to true, which freezes congestion window growth.

[ ... ]
> +	/* Terminates this function if cwnd is not fully utilized.
> +	 * In mobile networks like 5G, this termination causes the
> +	 * cwnd to be frozen at an excessively high value. This is
> +	 * because slow start or HyStart massively exceed the available
> +	 * bandwidth and leave the cwnd at an excessively high value.
> +	 * The cwnd cannot therefore be fully utilized because it is
> +	 * limited by the connection capacity.
> +	 *
> +	 * Also avoid cwnd-growth if we are in cwnd reduction phase.
> +	 */
> +	if (!tcp_is_cwnd_limited(sk) || sent_more_than_acked ||
> +	    tcp_in_cwnd_reduction(sk))
> +		return;

[Severity: High]
Can returning early here during TCP_CA_Recovery stall Fast Retransmit?

When a socket enters loss recovery, the TCP core delegates all congestion
control processing to the cong_control callback, bypassing the standard
tcp_cwnd_reduction() PRR routine.

If roccet_orbiter_update() always returns early without inflating snd_cwnd
for incoming dupACKs:

[ ... ]
> +static void roccet_state(struct sock *sk, u8 new_state)
> +{
[ ... ]
> +	} else if (new_state == TCP_CA_Recovery) {
> +		/* Directly reduce cwnd and rely on pacing */
> +		cwnd = roccet_handle_recovery(sk);
> +		tcp_snd_cwnd_set(tp, cwnd);
> +	}

The snd_cwnd is statically reduced upon entering recovery. Because it is
never inflated by dupACKs, tcp_packets_in_flight() will remain above the
reduced snd_cwnd.

When tcp_xmit_retransmit_queue() attempts to fast retransmit, it calculates
available segments as tcp_snd_cwnd(tp) - tcp_packets_in_flight(tp) which
evaluates to <= 0, completely preventing retransmissions until an RTO fires.

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