RE: [PATCH iwl-next v2] idpf: add flow-based XDP fallback for FWs without Tx FIFO support

[Holda, Patryk]

> -----Original Message-----
> From: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> Sent: Tuesday, August 25, 2026 2:45 PM
> To: intel-wired-lan@xxxxxxxxxxxxxxxx
> Cc: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; Simon Horman <horms@xxxxxxxxxx>; Loktionov,
> Aleksandr <aleksandr.loktionov@xxxxxxxxx>; YiFei Zhu
> <zhuyifei@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH iwl-next v2] idpf: add flow-based XDP fallback for FWs
> without Tx FIFO support
> 
> From: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> 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 <aleksandr.loktionov@xxxxxxxxx>
> > Tested-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> 
> 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
> > ander.lobakin@xxxxxxxxx
> > ---
> >  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 <patryk.holda@xxxxxxxxx>