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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Alexander Lobakin <aleksander.lobakin@intel.com>
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@intel.com>
> Tested-by: YiFei Zhu <zhuyifei@google.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

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-aleksander.lobakin@intel.com
> ---
>  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