On Fri, Jul 25, 2025 at 5:45 PM Larysa Zaremba <larysa.zaremba@xxxxxxxxx> wrote:
>
> On Sun, Jul 20, 2025 at 05:11:21PM +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@xxxxxxxxxxx>
> >
> > - 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
Sorry, I'm afraid I don't think so. In ice_xmit_zc(), if the budget is
zero, it will return true because of the following codes:
nb_pkts = xsk_tx_peek_release_desc_batch(xsk_pool, budget);
if (!nb_pkts)
return true;
Supposing there is no single desc in the tx ring, the budget will
always be zero even when the napi poll is triggered.
Thanks,
Jason
> 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@xxxxxxxxxxx>
> > ---
> > 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
> >
> >
