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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <47059a58-a12c-4a4d-ba00-fb057088a3f0@molgen.mpg.de>
Date: Wed, 30 Jul 2025 00:53:53 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Joshua A Hay <joshua.a.hay@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 Madhu Chittim <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

Dear Joshua,


Thank you for your reply.

Am 29.07.25 um 19:15 schrieb Hay, Joshua A:
>>> 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.

[…]


Kind regards,

Paul


PS: Just a note, that your client seems to have wrapped some of the code 
lines.