From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E9B653090C7 for ; Thu, 6 Nov 2025 08:52:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762419128; cv=none; b=mNWDGHb4Kby+xBK7kZzP3MOwxrYnvy1cEKRjUzjoHsJhpHpSlU9DmeBFp4xg/b2QX6BjyU2PSJH62b4bNZ9hb1nAZqJ8AvcB7nYysMgzy57O4Jb0LspJY6mnQz6M7vCUCjSxpBz5RgiJRtkI/YOjP01PCHyHUQsex7mK2g4i0S8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762419128; c=relaxed/simple; bh=l2uWDhEaQy0Z5iNX0dnyrT6a21b6LKnpGMNZZDcdtwE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=GQdmgSN4vvvnT9rQ8einWR5fZ/l4SpbQJFUQ6hRraTI/dnJ/M88knJfpBsEU6Ty7ir/caRecqH1pxtrF9srEsMv1i4AgR9SaAg5c4u2K3w4tfWK1D6YSMABXOroUgc4RLYt9liOiqEzFS2NUAqGeIA1xTgiiMryRoZ9d2eG8rVk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=NjrLSDwG; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="NjrLSDwG" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1762419126; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8NanxbbpkNDlbUyOTW56q78Ac+UJfat3j75mYZLY0ZE=; b=NjrLSDwGdkwHgffhe4bGbq8ItYr4Jto4fVhQKSWqsjIvn6WKVImATve+qvDWPeOfAO4fXK dsVF8NMBjIQ+ZLA8SbxI+HPX87eIj+ycXEDwC/8qt61+3BSKWZNSIPX15nSD1+xGj3vnl7 GnKTc1EFUYjUkG+TN31h8drC435jQx8= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-68-7sMfFXdFOYiDkghIt4stzA-1; Thu, 06 Nov 2025 03:52:04 -0500 X-MC-Unique: 7sMfFXdFOYiDkghIt4stzA-1 X-Mimecast-MFC-AGG-ID: 7sMfFXdFOYiDkghIt4stzA_1762419124 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-477563e531cso6627605e9.1 for ; Thu, 06 Nov 2025 00:52:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762419123; x=1763023923; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=8NanxbbpkNDlbUyOTW56q78Ac+UJfat3j75mYZLY0ZE=; b=lsApQ/FN3c1RPiRQFC9L0bL34J7NcLultGU5gADtKiuVcNTWmtuZx1nmWvuw7Ekm3l eurA2ghTaSRJcLWYNARJ2LaStiQSdNm0oPoN9laBuVddnsZ6fL8W9ka+udvlzAOXiTih S8JxTFm4crcbBpz0nd56whj1ISKelL85rPm5u0baQxbI2X4cuJsVmRrCnvbiuF3L0M1S LB+U0totqTygBh3XMrw7Vyj6Kd1TDfOKKwG5qL6XlAAc8LZ0nKPihGgaUn8eHleTnCcR Hktpa1Ijpakd7dTsABxjblqhZbMJnKSniEQjDfqJHHfTAkGDj7k8HuVUSR61xjcsGYcu lpuw== X-Forwarded-Encrypted: i=1; AJvYcCWm4iEhPFOqSQOdPssihW5ATp/zP4VzmIRA6//2b4DWHGaPcvJABLM7Wwf+GD1uSD4h8jp/@lists.linux.dev X-Gm-Message-State: AOJu0Yz1wNvVAB55lYvUFVdbNzguSDoboMDTDYAH5jbVD1a+TuKsft4P IOTHfs7AqhI1ptQ8yfj4A3tv6Fgvc/aBfBfJGcElcuQBrWtBovF+r8NXAoWqGbW890TXwCix1PU h43uTiOCKv2cIqYvnEG8bPtyyHBfMR4zgvjF0HVlhNc8VV6lR0aivdrw= X-Gm-Gg: ASbGncusnwMaDoMVNzTKgp5LT+cwN5OfdYmgoQseGSy4l75BcD76n81+bTH0tLUsh+8 Pw/17pA38RhBGZt/r19qkSSgPMzILRPaBAfSamEktMihBIDeeDNkAsopdu0rfGhFSFXkyZP1vj7 n0HWeVnjMbG0taNCdGxR8EbGyDTnfeEKsaujQWoG3xNo9cNXdM521A3HaCXA6U1Ia64Jxg4rVlN e3KnVcspnEOBr7tmuSaHTEj+mXuI5rwss3SJPnfIDEROpq/F+S0CSuuKIzWOAASb2uBxvPEM5dI Gy8ptZFa6AZbjY/8nRT1892nRxL8T/y2LeOUFfM5L33uUL8kjLe1SwIPFYPhOG4AeBYlY5IbUHG ckw== X-Received: by 2002:a05:600c:3487:b0:471:115e:9605 with SMTP id 5b1f17b1804b1-4775ce3d8a1mr64704925e9.35.1762419123473; Thu, 06 Nov 2025 00:52:03 -0800 (PST) X-Google-Smtp-Source: AGHT+IFPovKIk02f8ddjHUYyEZ+Oj38N6pOCkSx8ThIFm92RoMszw4bRUVHCaXxRkNiaePAEdRptZw== X-Received: by 2002:a05:600c:3487:b0:471:115e:9605 with SMTP id 5b1f17b1804b1-4775ce3d8a1mr64704515e9.35.1762419123036; Thu, 06 Nov 2025 00:52:03 -0800 (PST) Received: from [192.168.88.32] ([212.105.155.83]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-429eb410ffcsm3542471f8f.15.2025.11.06.00.52.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Nov 2025 00:52:02 -0800 (PST) Message-ID: <24cee5fb-1710-4d1e-a1af-793fb99fc9c7@redhat.com> Date: Thu, 6 Nov 2025 09:51:59 +0100 Precedence: bulk X-Mailing-List: quic@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v4 06/15] quic: add stream management To: Xin Long Cc: network dev , quic@lists.linux.dev, davem@davemloft.net, kuba@kernel.org, Eric Dumazet , Simon Horman , Stefan Metzmacher , Moritz Buhl , Tyler Fanelli , Pengtao He , Thomas Dreibholz , linux-cifs@vger.kernel.org, Steve French , Namjae Jeon , Paulo Alcantara , Tom Talpey , kernel-tls-handshake@lists.linux.dev, Chuck Lever , Jeff Layton , Steve Dickson , Hannes Reinecke , Alexander Aring , David Howells , Matthieu Baerts , John Ericson , Cong Wang , "D . Wythe" , Jason Baron , illiliti , Sabrina Dubroca , Marcelo Ricardo Leitner , Daniel Stenberg , Andy Gospodarek References: <6b527b669fe05f9743e37d9f584f7cd492a7649b.1761748557.git.lucien.xin@gmail.com> From: Paolo Abeni In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: eC4ma2wBKSRS5j1Y25jleipuQWKVjJ_E_0Z7C-eFfOY_1762419124 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 11/6/25 2:27 AM, Xin Long wrote: > On Tue, Nov 4, 2025 at 6:05 AM Paolo Abeni wrote: >> >> On 10/29/25 3:35 PM, Xin Long wrote: >> +/* Create and register new streams for sending. */ >>> +static struct quic_stream *quic_stream_send_create(struct quic_stream_table *streams, >>> + s64 max_stream_id, u8 is_serv) >>> +{ >>> + struct quic_stream *stream = NULL; >>> + s64 stream_id; >>> + >>> + stream_id = streams->send.next_bidi_stream_id; >>> + if (quic_stream_id_uni(max_stream_id)) >>> + stream_id = streams->send.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_KERNEL_ACCOUNT); >>> + if (!stream) >>> + return NULL; >>> + >>> + stream->id = stream_id; >>> + if (quic_stream_id_uni(stream_id)) { >>> + stream->send.max_bytes = streams->send.max_stream_data_uni; >>> + >>> + if (streams->send.next_uni_stream_id < stream_id + QUIC_STREAM_ID_STEP) >>> + streams->send.next_uni_stream_id = stream_id + QUIC_STREAM_ID_STEP; >> >> It's unclear to me the goal the above 2 statements. Dealing with id >> wrap-arounds? If 'streams->send.next_uni_stream_id < stream_id + >> QUIC_STREAM_ID_STEP' is not true the next quic_stream_send_create() will >> reuse the same stream_id. >> >> I moving the above in a separate helper with some comments would help. >> > I will add a macro for this: > > #define quic_stream_id_next_update(limits, type, id) \ > do { \ > if ((limits)->next_##type##_stream_id < (id) + > QUIC_STREAM_ID_STEP) \ > (limits)->next_##type##_stream_id = (id) + > QUIC_STREAM_ID_STEP; \ > (limits)->streams_##type++; > \ > } while (0) > > So that we can use it to update both next_uni_stream_id and next_bidi_stream_id. A function would be better tacking the next_id value as an argument. More importantly please document the goal here which is still unclear to me. >> The above 2 functions has a lot of code in common. I think you could >> deduplicate it by: >> - defining a named type for quic_stream_table.{send,recv} >> - define a generic /() helper using an additonal >> argument for the relevant table.{send,recv} >> - replace the above 2 functions with a single invocation to such helper. > This is a very smart idea! > > It will dedup not only quic_stream_recv_create(), but also > quic_stream_get_param() and quic_stream_set_param(). > > I will define a type named 'struct quic_stream_limits'. > Note that, since we must pass 'bool send' to quic_stream_create() for > setting the fields in a single 'stream' . > > 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; > } > > I'm planning not to pass additional argument of table.{send,recv}, > but do this in quic_stream_create(): > struct quic_stream_limits *limits = &streams->send; > gfp_t gfp = GFP_KERNEL_ACCOUNT; > > if (!send) { > limits = &streams->recv; > gfp = GFP_ATOMIC | __GFP_ACCOUNT; > } > >> >> It looks like there are more de-dup opportunity below. >> > Yes, the difference is only the variable name _uni_ and _bidi_. > I'm planning to de-dup them with macros like: > > #define quic_stream_id_below_next(streams, type, id, send) \ > ((send) ? ((id) < (streams)->send.next_##type##_stream_id) : \ > ((id) < (streams)->recv.next_##type##_stream_id)) > > /* 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) > { > if (quic_stream_id_uni(stream_id)) > return quic_stream_id_below_next(streams, uni, stream_id, send); > return quic_stream_id_below_next(streams, bidi, stream_id, send); > } > > #define quic_stream_id_above_max(streams, type, id) \ > (((id) > (streams)->send.max_##type##_stream_id) ? true : \ > (quic_stream_id_to_streams((id) - > (streams)->send.next_##type##_stream_id) + \ > (streams)->send.streams_##type > > (streams)->send.max_streams_##type)) Uhmm... with "more de-dup opportunity below" I intended quic_stream_get_param() and quic_stream_set_param(). I would refrain from adding macros. I think the above idea ('struct quic_stream_limits') would not need that?!? /P