[PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Resolve the budget negative overflow which leads to returning true in
ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.

Before this patch, when the budget is decreased to zero and finishes
sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
and enter into the while() statement to see if it should keep processing
packets, but in the meantime it unexpectedly decreases the value again to
'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
true, showing 'we complete cleaning the budget'. That also means
'clean_complete = true' in ixgbe_poll.

The true theory behind this is if that budget number of descs are consumed,
it implies that we might have more descs to be done. So we should return
false in ixgbe_xmit_zc to tell napi poll to find another chance to start
polling to handle the rest of descs. On the contrary, returning true here
means job done and we know we finish all the possible descs this time and
we don't intend to start a new napi poll.

It is apparently against our expectations. Please also see how
ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
to make sure the budget can be decreased to zero at most and the negative
overflow never happens.

The patch adds 'likely' because we rarely would not hit the loop codition
since the standard budget is 256.

Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
---
Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
1. use 'negative overflow' instead of 'underflow' (Willem)
2. add reviewed-by tag (Larysa)
3. target iwl-net branch (Larysa)
4. add the reason why the patch adds likely() (Larysa)
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index ac58964b2f08..7b941505a9d0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
 	dma_addr_t dma;
 	u32 cmd_type;
 
-	while (budget-- > 0) {
+	while (likely(budget)) {
 		if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
 			work_done = false;
 			break;
@@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
 		xdp_ring->next_to_use++;
 		if (xdp_ring->next_to_use == xdp_ring->count)
 			xdp_ring->next_to_use = 0;
+
+		budget--;
 	}
 
 	if (tx_desc) {
-- 
2.41.3

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Paul Menzel]

Dear Jason,


Thank you for the improved version.

Am 26.07.25 um 09:03 schrieb Jason Xing:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Resolve the budget negative overflow which leads to returning true in
> ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> 
> Before this patch, when the budget is decreased to zero and finishes
> sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> and enter into the while() statement to see if it should keep processing
> packets, but in the meantime it unexpectedly decreases the value again to
> 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> true, showing 'we complete cleaning the budget'. That also means
> 'clean_complete = true' in ixgbe_poll.
> 
> The true theory behind this is if that budget number of descs are consumed,
> it implies that we might have more descs to be done. So we should return
> false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> polling to handle the rest of descs. On the contrary, returning true here
> means job done and we know we finish all the possible descs this time and
> we don't intend to start a new napi poll.
> 
> It is apparently against our expectations. Please also see how
> ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> to make sure the budget can be decreased to zero at most and the negative
> overflow never happens.
> 
> The patch adds 'likely' because we rarely would not hit the loop codition
> since the standard budget is 256.
> 
> Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
> Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
> 1. use 'negative overflow' instead of 'underflow' (Willem)
> 2. add reviewed-by tag (Larysa)
> 3. target iwl-net branch (Larysa)
> 4. add the reason why the patch adds likely() (Larysa)
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> index ac58964b2f08..7b941505a9d0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
>   	dma_addr_t dma;
>   	u32 cmd_type;
>   
> -	while (budget-- > 0) {
> +	while (likely(budget)) {
>   		if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
>   			work_done = false;
>   			break;
> @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
>   		xdp_ring->next_to_use++;
>   		if (xdp_ring->next_to_use == xdp_ring->count)
>   			xdp_ring->next_to_use = 0;
> +
> +		budget--;
>   	}
>   
>   	if (tx_desc) {

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Is this just the smallest fix, and the rewrite to the more idiomatic for 
loop going to be done in a follow-up?


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Jason Xing]

Hi Paul,

On Sun, Jul 27, 2025 at 4:36 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Jason,
>
>
> Thank you for the improved version.
>
> Am 26.07.25 um 09:03 schrieb Jason Xing:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Resolve the budget negative overflow which leads to returning true in
> > ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> >
> > Before this patch, when the budget is decreased to zero and finishes
> > sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> > and enter into the while() statement to see if it should keep processing
> > packets, but in the meantime it unexpectedly decreases the value again to
> > 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> > true, showing 'we complete cleaning the budget'. That also means
> > 'clean_complete = true' in ixgbe_poll.
> >
> > The true theory behind this is if that budget number of descs are consumed,
> > it implies that we might have more descs to be done. So we should return
> > false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> > polling to handle the rest of descs. On the contrary, returning true here
> > means job done and we know we finish all the possible descs this time and
> > we don't intend to start a new napi poll.
> >
> > It is apparently against our expectations. Please also see how
> > ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> > to make sure the budget can be decreased to zero at most and the negative
> > overflow never happens.
> >
> > The patch adds 'likely' because we rarely would not hit the loop codition
> > since the standard budget is 256.
> >
> > Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> > Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
> > 1. use 'negative overflow' instead of 'underflow' (Willem)
> > 2. add reviewed-by tag (Larysa)
> > 3. target iwl-net branch (Larysa)
> > 4. add the reason why the patch adds likely() (Larysa)
> > ---
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > index ac58964b2f08..7b941505a9d0 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> >       dma_addr_t dma;
> >       u32 cmd_type;
> >
> > -     while (budget-- > 0) {
> > +     while (likely(budget)) {
> >               if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
> >                       work_done = false;
> >                       break;
> > @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> >               xdp_ring->next_to_use++;
> >               if (xdp_ring->next_to_use == xdp_ring->count)
> >                       xdp_ring->next_to_use = 0;
> > +
> > +             budget--;
> >       }
> >
> >       if (tx_desc) {
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
> Is this just the smallest fix, and the rewrite to the more idiomatic for
> loop going to be done in a follow-up?

Thanks for the review. But I'm not that sure if it's worth a follow-up
patch. Or if anyone else also expects to see a 'for loop' version, I
can send a V3 patch then. I have no strong opinion either way.

Thanks,
Jason

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Simon Horman]

On Sun, Jul 27, 2025 at 06:06:55PM +0800, Jason Xing wrote:
> Hi Paul,
> 
> On Sun, Jul 27, 2025 at 4:36 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Jason,
> >
> >
> > Thank you for the improved version.
> >
> > Am 26.07.25 um 09:03 schrieb Jason Xing:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Resolve the budget negative overflow which leads to returning true in
> > > ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> > >
> > > Before this patch, when the budget is decreased to zero and finishes
> > > sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> > > and enter into the while() statement to see if it should keep processing
> > > packets, but in the meantime it unexpectedly decreases the value again to
> > > 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> > > true, showing 'we complete cleaning the budget'. That also means
> > > 'clean_complete = true' in ixgbe_poll.
> > >
> > > The true theory behind this is if that budget number of descs are consumed,
> > > it implies that we might have more descs to be done. So we should return
> > > false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> > > polling to handle the rest of descs. On the contrary, returning true here
> > > means job done and we know we finish all the possible descs this time and
> > > we don't intend to start a new napi poll.
> > >
> > > It is apparently against our expectations. Please also see how
> > > ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> > > to make sure the budget can be decreased to zero at most and the negative
> > > overflow never happens.
> > >
> > > The patch adds 'likely' because we rarely would not hit the loop codition
> > > since the standard budget is 256.
> > >
> > > Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > ---
> > > Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
> > > 1. use 'negative overflow' instead of 'underflow' (Willem)
> > > 2. add reviewed-by tag (Larysa)
> > > 3. target iwl-net branch (Larysa)
> > > 4. add the reason why the patch adds likely() (Larysa)
> > > ---
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > index ac58964b2f08..7b941505a9d0 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > >       dma_addr_t dma;
> > >       u32 cmd_type;
> > >
> > > -     while (budget-- > 0) {
> > > +     while (likely(budget)) {
> > >               if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
> > >                       work_done = false;
> > >                       break;
> > > @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > >               xdp_ring->next_to_use++;
> > >               if (xdp_ring->next_to_use == xdp_ring->count)
> > >                       xdp_ring->next_to_use = 0;
> > > +
> > > +             budget--;
> > >       }
> > >
> > >       if (tx_desc) {
> >
> > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> >
> > Is this just the smallest fix, and the rewrite to the more idiomatic for
> > loop going to be done in a follow-up?
> 
> Thanks for the review. But I'm not that sure if it's worth a follow-up
> patch. Or if anyone else also expects to see a 'for loop' version, I
> can send a V3 patch then. I have no strong opinion either way.

I think we have iterated over this enough (pun intended).
If this patch is correct then lets stick with this approach.

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Jason Xing]

Hi Simon,

On Sun, Jul 27, 2025 at 9:55 PM Simon Horman <horms@kernel.org> wrote:
>
> On Sun, Jul 27, 2025 at 06:06:55PM +0800, Jason Xing wrote:
> > Hi Paul,
> >
> > On Sun, Jul 27, 2025 at 4:36 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > >
> > > Dear Jason,
> > >
> > >
> > > Thank you for the improved version.
> > >
> > > Am 26.07.25 um 09:03 schrieb Jason Xing:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Resolve the budget negative overflow which leads to returning true in
> > > > ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> > > >
> > > > Before this patch, when the budget is decreased to zero and finishes
> > > > sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> > > > and enter into the while() statement to see if it should keep processing
> > > > packets, but in the meantime it unexpectedly decreases the value again to
> > > > 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> > > > true, showing 'we complete cleaning the budget'. That also means
> > > > 'clean_complete = true' in ixgbe_poll.
> > > >
> > > > The true theory behind this is if that budget number of descs are consumed,
> > > > it implies that we might have more descs to be done. So we should return
> > > > false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> > > > polling to handle the rest of descs. On the contrary, returning true here
> > > > means job done and we know we finish all the possible descs this time and
> > > > we don't intend to start a new napi poll.
> > > >
> > > > It is apparently against our expectations. Please also see how
> > > > ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> > > > to make sure the budget can be decreased to zero at most and the negative
> > > > overflow never happens.
> > > >
> > > > The patch adds 'likely' because we rarely would not hit the loop codition
> > > > since the standard budget is 256.
> > > >
> > > > Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > ---
> > > > Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
> > > > 1. use 'negative overflow' instead of 'underflow' (Willem)
> > > > 2. add reviewed-by tag (Larysa)
> > > > 3. target iwl-net branch (Larysa)
> > > > 4. add the reason why the patch adds likely() (Larysa)
> > > > ---
> > > >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > index ac58964b2f08..7b941505a9d0 100644
> > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > > >       dma_addr_t dma;
> > > >       u32 cmd_type;
> > > >
> > > > -     while (budget-- > 0) {
> > > > +     while (likely(budget)) {
> > > >               if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
> > > >                       work_done = false;
> > > >                       break;
> > > > @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > > >               xdp_ring->next_to_use++;
> > > >               if (xdp_ring->next_to_use == xdp_ring->count)
> > > >                       xdp_ring->next_to_use = 0;
> > > > +
> > > > +             budget--;
> > > >       }
> > > >
> > > >       if (tx_desc) {
> > >
> > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > >
> > > Is this just the smallest fix, and the rewrite to the more idiomatic for
> > > loop going to be done in a follow-up?
> >
> > Thanks for the review. But I'm not that sure if it's worth a follow-up
> > patch. Or if anyone else also expects to see a 'for loop' version, I
> > can send a V3 patch then. I have no strong opinion either way.
>
> I think we have iterated over this enough (pun intended).

Hhh, interesting.

> If this patch is correct then lets stick with this approach.

No problem. Tomorrow I will send the 'for loop' version :)

Thanks,
Jason

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Simon Horman]

On Sun, Jul 27, 2025 at 10:16:10PM +0800, Jason Xing wrote:
> Hi Simon,
> 
> On Sun, Jul 27, 2025 at 9:55 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Sun, Jul 27, 2025 at 06:06:55PM +0800, Jason Xing wrote:
> > > Hi Paul,
> > >
> > > On Sun, Jul 27, 2025 at 4:36 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > > >
> > > > Dear Jason,
> > > >
> > > >
> > > > Thank you for the improved version.
> > > >
> > > > Am 26.07.25 um 09:03 schrieb Jason Xing:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > Resolve the budget negative overflow which leads to returning true in
> > > > > ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> > > > >
> > > > > Before this patch, when the budget is decreased to zero and finishes
> > > > > sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> > > > > and enter into the while() statement to see if it should keep processing
> > > > > packets, but in the meantime it unexpectedly decreases the value again to
> > > > > 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> > > > > true, showing 'we complete cleaning the budget'. That also means
> > > > > 'clean_complete = true' in ixgbe_poll.
> > > > >
> > > > > The true theory behind this is if that budget number of descs are consumed,
> > > > > it implies that we might have more descs to be done. So we should return
> > > > > false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> > > > > polling to handle the rest of descs. On the contrary, returning true here
> > > > > means job done and we know we finish all the possible descs this time and
> > > > > we don't intend to start a new napi poll.
> > > > >
> > > > > It is apparently against our expectations. Please also see how
> > > > > ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> > > > > to make sure the budget can be decreased to zero at most and the negative
> > > > > overflow never happens.
> > > > >
> > > > > The patch adds 'likely' because we rarely would not hit the loop codition
> > > > > since the standard budget is 256.
> > > > >
> > > > > Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > > ---
> > > > > Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
> > > > > 1. use 'negative overflow' instead of 'underflow' (Willem)
> > > > > 2. add reviewed-by tag (Larysa)
> > > > > 3. target iwl-net branch (Larysa)
> > > > > 4. add the reason why the patch adds likely() (Larysa)
> > > > > ---
> > > > >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
> > > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > > index ac58964b2f08..7b941505a9d0 100644
> > > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > > @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > > > >       dma_addr_t dma;
> > > > >       u32 cmd_type;
> > > > >
> > > > > -     while (budget-- > 0) {
> > > > > +     while (likely(budget)) {
> > > > >               if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
> > > > >                       work_done = false;
> > > > >                       break;
> > > > > @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > > > >               xdp_ring->next_to_use++;
> > > > >               if (xdp_ring->next_to_use == xdp_ring->count)
> > > > >                       xdp_ring->next_to_use = 0;
> > > > > +
> > > > > +             budget--;
> > > > >       }
> > > > >
> > > > >       if (tx_desc) {
> > > >
> > > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > > >
> > > > Is this just the smallest fix, and the rewrite to the more idiomatic for
> > > > loop going to be done in a follow-up?
> > >
> > > Thanks for the review. But I'm not that sure if it's worth a follow-up
> > > patch. Or if anyone else also expects to see a 'for loop' version, I
> > > can send a V3 patch then. I have no strong opinion either way.
> >
> > I think we have iterated over this enough (pun intended).
> 
> Hhh, interesting.
> 
> > If this patch is correct then lets stick with this approach.
> 
> No problem. Tomorrow I will send the 'for loop' version :)

I meant, I think the while loop is fine.
But anything that is correct is fine by me :)

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Jason Xing]

On Sun, Jul 27, 2025 at 10:47 PM Simon Horman <horms@kernel.org> wrote:
>
> On Sun, Jul 27, 2025 at 10:16:10PM +0800, Jason Xing wrote:
> > Hi Simon,
> >
> > On Sun, Jul 27, 2025 at 9:55 PM Simon Horman <horms@kernel.org> wrote:
> > >
> > > On Sun, Jul 27, 2025 at 06:06:55PM +0800, Jason Xing wrote:
> > > > Hi Paul,
> > > >
> > > > On Sun, Jul 27, 2025 at 4:36 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > > > >
> > > > > Dear Jason,
> > > > >
> > > > >
> > > > > Thank you for the improved version.
> > > > >
> > > > > Am 26.07.25 um 09:03 schrieb Jason Xing:
> > > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > > >
> > > > > > Resolve the budget negative overflow which leads to returning true in
> > > > > > ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> > > > > >
> > > > > > Before this patch, when the budget is decreased to zero and finishes
> > > > > > sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> > > > > > and enter into the while() statement to see if it should keep processing
> > > > > > packets, but in the meantime it unexpectedly decreases the value again to
> > > > > > 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> > > > > > true, showing 'we complete cleaning the budget'. That also means
> > > > > > 'clean_complete = true' in ixgbe_poll.
> > > > > >
> > > > > > The true theory behind this is if that budget number of descs are consumed,
> > > > > > it implies that we might have more descs to be done. So we should return
> > > > > > false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> > > > > > polling to handle the rest of descs. On the contrary, returning true here
> > > > > > means job done and we know we finish all the possible descs this time and
> > > > > > we don't intend to start a new napi poll.
> > > > > >
> > > > > > It is apparently against our expectations. Please also see how
> > > > > > ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> > > > > > to make sure the budget can be decreased to zero at most and the negative
> > > > > > overflow never happens.
> > > > > >
> > > > > > The patch adds 'likely' because we rarely would not hit the loop codition
> > > > > > since the standard budget is 256.
> > > > > >
> > > > > > Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> > > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > > > ---
> > > > > > Link: https://lore.kernel.org/all/20250720091123.474-3-kerneljasonxing@gmail.com/
> > > > > > 1. use 'negative overflow' instead of 'underflow' (Willem)
> > > > > > 2. add reviewed-by tag (Larysa)
> > > > > > 3. target iwl-net branch (Larysa)
> > > > > > 4. add the reason why the patch adds likely() (Larysa)
> > > > > > ---
> > > > > >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
> > > > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > > > index ac58964b2f08..7b941505a9d0 100644
> > > > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > > > > @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > > > > >       dma_addr_t dma;
> > > > > >       u32 cmd_type;
> > > > > >
> > > > > > -     while (budget-- > 0) {
> > > > > > +     while (likely(budget)) {
> > > > > >               if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
> > > > > >                       work_done = false;
> > > > > >                       break;
> > > > > > @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
> > > > > >               xdp_ring->next_to_use++;
> > > > > >               if (xdp_ring->next_to_use == xdp_ring->count)
> > > > > >                       xdp_ring->next_to_use = 0;
> > > > > > +
> > > > > > +             budget--;
> > > > > >       }
> > > > > >
> > > > > >       if (tx_desc) {
> > > > >
> > > > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > > > >
> > > > > Is this just the smallest fix, and the rewrite to the more idiomatic for
> > > > > loop going to be done in a follow-up?
> > > >
> > > > Thanks for the review. But I'm not that sure if it's worth a follow-up
> > > > patch. Or if anyone else also expects to see a 'for loop' version, I
> > > > can send a V3 patch then. I have no strong opinion either way.
> > >
> > > I think we have iterated over this enough (pun intended).
> >
> > Hhh, interesting.
> >
> > > If this patch is correct then lets stick with this approach.
> >
> > No problem. Tomorrow I will send the 'for loop' version :)
>
> I meant, I think the while loop is fine.
> But anything that is correct is fine by me :)

Okay, great, then no more updates will be made unless other reviewers
have opinions :)

Thanks,
Jason

RE: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Jason Xing
> Sent: Saturday, July 26, 2025 9:04 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; bjorn@kernel.org; Fijalkowski, Maciej
> <maciej.fijalkowski@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Jason
> Xing <kernelxing@tencent.com>
> Subject: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the
> negative overflow of budget in ixgbe_xmit_zc
> 
> From: Jason Xing <kernelxing@tencent.com>
> 
> Resolve the budget negative overflow which leads to returning true in
> ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> 
> Before this patch, when the budget is decreased to zero and finishes
> sending the last allowed desc in ixgbe_xmit_zc, it will always turn
> back and enter into the while() statement to see if it should keep
> processing packets, but in the meantime it unexpectedly decreases the
> value again to 'unsigned int (0--)', namely, UINT_MAX. Finally, the
> ixgbe_xmit_zc returns true, showing 'we complete cleaning the budget'.
> That also means 'clean_complete = true' in ixgbe_poll.
> 
> The true theory behind this is if that budget number of descs are
> consumed, it implies that we might have more descs to be done. So we
> should return false in ixgbe_xmit_zc to tell napi poll to find another
> chance to start polling to handle the rest of descs. On the contrary,
> returning true here means job done and we know we finish all the
> possible descs this time and we don't intend to start a new napi poll.
> 
> It is apparently against our expectations. Please also see how
> ixgbe_clean_tx_irq() handles the problem: it uses do..while()
> statement to make sure the budget can be decreased to zero at most and
> the negative overflow never happens.
> 
> The patch adds 'likely' because we rarely would not hit the loop
> codition since the standard budget is 256.
> 
> Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

> ---
> Link: https://lore.kernel.org/all/20250720091123.474-3-
> kerneljasonxing@gmail.com/
> 1. use 'negative overflow' instead of 'underflow' (Willem) 2. add
> reviewed-by tag (Larysa) 3. target iwl-net branch (Larysa) 4. add the
> reason why the patch adds likely() (Larysa)
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> index ac58964b2f08..7b941505a9d0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring
> *xdp_ring, unsigned int budget)
>  	dma_addr_t dma;
>  	u32 cmd_type;
> 
> -	while (budget-- > 0) {
> +	while (likely(budget)) {
>  		if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
>  			work_done = false;
>  			break;
> @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring
> *xdp_ring, unsigned int budget)
>  		xdp_ring->next_to_use++;
>  		if (xdp_ring->next_to_use == xdp_ring->count)
>  			xdp_ring->next_to_use = 0;
> +
> +		budget--;
>  	}
> 
>  	if (tx_desc) {
> --
> 2.41.3

Re: [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Jason Xing]

On Sat, Jul 26, 2025 at 3:04 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Resolve the budget negative overflow which leads to returning true in
> ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
>
> Before this patch, when the budget is decreased to zero and finishes
> sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> and enter into the while() statement to see if it should keep processing
> packets, but in the meantime it unexpectedly decreases the value again to
> 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> true, showing 'we complete cleaning the budget'. That also means
> 'clean_complete = true' in ixgbe_poll.
>
> The true theory behind this is if that budget number of descs are consumed,
> it implies that we might have more descs to be done. So we should return
> false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> polling to handle the rest of descs. On the contrary, returning true here
> means job done and we know we finish all the possible descs this time and
> we don't intend to start a new napi poll.
>
> It is apparently against our expectations. Please also see how
> ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> to make sure the budget can be decreased to zero at most and the negative
> overflow never happens.
>
> The patch adds 'likely' because we rarely would not hit the loop codition
> since the standard budget is 256.
>
> Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

Hi Tony,

Any update here? Thanks! I'm asking because I'm ready to send an afxdp
patch series based on the patch :)

Thanks,
jason

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Tony Nguyen]

On 8/10/2025 5:22 AM, Jason Xing wrote:
> On Sat, Jul 26, 2025 at 3:04 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>>
>> From: Jason Xing <kernelxing@tencent.com>
>>
>> Resolve the budget negative overflow which leads to returning true in
>> ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
>>
>> Before this patch, when the budget is decreased to zero and finishes
>> sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
>> and enter into the while() statement to see if it should keep processing
>> packets, but in the meantime it unexpectedly decreases the value again to
>> 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
>> true, showing 'we complete cleaning the budget'. That also means
>> 'clean_complete = true' in ixgbe_poll.
>>
>> The true theory behind this is if that budget number of descs are consumed,
>> it implies that we might have more descs to be done. So we should return
>> false in ixgbe_xmit_zc to tell napi poll to find another chance to start
>> polling to handle the rest of descs. On the contrary, returning true here
>> means job done and we know we finish all the possible descs this time and
>> we don't intend to start a new napi poll.
>>
>> It is apparently against our expectations. Please also see how
>> ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
>> to make sure the budget can be decreased to zero at most and the negative
>> overflow never happens.
>>
>> The patch adds 'likely' because we rarely would not hit the loop codition
>> since the standard budget is 256.
>>
>> Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
>> Signed-off-by: Jason Xing <kernelxing@tencent.com>
>> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> 
> Hi Tony,
> 
> Any update here? Thanks! I'm asking because I'm ready to send an afxdp
> patch series based on the patch :)

