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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jul 15, 2026 at 02:08:45PM +0000, sashiko-bot@kernel.org wrote:
> 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.
> --

For these issues a single invalid descriptor will be reflected at
batch.consumed_descs and based on that global consumer pointer will be
updated so user space will see reclaimed descriptor. The stall could
happen tho if ZC Tx driver would be misbehaving with respect to wakeup
flag. I believe the correct semantics are:

- clear tx wakeup at the tx zc napi side entry
- do the processing (or not)
- set tx wakeup flag back at the exit

which tells us that i40e and ice need massage. Libeth got it right from
what I saw.

> 
> --- 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