Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
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_reno_on_packet_acked() (net/quic/cong.c) grows
cong->window on every acknowledged packet, in slow start and in…
- [Low] The commit message does not describe the pacing logic this patch
adds, and it overstates what happens on transmit.
--
quic: add congestion control
This commit adds struct quic_cong for RTT measurement and congestion
control, with struct quic_cong_ops as the hook interface for
congestion control algorithms. Only New Reno is implemented. The
patch also adds pacing rate and pacing time helpers.
The implementation updates RTT estimates when packets are acknowledged,
reacts to loss and ECN signals, and adjusts the congestion window
accordingly during packet transmission or acknowledgment processing.
[Severity: Low]
Is this accurate for the transmit side? quic_cong_on_packet_sent() never
changes cong->window. New Reno leaves ops->on_packet_sent as NULL, so the
transmit path only calls quic_cong_update_pacing_time().
The commit body also leaves out the pacing logic this patch adds:
- quic_cong_pace_update() sets pacing_rate = 2 * window / smoothed_rtt,
capped at max_rate.
- quic_cong_update_pacing_time() schedules the next send in nanoseconds
using ktime_get_ns(), with a jitter credit of up to len_ns / 2.
The other timestamps in struct quic_cong are in microseconds.
The function list also leaves out quic_cong_pace_update(),
quic_cong_set_srtt() and quic_cong_set_algo(). Pacing is only mentioned
in the v5/v16 changelog notes, and those are dropped when the patch is
applied.
Could the commit message describe the pacing behavior, and drop the
claim that the window is adjusted on transmit?
quoted hunk ↗ jump to hunk
diff --git a/net/quic/cong.c b/net/quic/cong.c
new file mode 100644
index 0000000000000..b829755e8ac9e
--- /dev/null
+++ b/net/quic/cong.c
[ ... ]
quoted hunk ↗ jump to hunk
+static void quic_reno_on_packet_acked(struct quic_cong *cong, u64 time,
+ u32 bytes, s64 number)
+{
+ u64 new_window;
+
+ if (quic_cong_in_slow_start(cong)) { /* Slow start */
+ new_window = (u64)cong->window + bytes;
+ cong->window = min_t(u64, new_window, cong->max_window);
+ return;
+ }
+ /* Congestion avoidance - cong->window is never zero; it is initialized
+ * by quic_packet_route() during connect/accept.
+ */
+ new_window = div64_ul((u64)cong->mss * bytes, cong->window) +
+ cong->window;
+ cong->window = min_t(u64, new_window, cong->max_window);
+}
[ ... ]
quoted hunk ↗ jump to hunk
+void quic_cong_on_packet_acked(struct quic_cong *cong, u64 time, u32 bytes,
+ s64 number)
+{
+ if (quic_cong_in_recovery(cong, time))
+ return;
+ cong->ops->on_packet_acked(cong, time, bytes, number);
+}
[Severity: Medium]
Can cong->window keep growing while the sender is application-limited
or flow-control-limited?
RFC 9002 section 7.8 says the window should not grow when it is
underused. The OnPacketAcked() pseudocode in Appendix B.5 has:
if (IsAppOrFlowControlLimited())
return
quic_cong_on_packet_acked() only checks quic_cong_in_recovery() before
it calls ops->on_packet_acked(). struct quic_cong has no bytes-in-flight
field, and the hooks never receive one, so neither the wrapper nor
quic_reno_on_packet_acked() can apply this check.
In slow start the window grows by bytes on every ACK. A long-lived
connection that sends small request/response traffic could push
cong->window up by roughly the total bytes ever acked, toward
max_window (S32_MAX / 4, about 512MB).
quic_cong_pace_update() then computes the pacing rate from that window:
rate = div64_ul((u64)cong->window * USEC_PER_SEC * 2,
cong->smoothed_rtt);
When bulk data arrives, would the burst then be limited only by the
caller's max_rate?
Nothing in this series calls quic_cong_on_packet_acked() yet, so a later
caller could skip the call when app-limited. If that is the plan, could
the requirement be documented here? For comparison, TCP does this check
inside the algorithm: tcp_reno_cong_avoid() checks
tcp_is_cwnd_limited().
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794241654.git.lucien.xin%40gmail.com