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,13/15] quic: add timer management
Date: Wed, 4 Mar 2026 18:03:14 -0500 [thread overview]
Message-ID: <CADvbK_f96OxCWnP9UdKxYtJ9=3xY69_ScTrU=9AQF1iUrBp_Cg@mail.gmail.com> (raw)
In-Reply-To: <20260303083324.35531-1-pabeni@redhat.com>
On Tue, Mar 3, 2026 at 3:33 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 timer management
>
> This patch introduces 'quic_timer' to unify and manage the five main
> timers used in QUIC: loss detection, delayed ACK, path validation,
> PMTU probing, and pacing. These timers are critical for driving
> retransmissions, connection liveness, and flow control.
>
> > diff --git a/net/quic/Makefile b/net/quic/Makefile
> > index 58bb18f7926d..2ccf01ad9e22 100644
> > --- a/net/quic/Makefile
> > +++ b/net/quic/Makefile
>
> [ ... ]
>
> > diff --git a/net/quic/socket.c b/net/quic/socket.c
> > index 74e3e3939c61..93a7abefc226 100644
> > --- a/net/quic/socket.c
> > +++ b/net/quic/socket.c
>
> [ ... ]
>
> > diff --git a/net/quic/socket.h b/net/quic/socket.h
> > index d7811391cc8b..c5654fdc06b5 100644
> > --- a/net/quic/socket.h
> > +++ b/net/quic/socket.h
>
> [ ... ]
>
> > diff --git a/net/quic/timer.c b/net/quic/timer.c
> > new file mode 100644
> > index 000000000000..6f957385a341
> > --- /dev/null
> > +++ b/net/quic/timer.c
> > @@ -0,0 +1,196 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/* QUIC kernel implementation
> > + * (C) Copyright Red Hat Corp. 2023
> > + *
> > + * This file is part of the QUIC kernel implementation
> > + *
> > + * Initialization/cleanup for QUIC protocol support.
> > + *
> > + * Written or modified by:
> > + * Xin Long <lucien.xin@gmail.com>
> > + */
> > +
> > +#include "socket.h"
>
> [ ... ]
>
> > +static void quic_timer_sack_timeout(struct timer_list *t)
> > +{
> > + struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_SACK].t);
> > + struct sock *sk = &qs->inet.sk;
> > +
> > + bh_lock_sock(sk);
> > + if (sock_owned_by_user(sk)) {
> > + if (!test_and_set_bit(QUIC_SACK_DEFERRED, &sk->sk_tsq_flags))
> > + sock_hold(sk);
> > + goto out;
> > + }
> > +
> > + quic_timer_sack_handler(sk);
> > +out:
> > + bh_unlock_sock(sk);
> > + sock_put(sk);
> > +}
>
> [ ... ]
>
> > +static void quic_timer_loss_timeout(struct timer_list *t)
> > +{
> > + struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_LOSS].t);
> > + struct sock *sk = &qs->inet.sk;
> > +
> > + bh_lock_sock(sk);
> > + if (sock_owned_by_user(sk)) {
> > + if (!test_and_set_bit(QUIC_LOSS_DEFERRED, &sk->sk_tsq_flags))
> > + sock_hold(sk);
> > + goto out;
> > + }
> > +
> > + quic_timer_loss_handler(sk);
> > +out:
> > + bh_unlock_sock(sk);
> > + sock_put(sk);
> > +}
>
> [ ... ]
>
> > +static void quic_timer_path_timeout(struct timer_list *t)
> > +{
> > + struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_PATH].t);
> > + struct sock *sk = &qs->inet.sk;
> > +
> > + bh_lock_sock(sk);
> > + if (sock_owned_by_user(sk)) {
> > + if (!test_and_set_bit(QUIC_PATH_DEFERRED, &sk->sk_tsq_flags))
> > + sock_hold(sk);
> > + goto out;
> > + }
> > +
> > + quic_timer_path_handler(sk);
> > +out:
> > + bh_unlock_sock(sk);
> > + sock_put(sk);
> > +}
>
> [ ... ]
>
> > +static void quic_timer_pmtu_timeout(struct timer_list *t)
> > +{
> > + struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_PMTU].t);
> > + struct sock *sk = &qs->inet.sk;
> > +
> > + bh_lock_sock(sk);
> > + if (sock_owned_by_user(sk)) {
> > + if (!test_and_set_bit(QUIC_PMTU_DEFERRED, &sk->sk_tsq_flags))
> > + sock_hold(sk);
> > + goto out;
> > + }
> > +
> > + quic_timer_pmtu_handler(sk);
> > +out:
> > + bh_unlock_sock(sk);
> > + sock_put(sk);
> > +}
>
> This isn't a bug, but the four timer_list-based timeout callbacks
> (quic_timer_sack_timeout, quic_timer_loss_timeout, quic_timer_path_timeout,
> quic_timer_pmtu_timeout) are structurally identical.
>
> Each extracts the sock via container_of, takes bh_lock_sock, checks
> sock_owned_by_user to conditionally defer via test_and_set_bit/sock_hold,
> otherwise calls the handler directly, then bh_unlock_sock/sock_put. They
> differ only in the timer index constant, the deferred bit name, and the
> handler function.
>
> This pattern could be consolidated into a single parameterized timeout
> callback using a data table that maps timer type to deferred bit and handler
> function pointer, reducing around 60 lines of near-duplicate code to
> roughly 15 lines. The TCP subsystem uses a similar approach for some of its
> timer infrastructure.
>
> That said, this follows the existing per-timer-callback pattern used
> elsewhere in the kernel networking stack (for example, tcp_write_timer,
> tcp_delack_timer are also separate functions), so this is a minor style
> observation rather than a defect.
>
This is a good one. May not add a data table, but a simple helper like this:
static void quic_timer_timeout(struct timer_list *t, int type, int defer,
void (*handler)(struct sock *sk))
{
struct quic_sock *qs = container_of(t, struct quic_sock,
timers[type].t);
struct sock *sk = &qs->inet.sk;
bh_lock_sock(sk);
if (sock_owned_by_user(sk)) {
if (!test_and_set_bit(defer, &sk->sk_tsq_flags))
sock_hold(sk);
goto out;
}
handler(sk);
out:
bh_unlock_sock(sk);
sock_put(sk);
}
will reduce quite some dup code.
> > +void quic_timer_reset(struct sock *sk, u8 type, u64 timeout)
> > +{
> > + struct timer_list *t = quic_timer(sk, type);
> > +
> > + if (timeout && !mod_timer(t, jiffies + usecs_to_jiffies(timeout)))
> > + sock_hold(sk);
> > +}
>
> Can this function handle QUIC_TIMER_PACE safely? It unconditionally casts
> the result of quic_timer() to 'struct timer_list *' and calls mod_timer().
>
> Looking at the timer API in this patch:
>
> Both quic_timer_start() and quic_timer_stop() have explicit QUIC_TIMER_PACE
> guards that dispatch to hrtimer APIs:
>
> quic_timer_start():
> if (type == QUIC_TIMER_PACE) {
> hr = quic_timer(sk, type);
> if (!hrtimer_is_queued(hr)) {
> hrtimer_start(hr, ns_to_ktime(timeout), ...);
> sock_hold(sk);
> }
> return;
> }
>
> quic_timer_stop():
> if (type == QUIC_TIMER_PACE) {
> if (hrtimer_try_to_cancel(quic_timer(sk, type)) == 1)
> sock_put(sk);
> return;
> }
>
> However, quic_timer_reset() does not have a similar guard. If a future
> caller invokes quic_timer_reset(sk, QUIC_TIMER_PACE, timeout), mod_timer()
> will operate on memory that is actually a 'struct hrtimer', potentially
> causing memory corruption.
>
> Should quic_timer_reset() either add a QUIC_TIMER_PACE guard consistent with
> quic_timer_start()/quic_timer_stop(), or document that this function must
> only be used with timer_list-based timers?
>
There will be no callers to quic_timer_reset(QUIC_TIMER_PACE), so no need
to add a check for QUIC_TIMER_PACE, I will leave a comment there for this.
Thanks.
> > +void quic_timer_start(struct sock *sk, u8 type, u64 timeout)
> > +{
> > + struct timer_list *t;
> > + struct hrtimer *hr;
> > +
> > + if (type == QUIC_TIMER_PACE) {
> > + hr = quic_timer(sk, type);
> > +
> > + if (!hrtimer_is_queued(hr)) {
> > + hrtimer_start(hr, ns_to_ktime(timeout), HRTIMER_MODE_ABS_PINNED_SOFT);
> > + sock_hold(sk);
> > + }
> > + return;
> > + }
> > +
> > + t = quic_timer(sk, type);
> > + if (timeout && !timer_pending(t)) {
> > + if (!mod_timer(t, jiffies + usecs_to_jiffies(timeout)))
> > + sock_hold(sk);
> > + }
> > +}
> > +
> > +void quic_timer_stop(struct sock *sk, u8 type)
> > +{
> > + if (type == QUIC_TIMER_PACE) {
> > + if (hrtimer_try_to_cancel(quic_timer(sk, type)) == 1)
> > + sock_put(sk);
> > + return;
> > + }
> > + if (timer_delete(quic_timer(sk, type)))
> > + sock_put(sk);
> > +}
>
> [ ... ]
>
> > diff --git a/net/quic/timer.h b/net/quic/timer.h
> > new file mode 100644
> > index 000000000000..61b094325334
> > --- /dev/null
> > +++ b/net/quic/timer.h
>
> [ ... ]
>
next prev parent reply other threads:[~2026-03-04 23:03 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
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 [this message]
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_f96OxCWnP9UdKxYtJ9=3xY69_ScTrU=9AQF1iUrBp_Cg@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