[intel-net] i40e: fix receiving of single packets in xsk zero-copy mode

Commit Message

Magnus Karlsson March 19, 2021, 9:44 a.m. UTC

From: Magnus Karlsson <[e-mail]>

Fix so that single packets are received immediately instead of in
batches of 8. If you sent 1 pss to a system, you received 8 packets
every 8 seconds instead of 1 packet every second. The problem behind
this was that the work_done reporting from the Tx part of the driver
was broken. The work_done reporting in i40e controls not only the
reporting back to the napi logic but also the setting of the interrupt
throttling logic. When Tx or Rx reports that it has more to do,
interrupts are throttled or coalesced and when they both report that
they are done, interrupts are armed right away. If the wrong work_done
value is returned, the logic will start to throttle interrupts in a
situation where it should have just enabled them. This leads to the
undesired batching behavior seen in user-space.

Fix this by returning the correct boolean value from the Tx xsk
zero-copy path. Return true if there is nothing to do or if we got
fewer packets to process than we asked for. Return false if we got as
many packets as the budget since there might be more packets we can
process.

Fixes: 3106c580fb7c ("i40e: Use batched xsk Tx interfaces to increase performance")
Reported-by: Sreedevi Joshi <[e-mail]>
Signed-off-by: Magnus Karlsson <[e-mail]>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: c79a707072fe3fea0e3c92edee6ca85c1e53c29f

Comments

Fijalkowski, Maciej March 19, 2021, 11 a.m. UTC | #1

On Fri, Mar 19, 2021 at 10:44:10AM +0100, Magnus Karlsson wrote:
> From: Magnus Karlsson <[e-mail]>
> 
> Fix so that single packets are received immediately instead of in
> batches of 8. If you sent 1 pss to a system, you received 8 packets

pps?

> every 8 seconds instead of 1 packet every second. The problem behind
> this was that the work_done reporting from the Tx part of the driver
> was broken. The work_done reporting in i40e controls not only the
> reporting back to the napi logic but also the setting of the interrupt
> throttling logic. When Tx or Rx reports that it has more to do,
> interrupts are throttled or coalesced and when they both report that
> they are done, interrupts are armed right away. If the wrong work_done
> value is returned, the logic will start to throttle interrupts in a
> situation where it should have just enabled them. This leads to the
> undesired batching behavior seen in user-space.
> 
> Fix this by returning the correct boolean value from the Tx xsk
> zero-copy path. Return true if there is nothing to do or if we got
> fewer packets to process than we asked for. Return false if we got as
> many packets as the budget since there might be more packets we can
> process.
> 
> Fixes: 3106c580fb7c ("i40e: Use batched xsk Tx interfaces to increase performance")
> Reported-by: Sreedevi Joshi <[e-mail]>
> Signed-off-by: Magnus Karlsson <[e-mail]>

Acked-by: Maciej Fijalkowski <[e-mail]>

