> -----Original Message----- > From: Alexander Lobakin <[email protected]> > Sent: Tuesday, August 25, 2026 2:45 PM > To: [email protected] > Cc: Nguyen, Anthony L <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; Andrew Lunn <[email protected]>; > David S. Miller <[email protected]>; Eric Dumazet > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni > <[email protected]>; Simon Horman <[email protected]>; Loktionov, > Aleksandr <[email protected]>; YiFei Zhu > <[email protected]>; [email protected]; linux- > [email protected] > Subject: Re: [PATCH iwl-next v2] idpf: add flow-based XDP fallback for FWs > without Tx FIFO support > > From: Alexander Lobakin <[email protected]> > Date: Tue, 18 Aug 2026 17:51:01 +0200 > > > From the first days of XDP implementation in idpf, it relied and > > worked solely on top of the queue-based scheduling Tx mode, which > > basically means simple FIFO. However, turned out not every firmware > > supports this mode and XDP doesn't work there at all. > > > > Since the flow-based scheduling Tx mode is mandatory and supported by > > every FW, introduce a simple fallback guarded by a static key to not > > hurt the more performant mode. The FB mode generates a completion for > > each Tx descriptor and never guarantees that there won't be any > > out-of-order completions. Serialize that using a bitmap of completed > > descriptors and report contiguous blocks of free bits to match XDP and > > XSk expectations and avoid further code complication. > > > > The usage of a bitmap on hotpath might sound scary, but this fallback > > is able to reach around 70% of the QB mode's performance, which is > > comparable to what ice gives us. The main bottlenecks are unlikely()s > > and one completion per each descriptor, while in the QB mode we have > > one completion per batch (which might contain 64 or even 128 frames), > > plus the size of the completion descriptor is > > 8 bytes in this mode (4 bytes in the QB mode), which means a lot of > > additional PCI traffic. > > > > bloat-o-meter shows .text increase in about 2 Kb without adding new > > functions or uninlining any of the existing ones. I played a bunch > > with inlining and uninlining certain pieces or the whole fallback, but > > the compiler collapses and optimizes libeth templates so hardly so > > that each additional external call only makes things worse. > > > > Reviewed-by: Aleksandr Loktionov <[email protected]> > > Tested-by: YiFei Zhu <[email protected]> > > Signed-off-by: Alexander Lobakin <[email protected]> > > Comments from Sashiko: > > > --- > > I know the window is closed, this is to trigger the validation and for > > eventual reviews. > > > > From v1[0]: > > * rework static key management: move to idpf_xdpsqs_{get,put}() to > > avoid refcount imbalance issues as .ndo_bpf() is not always called > > in pairs (hardware reset etc.) (Sashiko, internal Sashiko); > > * don't zero the whole pending window but only the frames sent since > > the last batch to avoid missed OOO completions (internal Sashiko); > > * micro-optimize idpf_xdpsq_set_rs{,_fb}(). > > > > Regarding the rest of comments: > > > >> Could this sentinel bit cause a deadlock if the queue is completely full? > >> When full, next_to_use equals next_to_clean. If the hardware just > >> completed the oldest descriptor, its bit would be cleared, but this > >> __set_bit would blindly overwrite it back to 1. The completion would > >> be ignored and the queue might permanently stall. > > > > Intel HW works that way that we can't fill the ring completely. We > > need to always leave at least one descriptor free, otherwise ntc will > > equal ntu in the HW and the queue will stall. So in all sending > > routines, our budget is limited to `free - 1`, meaning the situation > > described above can't happen (next_to_use never has its bit set to 1, > > so it's safe to use this bit as a guard and reset it after the bitmap > > search is > complete). > > > >> Is ret bounded before it is used as a bitmap index here? > >> idpf_xdp_parse_cqe() returns upper_16_bits(val) straight from the > >> device completion descriptor, so ret can be anywhere in [0, 65535]. > >> pending_mask is allocated with bitmap_zalloc_node(desc_count), where > >> desc_count is at most IDPF_MAX_DESCS. > > > > No cards under this driver have ever been seen writing garbage instead > > of the completion tag. > > > > [0] > > https://lore.kernel.org/intel-wired-lan/20260708151327.1091570-1-aleks > > [email protected] > > --- > > drivers/net/ethernet/intel/idpf/idpf.h | 1 + > > drivers/net/ethernet/intel/idpf/idpf_txrx.h | 18 +-- > > drivers/net/ethernet/intel/idpf/xdp.h | 73 ++++++++++- > > include/net/libeth/xdp.h | 13 ++ > > drivers/net/ethernet/intel/idpf/idpf_txrx.c | 12 +- > > drivers/net/ethernet/intel/idpf/xdp.c | 131 +++++++++++++++++++- > > 6 files changed, 229 insertions(+), 19 deletions(-) > > [...] > > > @@ -197,12 +229,25 @@ int idpf_xdpsqs_get(const struct idpf_vport > *vport) > > xdpsq->pending = 0; > > xdpsq->xdp_tx = 0; > > xdpsq->thresh = libeth_xdp_queue_threshold(xdpsq- > >desc_count); > > + > > + if (static_branch_unlikely(&idpf_xdp_fb) && vport->xdpsq_fb) > { > > + xdpsq->pending_mask = masks[i - sqs]; > > + xdpsq->last_ntu = 0; > > + } > > } > > > > return 0; > > + > > +err_masks: > > + vport->xdpsq_fb = false; > > + > > + for (u32 i = 0; i < vport->num_xdp_txq; i++) > > + kfree(timers[i]); > > + > > + return -ENOMEM; > > > Does this mix goto-based cleanup with __free() declarations? > > The kernel cleanup guidelines mention that the expectation is that > > usage of goto and cleanup helpers is never mixed in the same function. > > Could this be converted entirely to scope-based cleanup to avoid > > violating the cleanup subsystem rules? > > Yes it does, BUT. While you can you __free() for pointers on the stack, you > can't use it for pointers in the dynamically allocated array. > So the arrays themselves are cleaned up automatically, while the pointers > inside them are freed manually. > > The sole alternative solution would be to free everything manually, but this > would complicate already quite sloppy code even more. > > > } > > > > -void idpf_xdpsqs_put(const struct idpf_vport *vport) > > +void idpf_xdpsqs_put(struct idpf_vport *vport) > > { > > struct net_device *dev; > > u32 sqs; > > [...] > > > @@ -250,6 +308,65 @@ static int idpf_xdp_parse_cqe(const struct > idpf_splitq_4b_tx_compl_desc *desc, > > return upper_16_bits(val); > > } > > > > +static u32 idpf_xdpsq_poll_fb(struct idpf_tx_queue *xdpsq, u32 > > +budget) { > > + struct idpf_compl_queue *cq = xdpsq->complq; > > + unsigned long *mask = xdpsq->pending_mask; > > + u32 done_frames, tx_cnt, new_ntc; > > + u32 ntc = cq->next_to_clean; > > + u32 cnt = cq->desc_count; > > + bool gen; > > + > > + gen = idpf_queue_has(GEN_CHK, cq); > > + > > + for (done_frames = 0; done_frames < budget; ) { > > + int ret; > > + > > + ret = idpf_xdp_parse_cqe(&cq->comp[ntc].common, gen); > > + if (ret >= 0) { > > + __clear_bit(ret, mask); > > > Is it possible for a buggy or malicious firmware to return an out-of > > bounds completion tag here? > > While it was noted in the commit message that cards have not been seen > > writing garbage, firmware is considered an untrusted source by kernel > > security standards. Could a large value in ret bypass the size of > > pending_mask and cause an out-of-bounds write? > > Tags are written by HW, not FW. If we needed to verify everything that HW > writes, our best would be 20 Mpps instead of almost 100 :p > > > + done_frames++; > > + > > + goto next; > > + } > > + > > + switch (ret) { > > + case -ENODATA: > > + goto out; > > + case -EINVAL: > > + break; > > + } > > + > > +next: > > + if (unlikely(++ntc == cnt)) { > > + ntc = 0; > > + gen = !gen; > > + idpf_queue_change(GEN_CHK, cq); > > + } > > + } > > + > > +out: > > + cq->next_to_clean = ntc; > > + > > + if (unlikely(!done_frames)) > > + return 0; > > + > > + tx_cnt = xdpsq->desc_count; > > + > > + /* Don't go past next_to_use */ > > + __set_bit(xdpsq->next_to_use, mask); > > + > > + new_ntc = find_next_bit(mask, tx_cnt, xdpsq->next_to_clean); > > + done_frames = new_ntc - xdpsq->next_to_clean; > > + > > + if (new_ntc == tx_cnt) > > + done_frames += find_first_bit(mask, tx_cnt); > > + > > + __clear_bit(xdpsq->next_to_use, mask); > > + > > + return done_frames; > > > If out-of-order completions are processed, could this cause the XDP Tx > > cleanup timer to permanently stall? > > If completions are processed in the loop above (so done_frames > 0) > > but the descriptor exactly at next_to_clean is still pending, the new > > done_frames calculated here will be 0. Returning 0 back up the call > > chain will reach libeth_xdpsq_run_timer() in include/net/libeth/xdp.h: > > The timer is cold path for cleanup only (as hotpath only cleans 1/4 of the > ring > at most) and is run in 1 second after the last completion polling, meaning no > traffic at all. Hotpath will still try to complete descriptors each time a new > batch is to be sent. > > > +} > > + > > u32 idpf_xdpsq_poll(struct idpf_tx_queue *xdpsq, u32 budget) { > > struct idpf_compl_queue *cq = xdpsq->complq; > > I hope that explanation is enough. > > Thanks, > Olek
Tested-by: Patryk Holda <[email protected]>
