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: <CAL+tcoAUW_J62aw3aGBru+0GmaTjoom1qu8Y=aiSc9EGU09Nww@mail.gmail.com>
Date: Fri, 25 Jul 2025 20:21:03 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Larysa Zaremba <larysa.zaremba@...el.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 Fri, Jul 25, 2025 at 5:45 PM Larysa Zaremba <larysa.zaremba@...el.com> wrote:
>
> 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

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