Hi Jason,

I have this slated to be part of my next net series. I do already have 
this patch applied/staged on the Intel tree [1] (dev-queue branch). You 
can base it on that and send the changes now; I'll ensure that this 
patch is merged before sending the other series.

Thanks,
Tony

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git/

> Thanks,
> jason

Re: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Jason Xing]

On Tue, Aug 12, 2025 at 5:38 AM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
>
>
> On 8/10/2025 5:22 AM, Jason Xing wrote:
> > On Sat, Jul 26, 2025 at 3:04 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >>
> >> From: Jason Xing <kernelxing@tencent.com>
> >>
> >> Resolve the budget negative overflow which leads to returning true in
> >> ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> >>
> >> Before this patch, when the budget is decreased to zero and finishes
> >> sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
> >> and enter into the while() statement to see if it should keep processing
> >> packets, but in the meantime it unexpectedly decreases the value again to
> >> 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
> >> true, showing 'we complete cleaning the budget'. That also means
> >> 'clean_complete = true' in ixgbe_poll.
> >>
> >> The true theory behind this is if that budget number of descs are consumed,
> >> it implies that we might have more descs to be done. So we should return
> >> false in ixgbe_xmit_zc to tell napi poll to find another chance to start
> >> polling to handle the rest of descs. On the contrary, returning true here
> >> means job done and we know we finish all the possible descs this time and
> >> we don't intend to start a new napi poll.
> >>
> >> It is apparently against our expectations. Please also see how
> >> ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
> >> to make sure the budget can be decreased to zero at most and the negative
> >> overflow never happens.
> >>
> >> The patch adds 'likely' because we rarely would not hit the loop codition
> >> since the standard budget is 256.
> >>
> >> Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> >> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> >> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> >
> > Hi Tony,
> >
> > Any update here? Thanks! I'm asking because I'm ready to send an afxdp
> > patch series based on the patch :)
>
> Hi Jason,
>
> I have this slated to be part of my next net series. I do already have
> this patch applied/staged on the Intel tree [1] (dev-queue branch). You
> can base it on that and send the changes now; I'll ensure that this
> patch is merged before sending the other series.
>
> Thanks,
> Tony
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git/

