> >>> 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 <[email protected]> > >>> Reviewed-by: Madhu Chittim <[email protected]> > >>> --- > >>> 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
