Re: [PATCH net-next v16 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: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] quic_cong_on_packet_lost() (net/quic/cong.c) lets a packet
  that was sent before the first RTT sample start the persistent-…
- [Medium] The New Reno handlers in net/quic/cong.c decide whether to
  react to loss or ECN using only cong->state.

--

quic: add congestion control

This patch adds struct quic_cong for RTT measurement and congestion
control, and a quic_cong_ops interface for congestion control
algorithms. It implements the RFC 9002 state machine (slow start,
congestion avoidance, recovery) with New Reno as the only algorithm. It
also adds persistent congestion detection and pacing.

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

[ ... ]

> +/* NEW RENO APIs */
> +static void quic_reno_handle_packet_lost(struct quic_cong *cong)
> +{
> +	switch (cong->state) {
> +	case QUIC_CONG_SLOW_START:
> +		pr_debug("%s: slow_start -> recovery, cwnd: %u, ssth: %u\n",
> +			 __func__, cong->window, cong->ssthresh);
> +		break;
> +	case QUIC_CONG_RECOVERY_PERIOD:
> +		return;
> +	case QUIC_CONG_CONGESTION_AVOIDANCE:

[ ... ]

> +	cong->recovery_time = cong->time;
> +	cong->state = QUIC_CONG_RECOVERY_PERIOD;
> +	cong->ssthresh = max(cong->window >> 1U, cong->min_window);
> +	cong->window = cong->ssthresh;
> +}
> +
> +static void quic_reno_on_packet_lost(struct quic_cong *cong, u64 time,
> +				     u32 bytes, s64 number)
> +{
> +	quic_reno_handle_packet_lost(cong);
> +}

[Severity: Medium]
Can one congestion event halve the window twice here?
quic_reno_on_packet_lost() gets the lost packet's send time in time, but
never uses it. quic_reno_handle_packet_lost() skips the reduction only
when cong->state is QUIC_CONG_RECOVERY_PERIOD.

In quic_reno_on_packet_acked(), the state leaves recovery as soon as one
packet sent after recovery_time is acked:

	case QUIC_CONG_RECOVERY_PERIOD:
		if (cong->recovery_time >= time)
			break;
		cong->state = QUIC_CONG_CONGESTION_AVOIDANCE;
		...
		fallthrough;

Here is one sequence:

  - A loss at time T starts recovery.
  - P20 was sent just before T and is lost in the network.
  - An ACK of P21, sent after T, ends recovery.

Later, P20 is declared lost, either by packet threshold on a later ACK
or by the time-threshold loss timer. At that point the state is
QUIC_CONG_CONGESTION_AVOIDANCE. A new recovery period starts and both
ssthresh and window are halved again.

In RFC 9002 Appendix B.6, OnCongestionEvent(sent_time) returns early
when sent_time <= congestion_recovery_start_time. B.7 applies the same
rule to ECN-CE.

Should quic_reno_on_packet_lost() compare time against
cong->recovery_time instead of relying only on cong->state?
quic_reno_on_process_ecn() gets no send time at all, so this check can't
be done there either.

The ACK side has a similar gap. In B.5, acked packets sent before the
recovery start don't grow cwnd. In QUIC_CONG_CONGESTION_AVOIDANCE,
quic_reno_on_packet_acked() grows the window for every acked packet,
whatever its send time.

[ ... ]

> +/* COMMON APIs */
> +void quic_cong_on_packet_lost(struct quic_cong *cong, u64 time, u32 bytes,
> +			      s64 number)
> +{
> +	if (cong->pc_detected)
> +		return;
> +
> +	if (cong->pc_start_time && time > cong->pc_start_time &&
> +	    quic_cong_check_persistent_congestion(cong, time)) {
> +		cong->pc_detected = 1;
> +		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 a packet sent before the first RTT sample start the persistent
congestion period here? time is the lost packet's send time. is_rtt_set
shows the state when the loss is detected, not when the packet was sent.

RFC 9002 section 7.6.2 says the persistent congestion period should not
start until there is at least one RTT sample. Appendix B.8 only looks at
lost packets with lost.time_sent > first_rtt_sample.

struct quic_cong has no field for the time of the first RTT sample.
quic_cong_rtt_update() sets cong->is_rtt_set = 1 but doesn't record
when.

Here is an example:

  - A client sends Initial PN0 at T0.
  - It sends a PTO probe PN1 at about T0 + 1s. The PTO from
    QUIC_RTT_INIT is about 999ms.
  - Both packets are lost.
  - A later probe, PN2, is acked and gives a first RTT sample of 50ms.

Loss detection on that ACK marks PN0 lost. is_rtt_set is now 1, so
PN0's send time becomes pc_start_time.

PN1 is then marked lost. The gap of about 1s is larger than
(50 + max(4 * 25, 1) + 25) * 3 = 525ms. So the persistent congestion
branch above runs, cwnd drops to 2 * mss, and min_rtt_valid is cleared,
right after the handshake.

Should quic_cong_rtt_update() record the time of the first RTT sample?
quic_cong_on_packet_lost() could then use only lost packets sent after
that time.

[ ... ]

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