> ---
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index fc32c5019b0f..12ca84113587 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -471,7 +471,7 @@ static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
>  
>  	nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, descs, budget);
>  	if (!nb_pkts)
> -		return false;
> +		return true;
>  
>  	if (xdp_ring->next_to_use + nb_pkts >= xdp_ring->count) {
>  		nb_processed = xdp_ring->count - xdp_ring->next_to_use;
> @@ -488,7 +488,7 @@ static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
>  
>  	i40e_update_tx_stats(xdp_ring, nb_pkts, total_bytes);
>  
> -	return true;
> +	return nb_pkts < budget;
>  }
>  
>  /**
> 
> base-commit: c79a707072fe3fea0e3c92edee6ca85c1e53c29f
> -- 
> 2.29.0
>

Magnus Karlsson March 19, 2021, 12:14 p.m. UTC | #2

On Fri, Mar 19, 2021 at 12:10 PM Maciej Fijalkowski
<[e-mail]> wrote:
>
> On Fri, Mar 19, 2021 at 10:44:10AM +0100, Magnus Karlsson wrote:
> > From: Magnus Karlsson <[e-mail]>
> >
> > Fix so that single packets are received immediately instead of in
> > batches of 8. If you sent 1 pss to a system, you received 8 packets
>
> pps?

Arghh, yes it should be pps, i.e. packets per second. I wonder what pss is?

> > every 8 seconds instead of 1 packet every second. The problem behind
> > this was that the work_done reporting from the Tx part of the driver
> > was broken. The work_done reporting in i40e controls not only the
> > reporting back to the napi logic but also the setting of the interrupt
> > throttling logic. When Tx or Rx reports that it has more to do,
> > interrupts are throttled or coalesced and when they both report that
> > they are done, interrupts are armed right away. If the wrong work_done
> > value is returned, the logic will start to throttle interrupts in a
> > situation where it should have just enabled them. This leads to the
> > undesired batching behavior seen in user-space.
> >
> > Fix this by returning the correct boolean value from the Tx xsk
> > zero-copy path. Return true if there is nothing to do or if we got
> > fewer packets to process than we asked for. Return false if we got as
> > many packets as the budget since there might be more packets we can
> > process.
> >
> > Fixes: 3106c580fb7c ("i40e: Use batched xsk Tx interfaces to increase performance")
> > Reported-by: Sreedevi Joshi <[e-mail]>
> > Signed-off-by: Magnus Karlsson <[e-mail]>
>
> Acked-by: Maciej Fijalkowski <[e-mail]>
>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > index fc32c5019b0f..12ca84113587 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > @@ -471,7 +471,7 @@ static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
> >
> >       nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, descs, budget);
> >       if (!nb_pkts)
> > -             return false;
> > +             return true;
> >
> >       if (xdp_ring->next_to_use + nb_pkts >= xdp_ring->count) {
> >               nb_processed = xdp_ring->count - xdp_ring->next_to_use;
> > @@ -488,7 +488,7 @@ static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
> >
> >       i40e_update_tx_stats(xdp_ring, nb_pkts, total_bytes);
> >
> > -     return true;
> > +     return nb_pkts < budget;
> >  }
> >
> >  /**
> >
> > base-commit: c79a707072fe3fea0e3c92edee6ca85c1e53c29f
> > --
> > 2.29.0
> >

Bhandare, KiranX March 31, 2021, 12:57 p.m. UTC | #3

> -----Original Message-----
> From: Intel-wired-lan <[e-mail]> On Behalf Of
> Magnus Karlsson
> Sent: Friday, March 19, 2021 3:14 PM
> To: Karlsson, Magnus <[e-mail]>; intel-wired-
> [e-mail]; Nguyen, Anthony L <[e-mail]>;
> Fijalkowski, Maciej <[e-mail]>
> Cc: [e-mail]; Joshi, Sreedevi <[e-mail]>
> Subject: [Intel-wired-lan] [PATCH intel-net] i40e: fix receiving of single packets
> in xsk zero-copy mode
> 
> From: Magnus Karlsson <[e-mail]>
> 
> Fix so that single packets are received immediately instead of in batches of 8.
> If you sent 1 pss to a system, you received 8 packets every 8 seconds instead
> of 1 packet every second. The problem behind this was that the work_done
> reporting from the Tx part of the driver was broken. The work_done
> reporting in i40e controls not only the reporting back to the napi logic but
> also the setting of the interrupt throttling logic. When Tx or Rx reports that it
> has more to do, interrupts are throttled or coalesced and when they both
> report that they are done, interrupts are armed right away. If the wrong
> work_done value is returned, the logic will start to throttle interrupts in a
> situation where it should have just enabled them. This leads to the undesired
> batching behavior seen in user-space.
> 
> Fix this by returning the correct boolean value from the Tx xsk zero-copy
> path. Return true if there is nothing to do or if we got fewer packets to
> process than we asked for. Return false if we got as many packets as the
> budget since there might be more packets we can process.
> 
> Fixes: 3106c580fb7c ("i40e: Use batched xsk Tx interfaces to increase
> performance")
> Reported-by: Sreedevi Joshi <[e-mail]>
> Signed-off-by: Magnus Karlsson <[e-mail]>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Tested-by: Kiran Bhandare <[e-mail]>  A Contingent Worker at Intel

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index fc32c5019b0f..12ca84113587 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -471,7 +471,7 @@  static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
 
 	nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, descs, budget);
 	if (!nb_pkts)
-		return false;
+		return true;
 
 	if (xdp_ring->next_to_use + nb_pkts >= xdp_ring->count) {
 		nb_processed = xdp_ring->count - xdp_ring->next_to_use;
@@ -488,7 +488,7 @@  static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
 
 	i40e_update_tx_stats(xdp_ring, nb_pkts, total_bytes);
 
-	return true;
+	return nb_pkts < budget;
 }
 
 /**