From: Xin Long <lucien.xin@gmail.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: hepengtao@xiaomi.com, kuba@kernel.org, jlayton@kernel.org,
metze@samba.org, davem@davemloft.net,
kernel-tls-handshake@lists.linux.dev, horms@kernel.org,
hare@suse.de, aahringo@redhat.com, tfanelli@redhat.com,
mail@johnericson.me, andrew.gospodarek@broadcom.com,
linkinjeon@kernel.org, dhowells@redhat.com, matttbe@kernel.org,
xiyou.wangcong@gmail.com, dreibh@simula.no, jbaron@akamai.com,
quic@lists.linux.dev, pc@manguebit.com, marc@fiuczynski.com,
edumazet@google.com, alibuda@linux.alibaba.com,
steved@redhat.com, illiliti@protonmail.com, tom@talpey.com,
marcelo.leitner@gmail.com, sd@queasysnail.net,
linux-cifs@vger.kernel.org, netdev@vger.kernel.org,
mbuhl@openbsd.org, smfrench@gmail.com, chuck.lever@oracle.com,
daniel@haxx.se
Subject: Re: [net-next,v10,09/15] quic: add congestion control
Date: Wed, 4 Mar 2026 16:41:59 -0500 [thread overview]
Message-ID: <CADvbK_cKefZpb=yCi_yxVWX4ear96LpCb+9W7pXs_thRWCWB2g@mail.gmail.com> (raw)
In-Reply-To: <20260303083207.34631-1-pabeni@redhat.com>
On Tue, Mar 3, 2026 at 3:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> quic: add congestion control
>
> This patch introduces quic_cong for RTT measurement and congestion
> control, implementing a state machine with slow start, congestion
> avoidance, and recovery phases using the New Reno algorithm.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> > diff --git a/net/quic/Makefile b/net/quic/Makefile
> > index 1565fb5cef9d..4d4a42c6d565 100644
> > --- a/net/quic/Makefile
> > +++ b/net/quic/Makefile
> > @@ -5,4 +5,5 @@
> >
> > obj-$(CONFIG_IP_QUIC) += quic.o
> >
> > -quic-y := common.o family.o protocol.o socket.o stream.o connid.o path.o
> > +quic-y := common.o family.o protocol.o socket.o stream.o connid.o path.o \
> > + cong.o
> > diff --git a/net/quic/cong.c b/net/quic/cong.c
> > new file mode 100644
> > index 000000000000..e0f78ecb00e4
> > --- /dev/null
> > +++ b/net/quic/cong.c
>
> [ ... ]
>
> > +static void quic_reno_on_packet_lost(struct quic_cong *cong, u64 time, u32 bytes, s64 number)
> > +{
> > + if (quic_cong_check_persistent_congestion(cong, time))
> > + return;
> > +
> > + switch (cong->state) {
> > + case QUIC_CONG_SLOW_START:
> > + pr_debug("%s: slow_start -> recovery, cwnd: %u, ssthresh: %u\n",
> > + __func__, cong->window, cong->ssthresh);
> > + break;
> > + case QUIC_CONG_RECOVERY_PERIOD:
> > + return;
> > + case QUIC_CONG_CONGESTION_AVOIDANCE:
> > + pr_debug("%s: cong_avoid -> recovery, cwnd: %u, ssthresh: %u\n",
> > + __func__, cong->window, cong->ssthresh);
> > + break;
> > + default:
> > + pr_debug("%s: wrong congestion state: %d\n", __func__, cong->state);
> > + return;
> > + }
> > +
> > + 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_process_ecn(struct quic_cong *cong)
> > +{
> > + switch (cong->state) {
> > + case QUIC_CONG_SLOW_START:
> > + pr_debug("%s: slow_start -> recovery, cwnd: %u, ssthresh: %u\n",
> > + __func__, cong->window, cong->ssthresh);
> > + break;
> > + case QUIC_CONG_RECOVERY_PERIOD:
> > + return;
> > + case QUIC_CONG_CONGESTION_AVOIDANCE:
> > + pr_debug("%s: cong_avoid -> recovery, cwnd: %u, ssthresh: %u\n",
> > + __func__, cong->window, cong->ssthresh);
> > + break;
> > + default:
> > + pr_debug("%s: wrong congestion state: %d\n", __func__, cong->state);
> > + return;
> > + }
> > +
> > + cong->recovery_time = cong->time;
> > + cong->state = QUIC_CONG_RECOVERY_PERIOD;
> > + cong->ssthresh = max(cong->window >> 1U, cong->min_window);
> > + cong->window = cong->ssthresh;
> > +}
>
> This isn't a bug, but these two functions share almost identical logic
> for entering the recovery state. The switch statement has the same three
> cases with identical handling in both functions, and the four-line
> recovery-entry block at the end is exactly the same:
>
> cong->recovery_time = cong->time;
> cong->state = QUIC_CONG_RECOVERY_PERIOD;
> cong->ssthresh = max(cong->window >> 1U, cong->min_window);
> cong->window = cong->ssthresh;
>
> The only difference is quic_reno_on_packet_lost() checks for persistent
> congestion first. Would factoring this shared logic into a helper
> function make future maintenance easier and ensure the recovery path
> stays consistent across both ECN and loss events?
>
I will add a helper quic_reno_handle_packet_lost() for this.
> [ ... ]
>
> > +void quic_cong_set_algo(struct quic_cong *cong, u8 algo)
> > +{
> > + /* The caller must ensure algo < QUIC_CONG_ALG_MAX. */
> > + cong->algo = algo;
> > + cong->state = QUIC_CONG_SLOW_START;
> > + cong->ssthresh = U32_MAX;
> > + cong->ops = &quic_congs[algo];
> > + cong->ops->on_init(cong);
> > +}
>
> Should this function validate the algo parameter? The comment says the
> caller must ensure algo is less than QUIC_CONG_ALG_MAX, but there's no
> runtime check. Since QUIC_CONG_ALG_MAX is defined in the UAPI and this
> function is exported in cong.h, could a future caller (for example, a
> setsockopt path accepting user-supplied algorithm choices) pass an
> out-of-range value and cause an out-of-bounds array access into
> quic_congs[]?
>
The callers will do the validation as the comment said, hopefully AI will not
flag this again.
Thanks.
next prev parent reply other threads:[~2026-03-04 21:42 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-25 2:34 [PATCH net-next v10 00/15] net: introduce QUIC infrastructure and core subcomponents Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 01/15] net: define IPPROTO_QUIC and SOL_QUIC constants Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 02/15] net: build socket infrastructure for QUIC protocol Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 03/15] quic: provide common utilities and data structures Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 04/15] quic: provide family ops for address and protocol Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 05/15] quic: provide quic.h header files for kernel and userspace Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 06/15] quic: add stream management Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 07/15] quic: add connection id management Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 08/15] quic: add path management Xin Long
2026-03-03 8:22 ` Paolo Abeni
2026-03-04 21:25 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 09/15] quic: add congestion control Xin Long
2026-03-03 8:32 ` [net-next,v10,09/15] " Paolo Abeni
2026-03-04 21:41 ` Xin Long [this message]
2026-02-25 2:34 ` [PATCH net-next v10 10/15] quic: add packet number space Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 11/15] quic: add crypto key derivation and installation Xin Long
2026-03-03 8:32 ` [net-next,v10,11/15] " Paolo Abeni
2026-03-04 21:58 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 12/15] quic: add crypto packet encryption and decryption Xin Long
2026-03-03 8:32 ` [net-next,v10,12/15] " Paolo Abeni
2026-03-04 22:31 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 13/15] quic: add timer management Xin Long
2026-03-03 8:33 ` [net-next,v10,13/15] " Paolo Abeni
2026-03-04 23:03 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 14/15] quic: add packet builder base Xin Long
2026-03-03 8:33 ` [net-next,v10,14/15] " Paolo Abeni
2026-03-04 23:13 ` Xin Long
2026-03-03 9:18 ` [PATCH net-next v10 14/15] " Paolo Abeni
2026-03-04 23:26 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 15/15] quic: add packet parser base Xin Long
2026-03-03 8:33 ` [net-next,v10,15/15] " Paolo Abeni
2026-03-04 23:37 ` Xin Long
2026-03-03 9:16 ` [PATCH net-next v10 15/15] " Paolo Abeni
2026-03-05 0:14 ` Xin Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CADvbK_cKefZpb=yCi_yxVWX4ear96LpCb+9W7pXs_thRWCWB2g@mail.gmail.com' \
--to=lucien.xin@gmail.com \
--cc=aahringo@redhat.com \
--cc=alibuda@linux.alibaba.com \
--cc=andrew.gospodarek@broadcom.com \
--cc=chuck.lever@oracle.com \
--cc=daniel@haxx.se \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=dreibh@simula.no \
--cc=edumazet@google.com \
--cc=hare@suse.de \
--cc=hepengtao@xiaomi.com \
--cc=horms@kernel.org \
--cc=illiliti@protonmail.com \
--cc=jbaron@akamai.com \
--cc=jlayton@kernel.org \
--cc=kernel-tls-handshake@lists.linux.dev \
--cc=kuba@kernel.org \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=mail@johnericson.me \
--cc=marc@fiuczynski.com \
--cc=marcelo.leitner@gmail.com \
--cc=matttbe@kernel.org \
--cc=mbuhl@openbsd.org \
--cc=metze@samba.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pc@manguebit.com \
--cc=quic@lists.linux.dev \
--cc=sd@queasysnail.net \
--cc=smfrench@gmail.com \
--cc=steved@redhat.com \
--cc=tfanelli@redhat.com \
--cc=tom@talpey.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox