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

[netdev-bot+sashiko@kernel.org]

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

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

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] When quic_cong_on_packet_lost() (net/quic/cong.c:94-99)
  declares persistent congestion, it sets cong->window =…
- [Low] The commit message says the implementation "adjusts the
  congestion window accordingly during packet transmission and…
- [Low] quic_cong_rtt_update() (net/quic/cong.c:207-209) throws away the
  whole RTT sample when the raw send-to-ACK interval (cong->time - time)…

--

quic: add congestion control

This patch adds struct quic_cong and struct quic_cong_ops to the QUIC
stack. They cover RTT measurement, loss and ECN handling, and pacing.
New Reno is the only algorithm so far, and quic_init_sock() sets it up
through quic_cong_init().

> 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]
Is this accurate for the transmit path? quic_cong_on_packet_sent() never
touches cong->window:

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

The New Reno entry in quic_congs[] has no .on_packet_sent, so on transmit
only cong->pacing_time changes.

The commit message body also doesn't mention pacing. That covers the
pacing_rate and pacing_time fields, the 2 * window / smoothed_rtt rate in
quic_cong_pace_update(), and quic_cong_update_pacing_time(). Pacing only
shows up in the version changelog notes.

Could the commit message describe pacing, and drop the part about
adjusting the window during transmission?

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

[ ... ]

> +/* NEW RENO APIs */
> +static void reno_recovery(struct quic_cong *cong)
> +{
> +	cong->recovery_time = cong->time;
> +	cong->ssthresh = max(cong->window >> 1U, cong->min_window);
> +	cong->window = cong->ssthresh;
> +}

[ ... ]

> +/* 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;
> +		return;
> +	}

[Severity: Medium]
Should this branch also reset cong->recovery_time?

The recovery state comes only from quic_cong_in_recovery():

    return time <= cong->recovery_time;

The only place that writes recovery_time is reno_recovery(), which sets
it to cong->time (now).

Take a single loss-detection pass after a link outage:

  P1 lost (pc_start_time is 0, first_rtt_time is set, not in recovery)
    quic_cong_on_packet_lost()
      cong->pc_start_time = time of P1
      quic_reno_on_packet_lost()
        reno_recovery()
          cong->recovery_time = cong->time;    /* now */

  P2 lost, sent more than the persistent congestion duration after P1
    quic_cong_on_packet_lost()
      quic_cong_check_persistent_congestion() returns true
      cong->window = cong->min_window;

Every packet still in flight was sent before now, so the ACK for each of
them returns early here:

    quic_cong_on_packet_acked()
        if (quic_cong_in_recovery(cong, time))
            return;

In that case ops->on_packet_acked() is never called. Would the window
then stay at min_window for at least one more RTT, until a packet sent
after recovery_time is acked?

RFC 9002 Appendix B.8 sets congestion_recovery_start_time to 0 on
persistent congestion, so these ACKs grow the window in slow start. The
state diagram in cong.h shows the same transition:

 *             persistent congestion |   Slow     |
 *         (O)---------------------->|   Start    |

Here, though, the derived state ends up as Recovery Period, not Slow
Start.

Nothing in this patch calls the quic_cong_on_packet_* hooks yet. This
would show up once the outqueue changes start calling them.

[ ... ]

> +/* rfc9002#section-5: Estimating the Round-Trip Time */
> +void quic_cong_rtt_update(struct quic_cong *cong, u64 time, u32 ack_delay)
> +{
> +	u32 adjusted_rtt, rttvar_sample;
> +
> +	/* Ignore RTT sample if ACK delay is suspiciously large. */
> +	if (ack_delay > cong->max_ack_delay * 2 ||
> +	    cong->time - time > QUIC_RTT_MAX)
> +		return;

[Severity: Low]
Is the 6 second QUIC_RTT_MAX cutoff too tight here?

cong->time - time is the raw interval from send to ACK, so it includes
the peer's ACK delay. RFC 9000 allows max_ack_delay up to 2^14 ms, which
matches common.h:

    #define QUIC_MAX_ACK_DELAY	(16384 * 1000)

For example, take a peer max_ack_delay of 8s, an ack_delay of 7s and a
measured interval of 7.1s. The sample passes the ack_delay check but is
still thrown away. The same happens on any path whose RTT stays above
6s.

The early return comes before anything is updated. smoothed_rtt and
rttvar stay at their initial 333ms and 166ms, and those feed into pto,
loss_delay and the pacing rate.

first_rtt_time also stays 0, so this condition in
quic_cong_on_packet_lost() is never true:

    if (!cong->pc_start_time && cong->first_rtt_time &&
        time > cong->first_rtt_time)
        cong->pc_start_time = time;

As a result, persistent congestion tracking never starts.

The v14 changelog says the cap prevents latest_rtt overflow. latest_rtt
is a u32 in microseconds, though, so it doesn't overflow until about
4295 seconds.

Could the limit be raised, or could ack_delay be subtracted before the
check? Nothing in this patch calls quic_cong_rtt_update() yet, so this
only matters once later patches call it.

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