Re: [PATCH v3 net 4/6] xsk: reclaim invalid multi-buffer Tx descs in ZC path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Processing an invalid descriptor or draining an invalid multi-buffer packet at the beginning of a Tx batch stalls the Tx queue.
--

--- Patch [4]: [PATCH v3 net 4/6] xsk: reclaim invalid multi-buffer Tx descs in ZC path ---
commit f71b54a29f7fa6021e0f05cf7b9e1dfd86287c21
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

xsk: reclaim invalid multi-buffer Tx descs in ZC path

This commit updates the zero-copy Tx batch parser to process descriptors in
packet units rather than individually. It ensures that descriptors belonging
to invalid or oversized multi-buffer packets are properly drained and
returned to the completion ring for reclamation without being sent to the
driver.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 385a3f4a1b320..2909a0ec68373 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -603,19 +758,16 @@ u32 xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, u32 nb_pkts)
>  	if (!nb_pkts)
>  		goto out;
>  
> -	nb_pkts = xskq_cons_read_desc_batch(xs->tx, pool, nb_pkts);
> -	if (!nb_pkts) {
> -		xs->tx->queue_empty_descs++;
> -		goto out;
> -	}
> -
> -	__xskq_cons_release(xs->tx);
> -	xskq_prod_write_addr_batch(pool->cq, pool->tx_descs, nb_pkts);
> -	xs->sk.sk_write_space(&xs->sk);
> +	batch = umem_shared ?
> +		xsk_tx_peek_release_shared_desc_batch(pool, nb_pkts) :
> +		__xsk_tx_peek_release_desc_batch(pool, xs,
> +						 pool->tx_descs,
> +						 nb_pkts);
> +	xsk_tx_commit_batch(pool, &batch);
>  
>  out:
>  	rcu_read_unlock();
> -	return nb_pkts;
> +	return batch.tx_descs;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but returning 0 to the driver
here from xsk_tx_peek_release_desc_batch when the batch consists solely of
reclaimed descriptors signals that the Tx ring is empty.

Since user space typically waits for hardware completions before manually
triggering sendmsg again, does returning 0 here result in a permanent
Tx queue stall until an unrelated Rx packet arrives to trigger an interrupt?

> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 3e3fbb73d23e2..a15ff1929db62 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h

[ ... ]

> @@ -282,26 +295,49 @@ u32 xskq_cons_read_desc_batch(struct xsk_queue *q, struct xsk_buff_pool *pool,
>  		descs[nb_entries] = ring->desc[idx];
>  		cached_cons++;
>  		parse_desc(q, pool, &descs[nb_entries], &parsed);
> -		if (unlikely(!parsed.valid))
> -			break;
> +		if (unlikely(!parsed.valid)) {
> +			if (!drain && !nr_frags && !parsed.mb)
> +				break;

[Severity: High]
This is a pre-existing issue, but when xskq_cons_read_desc_batch encounters
a standalone invalid descriptor at the very start of a batch, it breaks
early out of this processing loop.

If no valid descriptors were accumulated yet, doesn't this result in
returning an empty batch to the driver, causing NAPI polling to halt
prematurely and stranding any subsequent valid packets on the ring?

> +
> +			drain = true;
> +		}
> +
> +		nr_frags++;
> +		nb_entries++;
>  
>  		if (likely(!parsed.mb)) {
> -			total_descs += (nr_frags + 1);
> -			nr_frags = 0;
> -		} else {
> -			nr_frags++;
> -			if (nr_frags == pool->xdp_zc_max_segs) {
> +			if (unlikely(drain)) {
> +				batch.reclaim_descs = nr_frags;
> +				WRITE_ONCE(xs->drain_cont, false);
>  				nr_frags = 0;
>  				break;
>  			}

[Severity: High]
Similarly, this is a pre-existing issue, but breaking early here when
finishing a drained packet will leave batch.tx_descs as 0 if no valid
packets were seen first.

Could this stall the Tx queue, since no valid packets are submitted to the
hardware to generate the Tx completion interrupts that user space needs
to resume operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714140722.111645-1-maciej.fijalkowski@intel.com?part=4