From: Xin Long <lucien.xin@gmail.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: network dev <netdev@vger.kernel.org>,
quic@lists.linux.dev, davem@davemloft.net, kuba@kernel.org,
Eric Dumazet <edumazet@google.com>,
Simon Horman <horms@kernel.org>,
Stefan Metzmacher <metze@samba.org>,
Moritz Buhl <mbuhl@openbsd.org>,
Tyler Fanelli <tfanelli@redhat.com>,
Pengtao He <hepengtao@xiaomi.com>,
Thomas Dreibholz <dreibh@simula.no>,
linux-cifs@vger.kernel.org, Steve French <smfrench@gmail.com>,
Namjae Jeon <linkinjeon@kernel.org>,
Paulo Alcantara <pc@manguebit.com>, Tom Talpey <tom@talpey.com>,
kernel-tls-handshake@lists.linux.dev,
Chuck Lever <chuck.lever@oracle.com>,
Jeff Layton <jlayton@kernel.org>,
Steve Dickson <steved@redhat.com>,
Hannes Reinecke <hare@suse.de>,
Alexander Aring <aahringo@redhat.com>,
David Howells <dhowells@redhat.com>,
Matthieu Baerts <matttbe@kernel.org>,
John Ericson <mail@johnericson.me>,
Cong Wang <xiyou.wangcong@gmail.com>,
"D . Wythe" <alibuda@linux.alibaba.com>,
Jason Baron <jbaron@akamai.com>,
illiliti <illiliti@protonmail.com>,
Sabrina Dubroca <sd@queasysnail.net>,
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
Daniel Stenberg <daniel@haxx.se>,
Andy Gospodarek <andrew.gospodarek@broadcom.com>
Subject: Re: [PATCH net-next v6 06/16] quic: add stream management
Date: Thu, 8 Jan 2026 15:53:38 -0500 [thread overview]
Message-ID: <CADvbK_dEbOvdaMB4jGkaQMO7j0CnnpYUYJXmS-eKxmURybG09w@mail.gmail.com> (raw)
In-Reply-To: <CADvbK_eXRpT8n1B7p2-1T6eAZZ=4p7gQgJtMGBBQrHa036nyxw@mail.gmail.com>
On Thu, Jan 8, 2026 at 3:29 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Thu, Jan 8, 2026 at 10:36 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On 1/5/26 3:04 PM, Xin Long wrote:
> > > +/* Create and register new streams for sending or receiving. */
> > > +static struct quic_stream *quic_stream_create(struct quic_stream_table *streams,
> > > + s64 max_stream_id, bool send, bool is_serv)
> > > +{
> > > + struct quic_stream_limits *limits = &streams->send;
> > > + struct quic_stream *stream = NULL;
> > > + gfp_t gfp = GFP_KERNEL_ACCOUNT;
> > > + s64 stream_id;
> > > +
> > > + if (!send) {
> > > + limits = &streams->recv;
> > > + gfp = GFP_ATOMIC | __GFP_ACCOUNT;
> > > + }
> > > + stream_id = limits->next_bidi_stream_id;
> > > + if (quic_stream_id_uni(max_stream_id))
> > > + stream_id = limits->next_uni_stream_id;
> > > +
> > > + /* rfc9000#section-2.1: A stream ID that is used out of order results in all streams
> > > + * of that type with lower-numbered stream IDs also being opened.
> > > + */
> > > + while (stream_id <= max_stream_id) {
> > > + stream = kzalloc(sizeof(*stream), gfp);
> > > + if (!stream)
> > > + return NULL;
> >
> > Do you need to release the allocated ids in case of failure? It would be
> > sourprising to find some ids allocated when this call fails/returns NULL.
> I was aware of this, but didn't change it. As the streams are always opened
> sequentially, I think it's fine just to leave them without causing problems
> when users assume these streams are not yet open.
>
> >
> > > +
> > > + stream->id = stream_id;
> > > + if (quic_stream_id_uni(stream_id)) {
> > > + if (send) {
> > > + stream->send.max_bytes = limits->max_stream_data_uni;
> > > + } else {
> > > + stream->recv.max_bytes = limits->max_stream_data_uni;
> > > + stream->recv.window = stream->recv.max_bytes;
> > > + }
> > > + /* Streams must be opened sequentially. Update the next stream ID so the
> > > + * correct starting point is known if an out-of-order open is requested.
> > > + */
> > > + limits->next_uni_stream_id = stream_id + QUIC_STREAM_ID_STEP;
> > > + limits->streams_uni++;
> > > +
> > > + quic_stream_add(streams, stream);
> > > + stream_id += QUIC_STREAM_ID_STEP;
> > > + continue;
> > > + }
> > > +
> > > + if (quic_stream_id_local(stream_id, is_serv)) {
> > > + stream->send.max_bytes = streams->send.max_stream_data_bidi_remote;
> > > + stream->recv.max_bytes = streams->recv.max_stream_data_bidi_local;
> > > + } else {
> > > + stream->send.max_bytes = streams->send.max_stream_data_bidi_local;
> > > + stream->recv.max_bytes = streams->recv.max_stream_data_bidi_remote;
> > > + }
> > > + stream->recv.window = stream->recv.max_bytes;
> > > +
> > > + limits->next_bidi_stream_id = stream_id + QUIC_STREAM_ID_STEP;
> > > + limits->streams_bidi++;
> > > +
> > > + quic_stream_add(streams, stream);
> > > + stream_id += QUIC_STREAM_ID_STEP;
> > > + }
> > > + return stream;
> > > +}
> > > +
> > > +/* Check if a send or receive stream ID is already closed. */
> > > +static bool quic_stream_id_closed(struct quic_stream_table *streams, s64 stream_id, bool send)
> > > +{
> > > + struct quic_stream_limits *limits = send ? &streams->send : &streams->recv;
> > > +
> > > + if (quic_stream_id_uni(stream_id))
> > > + return stream_id < limits->next_uni_stream_id;
> > > + return stream_id < limits->next_bidi_stream_id;
> >
> > I can't recall if I mentioned the following in a past review... it looks
> > like the above assumes wrap around are not possible, which is realistic
> > given the u64 counters - it would require > 100y on a server allocating
> > 4G ids per second.
> >
> > But it would be nice to explcitly document such assumption somewhere.
> >
> How about I add a simple comment in quic_stream_create() right above
> the next_uni_stream_id/streams_uni increases, like
>
> "Note overflow of next_uni_stream_id/streams_uni is impossible with u64."
>
> > > +}
> > > +
> > > +/* Check if a stream ID would exceed local (recv) or peer (send) limits. */
> > > +bool quic_stream_id_exceeds(struct quic_stream_table *streams, s64 stream_id, bool send)
> > > +{
> > > + u64 nstreams;
> > > +
> > > + if (!send) {
> > > + if (quic_stream_id_uni(stream_id))
> > > + return stream_id > streams->recv.max_uni_stream_id;
> > > + return stream_id > streams->recv.max_bidi_stream_id;
> > > + }
> > > +
> > > + if (quic_stream_id_uni(stream_id)) {
> > > + if (stream_id > streams->send.max_uni_stream_id)
> > > + return true;
> > > + stream_id -= streams->send.next_uni_stream_id;
> > > + nstreams = quic_stream_id_to_streams(stream_id);
> >
> > It's not clear to me why send streams only have this additional check.
> This is a good question.
>
> For recv.max_uni_stream_id, it changes based on next_uni/bidi_stream_id,
> max_streams_uni/bidi and streams_uni/bidi in quic_stream_max_streams_update(),
> there's no need to check them again. (maybe I should leave a comment here)
>
> But for send.max_uni_stream_id, it was updated simply from the peer's updated
> recv.max_uni_stream_id announcement, it must check its local counts and
> limits as well.
>
> >
> > > + return nstreams + streams->send.streams_uni > streams->send./;
> >
> > Possibly it would be more consistent
> >
> > max_uni_stream_id -> max_stream_ids_uni
> >
> > (no strong preferences)
> I actually got the variable name from
> https://datatracker.ietf.org/doc/html/rfc9000.
>
Sorry, I may misunderstand here. from the variable names:
- max_uni_stream_id: is a stream_id
- max_stream_ids_uni: should be a stream_id counter.
max_uni_stream_id = quic_stream_id_to_streams(max_stream_ids_uni);
max_stream_ids_uni = quic_stream_streams_to_id(max_uni_stream_id)
I used max_uni_stream_id, as in most places it's used to check against
stream_id.
Thanks.
> >
> > > + }
> > > +
> > > + if (stream_id > streams->send.max_bidi_stream_id)
> > > + return true;
> > > + stream_id -= streams->send.next_bidi_stream_id;
> > > + nstreams = quic_stream_id_to_streams(stream_id);
> > > + return nstreams + streams->send.streams_bidi > streams->send.max_streams_bidi;
> > > +}
> >
> > [...]
> > > +/* Get or create a receive stream by ID. Requires sock lock held. */
> > > +struct quic_stream *quic_stream_recv_get(struct quic_stream_table *streams, s64 stream_id,
> > > + bool is_serv)
> > > +{
> > > + struct quic_stream *stream;
> > > +
> > > + if (!quic_stream_id_valid(stream_id, is_serv, false))
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + stream = quic_stream_find(streams, stream_id);
> > > + if (stream)
> > > + return stream;
> > > +
> > > + if (quic_stream_id_local(stream_id, is_serv)) {
> > > + if (quic_stream_id_closed(streams, stream_id, true))
> > > + return ERR_PTR(-ENOSTR);
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > +
> > > + if (quic_stream_id_closed(streams, stream_id, false))
> > > + return ERR_PTR(-ENOSTR);
> > > +
> > > + if (quic_stream_id_exceeds(streams, stream_id, false))
> > > + return ERR_PTR(-EAGAIN);
> > > +
> > > + stream = quic_stream_create(streams, stream_id, false, is_serv);
> > > + if (!stream)
> > > + return ERR_PTR(-ENOSTR);
> > > + if (quic_stream_id_valid(stream_id, is_serv, true))
> > > + streams->send.active_stream_id = stream_id;
> >
> > This function is really similar to quic_stream_send_get(), I think it
> > should be easy factor out a common helper (and possibly use directly
> > such helper with no send/recv wrapper).
> >
> I will factor out a common helper quic_stream_get() but keep
> quic_stream_send_get/put() as:
>
> struct quic_stream *quic_stream_send_get(...)
> {
> return quic_stream_get(streams, stream_id, is_serv, true);
> }
>
> struct quic_stream *quic_stream_recv_get(...)
> {
> return quic_stream_get(streams, stream_id, is_serv, false);
> }
>
> Thanks.
next prev parent reply other threads:[~2026-01-08 20:53 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-05 14:04 [PATCH net-next v6 00/16] net: introduce QUIC infrastructure and core subcomponents Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 01/16] net: define IPPROTO_QUIC and SOL_QUIC constants Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 02/16] net: build socket infrastructure for QUIC protocol Xin Long
2026-01-08 14:40 ` Paolo Abeni
2026-01-08 22:19 ` Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 03/16] quic: provide common utilities and data structures Xin Long
2026-01-08 14:45 ` Paolo Abeni
2026-01-08 16:58 ` Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 04/16] quic: provide family ops for address and protocol Xin Long
2026-01-08 14:51 ` Paolo Abeni
2026-01-05 14:04 ` [PATCH net-next v6 05/16] quic: provide quic.h header files for kernel and userspace Xin Long
2026-01-08 5:29 ` Yohei Kojima
2026-01-08 9:15 ` Stefan Metzmacher
2026-01-08 10:32 ` Yohei Kojima
2026-01-08 15:00 ` Paolo Abeni
2026-01-08 17:44 ` Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 06/16] quic: add stream management Xin Long
2026-01-08 15:35 ` Paolo Abeni
2026-01-08 20:29 ` Xin Long
2026-01-08 20:53 ` Xin Long [this message]
2026-01-05 14:04 ` [PATCH net-next v6 07/16] quic: add connection id management Xin Long
2026-01-08 15:52 ` Paolo Abeni
2026-01-08 18:07 ` Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 08/16] quic: add path management Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 09/16] quic: add congestion control Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 10/16] quic: add packet number space Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 11/16] quic: add crypto key derivation and installation Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 12/16] quic: add crypto packet encryption and decryption Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 13/16] quic: add timer management Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 14/16] quic: add frame encoder and decoder base Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 15/16] quic: add packet builder base Xin Long
2026-01-05 14:04 ` [PATCH net-next v6 16/16] quic: add packet parser base 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_dEbOvdaMB4jGkaQMO7j0CnnpYUYJXmS-eKxmURybG09w@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=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