netdev - RE: [Intel-wired-lan] [PATCH iwl-net v3 1/6] idpf: add support for Tx refillqs in flow scheduling mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DM4PR11MB650204E85BA63C9E528AD920D427A@DM4PR11MB6502.namprd11.prod.outlook.com>
Date: Thu, 31 Jul 2025 21:08:40 +0000
From: "Hay, Joshua A" <joshua.a.hay@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Chittim, Madhu"
	<madhu.chittim@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net v3 1/6] idpf: add support for Tx
 refillqs in flow scheduling mode

> >>> In certain production environments, it is possible for completion tags
> >>> to collide, meaning N packets with the same completion tag are in flight
> >>> at the same time. In this environment, any given Tx queue is effectively
> >>> used to send both slower traffic and higher throughput traffic
> >>> simultaneously. This is the result of a customer's specific
> >>> configuration in the device pipeline, the details of which Intel cannot
> >>> provide. This configuration results in a small number of out-of-order
> >>> completions, i.e., a small number of packets in flight. The existing
> >>> guardrails in the driver only protect against a large number of packets
> >>> in flight. The slower flow completions are delayed which causes the
> >>> out-of-order completions. The fast flow will continue sending traffic
> >>> and generating tags. Because tags are generated on the fly, the fast
> >>> flow eventually uses the same tag for a packet that is still in flight
> >>> from the slower flow. The driver has no idea which packet it should
> >>> clean when it processes the completion with that tag, but it will look
> >>> for the packet on the buffer ring before the hash table.  If the slower
> >>> flow packet completion is processed first, it will end up cleaning the
> >>> fast flow packet on the ring prematurely. This leaves the descriptor
> >>> ring in a bad state resulting in a crashes or Tx timeout.
> >>
> >> “in a crash” or just “crashes” wtih no article.
> >
> > Will fix.
> >
> >>
> >>> In summary, generating a tag when a packet is sent can lead to the same
> >>> tag being associated with multiple packets. This can lead to resource
> >>> leaks, crashes, and/or Tx timeouts.
> >>>
> >>> Before we can replace the tag generation, we need a new mechanism for
> >>> the send path to know what tag to use next. The driver will allocate and
> >>> initialize a refillq for each TxQ with all of the possible free tag
> >>> values. During send, the driver grabs the next free tag from the refillq
> >>> from next_to_clean. While cleaning the packet, the clean routine posts
> >>> the tag back to the refillq's next_to_use to indicate that it is now
> >>> free to use.
> >>>
> >>> This mechanism works exactly the same way as the existing Rx refill
> >>> queues, which post the cleaned buffer IDs back to the buffer queue to be
> >>> reposted to HW. Since we're using the refillqs for both Rx and Tx now,
> >>> genercize some of the existing refillq support.
> >>
> >> gener*i*cize
> >
> > Will fix.
> >
> >>
> >>> Note: the refillqs will not be used yet. This is only demonstrating how
> >>> they will be used to pass free tags back to the send path.
> >>>
> >>> Signed-off-by: Joshua Hay <joshua.a.hay@...el.com>
> >>> Reviewed-by: Madhu Chittim <madhu.chittim@...el.com>
> >>> ---
> >>> v2:
> >>> - reorder refillq init logic to reduce indentation
> >>> - don't drop skb if get free bufid fails, increment busy counter
> >>> - add missing unlikely
> >>> ---
> >>>    drivers/net/ethernet/intel/idpf/idpf_txrx.c | 93
> +++++++++++++++++++--
> >>>    drivers/net/ethernet/intel/idpf/idpf_txrx.h |  8 +-
> >>>    2 files changed, 91 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> >>> index 5cf440e09d0a..d5908126163d 100644
> >>> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> >>> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> 
> […]
> 
> >>> @@ -267,6 +271,29 @@ static int idpf_tx_desc_alloc(const struct
> idpf_vport *vport,
> >>>    	tx_q->next_to_clean = 0;
> >>>    	idpf_queue_set(GEN_CHK, tx_q);
> >>>
> >>> +	if (!idpf_queue_has(FLOW_SCH_EN, tx_q))
> >>> +		return 0;
> >>> +
> >>> +	refillq = tx_q->refillq;
> >>> +	refillq->desc_count = tx_q->desc_count;
> >>> +	refillq->ring = kcalloc(refillq->desc_count, sizeof(u32),
> >>> +				GFP_KERNEL);
> >>> +	if (!refillq->ring) {
> >>> +		err = -ENOMEM;
> >>> +		goto err_alloc;
> >>> +	}
> >>> +
> >>> +	for (u32 i = 0; i < refillq->desc_count; i++)
> >>
> >> No need to limit the length, as far as I can see.
> >
> > Apologies, I'm not sure I follow. Can you please clarify?
> 
> Sorry, for being ambiguous. I meant, just use `unsigned int` as type for
> the counting variable.

Ah, sure, will fix.

> 
> […]
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> PS: Just a note, that your client seems to have wrapped some of the code
> lines.

Noted, I'll look into it. Thanks!
Josh