* [PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit
@ 2025-07-21 8:33 Jason Xing
2025-07-21 8:33 ` [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode Jason Xing
2025-07-21 8:33 ` [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts " Jason Xing
0 siblings, 2 replies; 14+ messages in thread
From: Jason Xing @ 2025-07-21 8:33 UTC (permalink / raw)
To: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue
Cc: linux-stm32, bpf, intel-wired-lan, netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
Fix two underflow issues around {stmmac_xdp|igb}_xmit_zc().
Jason Xing (2):
stmmac: xsk: fix underflow of budget in zerocopy mode
igb: xsk: solve underflow of nb_pkts in zerocopy mode
drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
--
2.41.3
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 8:33 [PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit Jason Xing
@ 2025-07-21 8:33 ` Jason Xing
2025-07-21 8:56 ` [Intel-wired-lan] " Paul Menzel
2025-07-21 15:37 ` Stanislav Fomichev
2025-07-21 8:33 ` [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts " Jason Xing
1 sibling, 2 replies; 14+ messages in thread
From: Jason Xing @ 2025-07-21 8:33 UTC (permalink / raw)
To: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue
Cc: linux-stm32, bpf, intel-wired-lan, netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
The issue can happen when the budget number of descs are consumed. As
long as the budget is decreased to zero, it will again go into
while (budget-- > 0) statement and get decreased by one, so the
underflow issue can happen. It will lead to returning true whereas the
expected value should be false.
In this case where all the budget are used up, it means zc function
should return false to let the poll run again because normally we
might have more data to process.
Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f350a6662880..ea5541f9e9a6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
budget = min(budget, stmmac_tx_avail(priv, queue));
- while (budget-- > 0) {
+ while (budget > 0) {
struct stmmac_metadata_request meta_req;
struct xsk_tx_metadata *meta = NULL;
dma_addr_t dma_addr;
@@ -2681,6 +2681,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
tx_q->cur_tx = STMMAC_GET_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
entry = tx_q->cur_tx;
+
+ budget--;
}
u64_stats_update_begin(&txq_stats->napi_syncp);
u64_stats_add(&txq_stats->napi.tx_set_ic_bit, tx_set_ic_bit);
--
2.41.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode
2025-07-21 8:33 [PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit Jason Xing
2025-07-21 8:33 ` [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode Jason Xing
@ 2025-07-21 8:33 ` Jason Xing
2025-07-21 10:12 ` Simon Horman
1 sibling, 1 reply; 14+ messages in thread
From: Jason Xing @ 2025-07-21 8:33 UTC (permalink / raw)
To: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue
Cc: linux-stm32, bpf, intel-wired-lan, netdev, Jason Xing
From: Jason Xing <kernelxing@tencent.com>
There is no break time in the while() loop, so every time at the end of
igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
value always false. But theoretically, the result should be set after
calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
a good example.
Returning false means we're not done with transmission and we need one
more poll, which is exactly what igb_xmit_zc() always did before this
patch. After this patch, the return value depends on the nb_pkts value.
Two cases might happen then:
1. if (nb_pkts < budget), it means we process all the possible data, so
return true and no more necessary poll will be triggered because of
this.
2. if (nb_pkts == budget), it means we might have more data, so return
false to let another poll run again.
Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index 5cf67ba29269..243f4246fee8 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
if (!nb_pkts)
return true;
- while (nb_pkts-- > 0) {
+ while (i < nb_pkts) {
dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
--
2.41.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 8:33 ` [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode Jason Xing
@ 2025-07-21 8:56 ` Paul Menzel
2025-07-21 9:15 ` Jason Xing
2025-07-21 15:37 ` Stanislav Fomichev
1 sibling, 1 reply; 14+ messages in thread
From: Paul Menzel @ 2025-07-21 8:56 UTC (permalink / raw)
To: Jason Xing
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
Dear Jason,
Thank you for your patch.
Am 21.07.25 um 10:33 schrieb Jason Xing:
> From: Jason Xing <kernelxing@tencent.com>
>
> The issue can happen when the budget number of descs are consumed. As
Instead of “The issue”, I’d use “An underflow …”.
> long as the budget is decreased to zero, it will again go into
> while (budget-- > 0) statement and get decreased by one, so the
> underflow issue can happen. It will lead to returning true whereas the
> expected value should be false.
What is “it”?
> In this case where all the budget are used up, it means zc function
*is* used?
> should return false to let the poll run again because normally we
> might have more data to process.
Do you have a reproducer, you could add to the commit message?
> Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f350a6662880..ea5541f9e9a6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>
> budget = min(budget, stmmac_tx_avail(priv, queue));
>
> - while (budget-- > 0) {
> + while (budget > 0) {
So, if the while loop should not be entered with budget being 0, then
the line could be changed to `while (--budget > 0) {`? But then it
wouldn’t be called for budget being 1.
A for loop might be the better choice for a loop with budget as counting
variable?
> struct stmmac_metadata_request meta_req;
> struct xsk_tx_metadata *meta = NULL;
> dma_addr_t dma_addr;
> @@ -2681,6 +2681,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>
> tx_q->cur_tx = STMMAC_GET_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
> entry = tx_q->cur_tx;
> +
> + budget--;
> }
> u64_stats_update_begin(&txq_stats->napi_syncp);
> u64_stats_add(&txq_stats->napi.tx_set_ic_bit, tx_set_ic_bit);
Excuse my ignorance, but I do not yet see the problem that the while
loop is entered and buffer is set to 0. Is it later the return condition?
return !!budget && work_done;
Kind regards,
Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 8:56 ` [Intel-wired-lan] " Paul Menzel
@ 2025-07-21 9:15 ` Jason Xing
2025-07-22 14:16 ` Willem de Bruijn
0 siblings, 1 reply; 14+ messages in thread
From: Jason Xing @ 2025-07-21 9:15 UTC (permalink / raw)
To: Paul Menzel
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
Hi Paul,
On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Jason,
>
>
> Thank you for your patch.
Thanks for your quick response and review :)
>
> Am 21.07.25 um 10:33 schrieb Jason Xing:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The issue can happen when the budget number of descs are consumed. As
>
> Instead of “The issue”, I’d use “An underflow …”.
Will change it.
>
> > long as the budget is decreased to zero, it will again go into
> > while (budget-- > 0) statement and get decreased by one, so the
> > underflow issue can happen. It will lead to returning true whereas the
> > expected value should be false.
>
> What is “it”?
It means 'underflow of budget' behavior.
>
> > In this case where all the budget are used up, it means zc function
>
> *is* used?
Got it.
>
> > should return false to let the poll run again because normally we
> > might have more data to process.
>
> Do you have a reproducer, you could add to the commit message?
Sorry, I didn't have a reproducer. I cooked this patch after analyzing
the whole logic (because recently I'm reading the zc xmit
implementation among various drivers.)
>
> > Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f350a6662880..ea5541f9e9a6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> >
> > budget = min(budget, stmmac_tx_avail(priv, queue));
> >
> > - while (budget-- > 0) {
> > + while (budget > 0) {
>
> So, if the while loop should not be entered with budget being 0, then
> the line could be changed to `while (--budget > 0) {`? But then it
> wouldn’t be called for budget being 1.
Right, so it shouldn't be '--budget'.
>
> A for loop might be the better choice for a loop with budget as counting
> variable?
Sorry, I didn't follow you.
>
> > struct stmmac_metadata_request meta_req;
> > struct xsk_tx_metadata *meta = NULL;
> > dma_addr_t dma_addr;
> > @@ -2681,6 +2681,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> >
> > tx_q->cur_tx = STMMAC_GET_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
> > entry = tx_q->cur_tx;
> > +
> > + budget--;
> > }
> > u64_stats_update_begin(&txq_stats->napi_syncp);
> > u64_stats_add(&txq_stats->napi.tx_set_ic_bit, tx_set_ic_bit);
>
> Excuse my ignorance, but I do not yet see the problem that the while
> loop is entered and buffer is set to 0. Is it later the return condition?
Let me give a simple example. Supposing the budget is one initially,
at the first round, the budget will be zero. Later, after this desc
being processed, the 'while (budget-- > 0)' statement will be accessed
again, and then the budget will be decreased by one which is u32(0 -
1), namely, UINT_MAX. !!UINT_MAX is true while the expected return
value is false (!!0, 0 is the expected budget value).
i40e_clean_tx_irq() handles this correctly, FYI.
Thanks,
Jason
>
> return !!budget && work_done;
>
>
> Kind regards,
>
> Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode
2025-07-21 8:33 ` [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts " Jason Xing
@ 2025-07-21 10:12 ` Simon Horman
2025-07-21 10:22 ` Jason Xing
0 siblings, 1 reply; 14+ messages in thread
From: Simon Horman @ 2025-07-21 10:12 UTC (permalink / raw)
To: Jason Xing
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
On Mon, Jul 21, 2025 at 04:33:43PM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
>
> There is no break time in the while() loop, so every time at the end of
> igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
> value always false. But theoretically, the result should be set after
> calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
> a good example.
>
> Returning false means we're not done with transmission and we need one
> more poll, which is exactly what igb_xmit_zc() always did before this
> patch. After this patch, the return value depends on the nb_pkts value.
> Two cases might happen then:
> 1. if (nb_pkts < budget), it means we process all the possible data, so
> return true and no more necessary poll will be triggered because of
> this.
> 2. if (nb_pkts == budget), it means we might have more data, so return
> false to let another poll run again.
>
> Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index 5cf67ba29269..243f4246fee8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
> if (!nb_pkts)
> return true;
>
> - while (nb_pkts-- > 0) {
> + while (i < nb_pkts) {
Hi Jason,
FWIIW, I think using a for loop is a more idiomatic way
of handling the relationship between i, nb_pkts, and
the iterations of this loop.
> dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
>
> --
> 2.41.3
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts in zerocopy mode
2025-07-21 10:12 ` Simon Horman
@ 2025-07-21 10:22 ` Jason Xing
0 siblings, 0 replies; 14+ messages in thread
From: Jason Xing @ 2025-07-21 10:22 UTC (permalink / raw)
To: Simon Horman
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
Hi Simon,
On Mon, Jul 21, 2025 at 6:12 PM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jul 21, 2025 at 04:33:43PM +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > There is no break time in the while() loop, so every time at the end of
> > igb_xmit_zc(), underflow of nb_pkts will occur, which renders the return
> > value always false. But theoretically, the result should be set after
> > calling xsk_tx_peek_release_desc_batch(). We can take i40e_xmit_zc() as
> > a good example.
> >
> > Returning false means we're not done with transmission and we need one
> > more poll, which is exactly what igb_xmit_zc() always did before this
> > patch. After this patch, the return value depends on the nb_pkts value.
> > Two cases might happen then:
> > 1. if (nb_pkts < budget), it means we process all the possible data, so
> > return true and no more necessary poll will be triggered because of
> > this.
> > 2. if (nb_pkts == budget), it means we might have more data, so return
> > false to let another poll run again.
> >
> > Fixes: f8e284a02afc ("igb: Add AF_XDP zero-copy Tx support")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > drivers/net/ethernet/intel/igb/igb_xsk.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > index 5cf67ba29269..243f4246fee8 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > @@ -482,7 +482,7 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
> > if (!nb_pkts)
> > return true;
> >
> > - while (nb_pkts-- > 0) {
> > + while (i < nb_pkts) {
>
> Hi Jason,
>
> FWIIW, I think using a for loop is a more idiomatic way
> of handling the relationship between i, nb_pkts, and
> the iterations of this loop.
Sure, I can turn it into 'for (i = 0; i < nb_pkts; i++)' in the next version.
Thanks,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 8:33 ` [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode Jason Xing
2025-07-21 8:56 ` [Intel-wired-lan] " Paul Menzel
@ 2025-07-21 15:37 ` Stanislav Fomichev
2025-07-21 23:05 ` Jason Xing
1 sibling, 1 reply; 14+ messages in thread
From: Stanislav Fomichev @ 2025-07-21 15:37 UTC (permalink / raw)
To: Jason Xing
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
On 07/21, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
>
> The issue can happen when the budget number of descs are consumed. As
> long as the budget is decreased to zero, it will again go into
> while (budget-- > 0) statement and get decreased by one, so the
> underflow issue can happen. It will lead to returning true whereas the
> expected value should be false.
>
> In this case where all the budget are used up, it means zc function
> should return false to let the poll run again because normally we
> might have more data to process.
>
> Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f350a6662880..ea5541f9e9a6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>
> budget = min(budget, stmmac_tx_avail(priv, queue));
>
> - while (budget-- > 0) {
> + while (budget > 0) {
There is a continue on line 2621. Should we do 'for (; budget > 0; budget--)'
instead? And maybe the same for ixgbe [0]?
0: https://lore.kernel.org/netdev/20250720091123.474-3-kerneljasonxing@gmail.com/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 15:37 ` Stanislav Fomichev
@ 2025-07-21 23:05 ` Jason Xing
2025-07-22 0:12 ` Jason Xing
0 siblings, 1 reply; 14+ messages in thread
From: Jason Xing @ 2025-07-21 23:05 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
<stfomichev@gmail.com> wrote:
>
> On 07/21, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The issue can happen when the budget number of descs are consumed. As
> > long as the budget is decreased to zero, it will again go into
> > while (budget-- > 0) statement and get decreased by one, so the
> > underflow issue can happen. It will lead to returning true whereas the
> > expected value should be false.
> >
> > In this case where all the budget are used up, it means zc function
> > should return false to let the poll run again because normally we
> > might have more data to process.
> >
> > Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f350a6662880..ea5541f9e9a6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> >
> > budget = min(budget, stmmac_tx_avail(priv, queue));
> >
> > - while (budget-- > 0) {
> > + while (budget > 0) {
>
> There is a continue on line 2621.
Thanks for catching this!
> Should we do 'for (; budget > 0; budget--)'
> instead? And maybe the same for ixgbe [0]?
Not really. I think I can move the 'budget--' just before the
'continue' part. If we convert it to use 'for' loop and then we end up
with one of 'break' statements, the budget still gets accidently
increased by one whereas ixgbe driver even doesn't handle the desc
this time. IIUC, it should not happen, right?
>
> 0: https://lore.kernel.org/netdev/20250720091123.474-3-kerneljasonxing@gmail.com/
The same logic as above can be applied here as well. There are three
'break' statements in ixgbe_xmit_zc().
Hence, IMHO, I prefer to use while(...) in this case but I ought to
adjust the position of budget--.
Thanks,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 23:05 ` Jason Xing
@ 2025-07-22 0:12 ` Jason Xing
0 siblings, 0 replies; 14+ messages in thread
From: Jason Xing @ 2025-07-22 0:12 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
On Tue, Jul 22, 2025 at 7:05 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Mon, Jul 21, 2025 at 11:37 PM Stanislav Fomichev
> <stfomichev@gmail.com> wrote:
> >
> > On 07/21, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > The issue can happen when the budget number of descs are consumed. As
> > > long as the budget is decreased to zero, it will again go into
> > > while (budget-- > 0) statement and get decreased by one, so the
> > > underflow issue can happen. It will lead to returning true whereas the
> > > expected value should be false.
> > >
> > > In this case where all the budget are used up, it means zc function
> > > should return false to let the poll run again because normally we
> > > might have more data to process.
> > >
> > > Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index f350a6662880..ea5541f9e9a6 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> > >
> > > budget = min(budget, stmmac_tx_avail(priv, queue));
> > >
> > > - while (budget-- > 0) {
> > > + while (budget > 0) {
> >
> > There is a continue on line 2621.
>
> Thanks for catching this!
>
> > Should we do 'for (; budget > 0; budget--)'
> > instead? And maybe the same for ixgbe [0]?
>
> Not really. I think I can move the 'budget--' just before the
> 'continue' part. If we convert it to use 'for' loop and then we end up
> with one of 'break' statements, the budget still gets accidently
> increased by one whereas ixgbe driver even doesn't handle the desc
> this time. IIUC, it should not happen, right?
Sorry, I was totally wrong. Your suggestions work and I will revise
them as you said :)
Thanks,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-21 9:15 ` Jason Xing
@ 2025-07-22 14:16 ` Willem de Bruijn
2025-07-22 15:53 ` Jason Xing
0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2025-07-22 14:16 UTC (permalink / raw)
To: Jason Xing, Paul Menzel
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
Jason Xing wrote:
> Hi Paul,
>
> On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Jason,
> >
> >
> > Thank you for your patch.
>
> Thanks for your quick response and review :)
>
> >
> > Am 21.07.25 um 10:33 schrieb Jason Xing:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > The issue can happen when the budget number of descs are consumed. As
> >
> > Instead of “The issue”, I’d use “An underflow …”.
>
> Will change it.
>
> >
> > > long as the budget is decreased to zero, it will again go into
> > > while (budget-- > 0) statement and get decreased by one, so the
> > > underflow issue can happen. It will lead to returning true whereas the
> > > expected value should be false.
> >
> > What is “it”?
>
> It means 'underflow of budget' behavior.
A technicality, but this is (negative) overflow.
Underflow is a computation that results in a value that is too small
to be represented by the given type.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-22 14:16 ` Willem de Bruijn
@ 2025-07-22 15:53 ` Jason Xing
2025-07-22 17:29 ` Willem de Bruijn
0 siblings, 1 reply; 14+ messages in thread
From: Jason Xing @ 2025-07-22 15:53 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Paul Menzel, anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev,
davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
On Tue, Jul 22, 2025 at 10:16 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > Hi Paul,
> >
> > On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > >
> > > Dear Jason,
> > >
> > >
> > > Thank you for your patch.
> >
> > Thanks for your quick response and review :)
> >
> > >
> > > Am 21.07.25 um 10:33 schrieb Jason Xing:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > The issue can happen when the budget number of descs are consumed. As
> > >
> > > Instead of “The issue”, I’d use “An underflow …”.
> >
> > Will change it.
> >
> > >
> > > > long as the budget is decreased to zero, it will again go into
> > > > while (budget-- > 0) statement and get decreased by one, so the
> > > > underflow issue can happen. It will lead to returning true whereas the
> > > > expected value should be false.
> > >
> > > What is “it”?
> >
> > It means 'underflow of budget' behavior.
>
> A technicality, but this is (negative) overflow.
>
> Underflow is a computation that results in a value that is too small
> to be represented by the given type.
Interesting. Thanks for sharing this with me:)
I just checked the wikipedia[1] that says " Underflow can in part be
regarded as negative overflow of the exponent of the floating-point
value.". I assume this rule can also be applied in this case? I'm
hesitant to send the v3 patch tomorrow with this 'negative overflow'
term included.
[1]: https://en.wikipedia.org/wiki/Arithmetic_underflow
Thanks,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-22 15:53 ` Jason Xing
@ 2025-07-22 17:29 ` Willem de Bruijn
2025-07-22 23:04 ` Jason Xing
0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2025-07-22 17:29 UTC (permalink / raw)
To: Jason Xing, Willem de Bruijn
Cc: Paul Menzel, anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev,
davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
Jason Xing wrote:
> On Tue, Jul 22, 2025 at 10:16 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > Hi Paul,
> > >
> > > On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > > >
> > > > Dear Jason,
> > > >
> > > >
> > > > Thank you for your patch.
> > >
> > > Thanks for your quick response and review :)
> > >
> > > >
> > > > Am 21.07.25 um 10:33 schrieb Jason Xing:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > The issue can happen when the budget number of descs are consumed. As
> > > >
> > > > Instead of “The issue”, I’d use “An underflow …”.
> > >
> > > Will change it.
> > >
> > > >
> > > > > long as the budget is decreased to zero, it will again go into
> > > > > while (budget-- > 0) statement and get decreased by one, so the
> > > > > underflow issue can happen. It will lead to returning true whereas the
> > > > > expected value should be false.
> > > >
> > > > What is “it”?
> > >
> > > It means 'underflow of budget' behavior.
> >
> > A technicality, but this is (negative) overflow.
> >
> > Underflow is a computation that results in a value that is too small
> > to be represented by the given type.
>
> Interesting. Thanks for sharing this with me:)
>
> I just checked the wikipedia[1] that says " Underflow can in part be
> regarded as negative overflow of the exponent of the floating-point
> value.". I assume this rule can also be applied in this case? I'm
> hesitant to send the v3 patch tomorrow with this 'negative overflow'
> term included.
My point is very pedantic. I think these cases are not underflow.
But it is often called that, understandably. So choose as you see fit.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode
2025-07-22 17:29 ` Willem de Bruijn
@ 2025-07-22 23:04 ` Jason Xing
0 siblings, 0 replies; 14+ messages in thread
From: Jason Xing @ 2025-07-22 23:04 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Paul Menzel, anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev,
davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, mcoquelin.stm32, alexandre.torgue, linux-stm32,
bpf, intel-wired-lan, netdev, Jason Xing
On Wed, Jul 23, 2025 at 1:29 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Tue, Jul 22, 2025 at 10:16 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > Hi Paul,
> > > >
> > > > On Mon, Jul 21, 2025 at 4:56 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > > > >
> > > > > Dear Jason,
> > > > >
> > > > >
> > > > > Thank you for your patch.
> > > >
> > > > Thanks for your quick response and review :)
> > > >
> > > > >
> > > > > Am 21.07.25 um 10:33 schrieb Jason Xing:
> > > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > > >
> > > > > > The issue can happen when the budget number of descs are consumed. As
> > > > >
> > > > > Instead of “The issue”, I’d use “An underflow …”.
> > > >
> > > > Will change it.
> > > >
> > > > >
> > > > > > long as the budget is decreased to zero, it will again go into
> > > > > > while (budget-- > 0) statement and get decreased by one, so the
> > > > > > underflow issue can happen. It will lead to returning true whereas the
> > > > > > expected value should be false.
> > > > >
> > > > > What is “it”?
> > > >
> > > > It means 'underflow of budget' behavior.
> > >
> > > A technicality, but this is (negative) overflow.
> > >
> > > Underflow is a computation that results in a value that is too small
> > > to be represented by the given type.
> >
> > Interesting. Thanks for sharing this with me:)
> >
> > I just checked the wikipedia[1] that says " Underflow can in part be
> > regarded as negative overflow of the exponent of the floating-point
> > value.". I assume this rule can also be applied in this case? I'm
> > hesitant to send the v3 patch tomorrow with this 'negative overflow'
> > term included.
>
> My point is very pedantic. I think these cases are not underflow.
>
> But it is often called that, understandably. So choose as you see fit.
I see. Thanks for pointing that out. I will change it :)
Thanks,
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-07-22 23:05 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-21 8:33 [PATCH net-next 0/2] xsk: fix underflow issues in zerocopy xmit Jason Xing
2025-07-21 8:33 ` [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode Jason Xing
2025-07-21 8:56 ` [Intel-wired-lan] " Paul Menzel
2025-07-21 9:15 ` Jason Xing
2025-07-22 14:16 ` Willem de Bruijn
2025-07-22 15:53 ` Jason Xing
2025-07-22 17:29 ` Willem de Bruijn
2025-07-22 23:04 ` Jason Xing
2025-07-21 15:37 ` Stanislav Fomichev
2025-07-21 23:05 ` Jason Xing
2025-07-22 0:12 ` Jason Xing
2025-07-21 8:33 ` [PATCH net-next 2/2] igb: xsk: solve underflow of nb_pkts " Jason Xing
2025-07-21 10:12 ` Simon Horman
2025-07-21 10:22 ` Jason Xing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).