Re: [PATCH net-next v15 09/15] quic: add congestion control

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Spurious persistent-congestion detection:
  quic_cong_on_packet_lost() (net/quic/cong.c:125-140) is the ONLY place…
- [Low] Commit message vs. what the patch actually enables: the message
  says the implementation 'adjusts the congestion window accordingly…

--

quic: add congestion control

This adds net/quic/cong.c and net/quic/cong.h with RTT estimation, a
congestion control state machine (slow start, recovery, congestion
avoidance), pacing helpers and the New Reno algorithm behind a
quic_cong_ops indirection.  quic_init_sock() now calls quic_cong_init().

A couple of questions below.

> This patch introduces 'quic_cong' for RTT measurement and congestion
> control. The 'quic_cong_ops' is added to define the congestion
> control algorithm.
[ ... ]
> The implementation updates RTT estimates when packets are acknowledged,
> reacts to loss and ECN signals, and adjusts the congestion window
> accordingly during packet transmission and acknowledgment processing.

[Severity: Low]
This isn't a bug, but does the description match what this patch alone
does?

None of the six documented entry points has a caller at this commit, and
quic_cong_on_packet_sent() only advances cong->pacing_time because New
Reno registers no on_packet_sent hook:

	if (cong->ops->on_packet_sent)
		cong->ops->on_packet_sent(cong, time, bytes, number);
	quic_cong_update_pacing_time(cong, bytes);

Similarly quic_cong_set_mss() in net/quic/cong.h, the only initializer of
cong->mss, cong->min_window and cong->window, has no caller yet, and
cong->time is never assigned here.  The comment in
quic_reno_on_packet_acked() also refers to quic_packet_route(), which is
only introduced by the later "quic: add packet builder base" patch:

		/* cong->window is never zero; it is initialized by
		 * quic_packet_route() during connect/accept.
		 */

Would something like "the congestion control infrastructure is added here
and wired up in <later patch>" describe this patch more accurately?

> diff --git a/net/quic/cong.c b/net/quic/cong.c
> new file mode 100644
> index 0000000000000..78f28bbdfb374
> --- /dev/null
> +++ b/net/quic/cong.c
> @@ -0,0 +1,340 @@

[ ... ]

> +static bool quic_cong_check_persistent_congestion(struct quic_cong *cong,
> +						  u64 time)
> +{
> +	u32 ssthresh;
> +
> +	time -= cong->pc_start_time;
> +
> +	/* rfc9002#section-7.6.1:
> +	 *   (smoothed_rtt + max(4*rttvar, kGranularity) + max_ack_delay) *
> +	 *      kPersistentCongestionThreshold
> +	 */
> +	ssthresh = cong->smoothed_rtt +
> +		   max(4 * cong->rttvar, QUIC_KGRANULARITY);
> +	ssthresh = (ssthresh + cong->max_ack_delay) *
> +		   QUIC_KPERSISTENT_CONGESTION_THRESHOLD;
> +
> +	return time > ssthresh;
> +}
> +
> +/* COMMON APIs */
> +void quic_cong_on_packet_lost(struct quic_cong *cong, u64 time, u32 bytes,
> +			      s64 number)
> +{
> +	if (cong->pc_start_time && time > cong->pc_start_time &&
> +	    quic_cong_check_persistent_congestion(cong, time)) {
> +		cong->pc_start_time = 0;
> +		cong->min_rtt_valid = 0;
> +		cong->window = cong->min_window;
> +		cong->state = QUIC_CONG_SLOW_START;
> +		return;
> +	}
> +
> +	if (!cong->pc_start_time && cong->is_rtt_set)
> +		cong->pc_start_time = time;
                ^^^^

[Severity: Medium]
Can persistent congestion be declared here even though a packet sent
inside the interval was acknowledged?

quic_cong_on_packet_lost() is the only place that establishes
cong->pc_start_time, and quic_cong_on_packet_acked() can only clear an
interval that already exists, so an acknowledgment processed before the
first loss callback leaves no state behind.

With srtt = 100ms the duration threshold is roughly 400ms.  Consider
packet A sent at t=0, B sent at t=100ms, C sent at t=500ms, and B
acknowledged first (ACK processing precedes loss detection):

quic_cong_on_packet_acked(time=100ms)
	/* pc_start_time == 0, so this guard is false */
	if (cong->pc_start_time && time > cong->pc_start_time &&
	    !quic_cong_check_persistent_congestion(cong, time))
		cong->pc_start_time = 0;

quic_cong_on_packet_lost(time=0ms)      /* A declared lost */
	if (!cong->pc_start_time && cong->is_rtt_set)
		cong->pc_start_time = time;     /* pc_start_time = 0ms */

quic_cong_on_packet_lost(time=500ms)    /* C declared lost */
	/* 500ms - 0ms > 400ms */
	quic_cong_check_persistent_congestion() == true
		cong->window = cong->min_window;
		cong->state = QUIC_CONG_SLOW_START;
		cong->min_rtt_valid = 0;

B was sent at 100ms, i.e. between the send times of A and C, and it was
acknowledged, which rfc9002#section-7.6.2 says should disqualify the
interval.  Since neither the acknowledgment timestamp nor the packet
number is retained (the number argument is unused in both callbacks), is
there a way to reject this interval, for example by recording the send
time or packet number of the most recently acknowledged packet?

> +
> +	cong->ops->on_packet_lost(cong, time, bytes, number);
> +}
> +
> +void quic_cong_on_packet_acked(struct quic_cong *cong, u64 time, u32 bytes,
> +			       s64 number)
> +{
[ ... ]
> +	if (cong->pc_start_time && time > cong->pc_start_time &&
> +	    !quic_cong_check_persistent_congestion(cong, time))
> +		cong->pc_start_time = 0;
> +
> +	cong->ops->on_packet_acked(cong, time, bytes, number);
> +}

[ ... ]

> +void quic_cong_on_packet_sent(struct quic_cong *cong, u64 time, u32 bytes,
> +			      s64 number)
> +{
> +	if (!bytes)
> +		return;
> +	if (cong->ops->on_packet_sent)
> +		cong->ops->on_packet_sent(cong, time, bytes, number);
> +	quic_cong_update_pacing_time(cong, bytes);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789393775.git.lucien.xin%40gmail.com