netdev - Re: [PATCH net-next 3/5] ixgbe: xsk: use ixgbe_desc_unused as the budget in ixgbe_xmit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aINSDD1BezlEn_gM@soc-5CG4396X81.clients.intel.com>
Date: Fri, 25 Jul 2025 11:44:44 +0200
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Jason Xing <kerneljasonxing@...il.com>
CC: <anthony.l.nguyen@...el.com>, <przemyslaw.kitszel@...el.com>,
	<andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <bjorn@...nel.org>,
	<magnus.karlsson@...el.com>, <maciej.fijalkowski@...el.com>,
	<jonathan.lemon@...il.com>, <sdf@...ichev.me>, <ast@...nel.org>,
	<daniel@...earbox.net>, <hawk@...nel.org>, <john.fastabend@...il.com>,
	<bpf@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 3/5] ixgbe: xsk: use ixgbe_desc_unused as the
 budget in ixgbe_xmit_zc

On Sun, Jul 20, 2025 at 05:11:21PM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> - Adjust ixgbe_desc_unused as the budget value.
> - Avoid checking desc_unused over and over again in the loop.
> 
> The patch makes ixgbe follow i40e driver that was done in commit
> 1fd972ebe523 ("i40e: move check of full Tx ring to outside of send loop").
> [ Note that the above i40e patch has problem when ixgbe_desc_unused(tx_ring)
> returns zero. The zero value as the budget value means we don't have any
> possible descs to be sent, so it should return true instead to tell the
> napi poll not to launch another poll to handle tx packets.

I do not think such reasoning is correct. If you look at the current mature 
implementation in i40e and ice, it always returns (nb_pkts < budget), so when 
the budget is `0`, the napi will always be rescheduled. Zero unused descriptors 
means that the entire ring is held by HW, so it makes sense to retry to 
reclaim some resources ASAP. Also, zero unused normal descriptors does not mean 
there is no UMEM descriptors to process.

Please, remove the following lines and the patch should be fine:

+     if (!budget)
+             return true;

> Even though
> that patch behaves correctly by returning true in this case, it happens
> because of the unexpected underflow of the budget. Taking the current
> version of i40e_xmit_zc() as an example, it returns true as expected. ]
> Hence, this patch adds a standalone if statement of zero budget in front
> of ixgbe_xmit_zc() as explained before.
> 
> Use ixgbe_desc_unused to replace the original fixed budget with the number
> of available slots in the Tx ring. It can gain some performance.
> 
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> index a463c5ac9c7c..f3d3f5c1cdc7 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> @@ -393,17 +393,14 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
>  	struct xsk_buff_pool *pool = xdp_ring->xsk_pool;
>  	union ixgbe_adv_tx_desc *tx_desc = NULL;
>  	struct ixgbe_tx_buffer *tx_bi;
> -	bool work_done = true;
>  	struct xdp_desc desc;
>  	dma_addr_t dma;
>  	u32 cmd_type;
>  
> -	while (likely(budget)) {
> -		if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
> -			work_done = false;
> -			break;
> -		}
> +	if (!budget)
> +		return true;
>  
> +	while (likely(budget)) {
>  		if (!netif_carrier_ok(xdp_ring->netdev))
>  			break;
>  
> @@ -442,7 +439,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
>  		xsk_tx_release(pool);
>  	}
>  
> -	return !!budget && work_done;
> +	return !!budget;
>  }
>  
>  static void ixgbe_clean_xdp_tx_buffer(struct ixgbe_ring *tx_ring,
> @@ -505,7 +502,7 @@ bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector,
>  	if (xsk_uses_need_wakeup(pool))
>  		xsk_set_tx_need_wakeup(pool);
>  
> -	return ixgbe_xmit_zc(tx_ring, q_vector->tx.work_limit);
> +	return ixgbe_xmit_zc(tx_ring, ixgbe_desc_unused(tx_ring));
>  }
>  
>  int ixgbe_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
> -- 
> 2.41.3
> 
>