Great, thanks for the pointer!

Thanks,
Jason

RE: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

[Singh, PriyaX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Jason Xing
> Sent: Saturday, July 26, 2025 12:34 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; bjorn@kernel.org; Fijalkowski, Maciej
> <maciej.fijalkowski@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Jason Xing
> <kernelxing@tencent.com>
> Subject: [Intel-wired-lan] [PATCH v2 iwl-net] ixgbe: xsk: resolve the negative
> overflow of budget in ixgbe_xmit_zc
> 
> From: Jason Xing <kernelxing@tencent.com>
> 
> Resolve the budget negative overflow which leads to returning true in
> ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.
> 
> Before this patch, when the budget is decreased to zero and finishes
> sending the last allowed desc in ixgbe_xmit_zc, it will always turn back and
> enter into the while() statement to see if it should keep processing packets,
> but in the meantime it unexpectedly decreases the value again to 'unsigned
> int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns true,
> showing 'we complete cleaning the budget'. That also means
> 'clean_complete = true' in ixgbe_poll.
> 
> The true theory behind this is if that budget number of descs are
> consumed, it implies that we might have more descs to be done. So we
> should return false in ixgbe_xmit_zc to tell napi poll to find another chance
> to start polling to handle the rest of descs. On the contrary, returning true
> here means job done and we know we finish all the possible descs this time
> and we don't intend to start a new napi poll.
> 
> It is apparently against our expectations. Please also see how
> ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement to
> make sure the budget can be decreased to zero at most and the negative
> overflow never happens.
> 
> The patch adds 'likely' because we rarely would not hit the loop codition
> since the standard budget is 256.
> 
> Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
> Link: https://lore.kernel.org/all/20250720091123.474-3-
> kerneljasonxing@gmail.com/
> 1. use 'negative overflow' instead of 'underflow' (Willem) 2. add reviewed-by
> tag (Larysa) 3. target iwl-net branch (Larysa) 4. add the reason why the patch
> adds likely() (Larysa)
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> index ac58964b2f08..7b941505a9d0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring
> *xdp_ring, unsigned int budget)
>  	dma_addr_t dma;
>  	u32 cmd_type;
> 
> -	while (budget-- > 0) {
> +	while (likely(budget)) {
>  		if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
>  			work_done = false;
>  			break;
> @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring
> *xdp_ring, unsigned int budget)
>  		xdp_ring->next_to_use++;
>  		if (xdp_ring->next_to_use == xdp_ring->count)
>  			xdp_ring->next_to_use = 0;
> +
> +		budget--;
>  	}
> 
>  	if (tx_desc) {
> --
> 2.41.3

Tested-by: Priya Singh <priyax.singh@intel.com>