| 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
| ||
|
Message-ID: <CAL+tcoDHgcr+fapRxVZMoefi+PujENenYSr+e-Dd=Tb=jJP03w@mail.gmail.com>
Date: Wed, 13 Aug 2025 19:44:25 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
Cc: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"horms@...nel.org" <horms@...nel.org>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>, "sdf@...ichev.me" <sdf@...ichev.me>,
"Zaremba, Larysa" <larysa.zaremba@...el.com>,
"Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, Jason Xing <kernelxing@...cent.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2 2/3] ixgbe: xsk: use
ixgbe_desc_unused as the budget in ixgbe_xmit_zc
On Wed, Aug 13, 2025 at 7:14 PM Loktionov, Aleksandr
<aleksandr.loktionov@...el.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf
> > Of Jason Xing
> > Sent: Tuesday, August 12, 2025 9:55 AM
> > To: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> > pabeni@...hat.com; horms@...nel.org; andrew+netdev@...n.ch; Nguyen,
> > Anthony L <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@...el.com>; sdf@...ichev.me; Zaremba, Larysa
> > <larysa.zaremba@...el.com>; Fijalkowski, Maciej
> > <maciej.fijalkowski@...el.com>
> > Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Jason
> > Xing <kernelxing@...cent.com>
> > Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/3] ixgbe: xsk: use
> > ixgbe_desc_unused as the budget in ixgbe_xmit_zc
> >
> > 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. 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.
> You state “It can gain some performance” but provide no numbers
> (before/after metrics, hardware, workload, methodology).
> The https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> ask to quantify optimizations with measurements and discuss trade‑offs.
Based on my understanding of performance, there are two kinds: 1) it
can save some cycles and indeed reduce the time but cannot be easily
observed, 2) it can be directly shown through various tests. The whole
series belongs to the former due to limited tests. We cannot deny the
optimization even though we cannot see it from the numbers but we can
conclude it from the theory.
As the official doc requires us to do so, I will remove all the
related words to avoid further confusion in V3. Thanks for sharing it
with me.
>
> >
> If the change addresses a behavioral bug (e.g., incorrect NAPI completion behavior when budget is zero),
> add Fixes: <sha1> ("commit subject") to help backporting and tracking.
Well, it's not a bugfix. I just pointed out that the i40e patch that
has a bug was overwritten/buried by another patch :)
Thanks,
Jason
>
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> > In this version, I keep it as is (please see the following link)
> > https://lore.kernel.org/intel-wired-
> > lan/CAL+tcoAUW_J62aw3aGBru+0GmaTjoom1qu8Y=aiSc9EGU09Nww@...l.gmail.c
> > om/
> > ---
> > 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
>
Powered by blists - more mailing lists