[PATCH net] i40e: xsk: remove count_mask

[PATCH net] i40e: xsk: remove count_mask

[Maciej Fijalkowski]

Cited commit introduced a neat way of updating next_to_clean that does
not require boundary checks on each increment. This was done by masking
the new value with (ring length - 1) mask. Problem is that this is
applicable only for power of 2 ring sizes, for every other size this
assumption can not be made. In turn, it leads to cleaning descriptors
out of order as well as splats:

[ 1388.411915] Workqueue: events xp_release_deferred
[ 1388.411919] RIP: 0010:xp_free+0x1a/0x50
[ 1388.411921] Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 8b 57 70 48 8d 47 70 48 89 e5 48 39 d0 74 06 <5d> c3 cc cc cc cc 48 8b 57 60 83 82 b8 00 00 00 01 48 8b 57 60 48
[ 1388.411922] RSP: 0018:ffa0000000a83cb0 EFLAGS: 00000206
[ 1388.411923] RAX: ff11000119aa5030 RBX: 000000000000001d RCX: ff110001129b6e50
[ 1388.411924] RDX: ff11000119aa4fa0 RSI: 0000000055555554 RDI: ff11000119aa4fc0
[ 1388.411925] RBP: ffa0000000a83cb0 R08: 0000000000000000 R09: 0000000000000000
[ 1388.411926] R10: 0000000000000001 R11: 0000000000000000 R12: ff11000115829b80
[ 1388.411927] R13: 000000000000005f R14: 0000000000000000 R15: ff11000119aa4fc0
[ 1388.411928] FS:  0000000000000000(0000) GS:ff11000277e00000(0000) knlGS:0000000000000000
[ 1388.411929] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1388.411930] CR2: 00007f1f564e6c14 CR3: 000000000783c005 CR4: 0000000000771ef0
[ 1388.411931] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1388.411931] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1388.411932] PKRU: 55555554
[ 1388.411933] Call Trace:
[ 1388.411934]  <IRQ>
[ 1388.411935]  ? show_regs+0x6e/0x80
[ 1388.411937]  ? watchdog_timer_fn+0x1d2/0x240
[ 1388.411939]  ? __pfx_watchdog_timer_fn+0x10/0x10
[ 1388.411941]  ? __hrtimer_run_queues+0x10e/0x290
[ 1388.411945]  ? clockevents_program_event+0xae/0x130
[ 1388.411947]  ? hrtimer_interrupt+0x105/0x240
[ 1388.411949]  ? __sysvec_apic_timer_interrupt+0x54/0x150
[ 1388.411952]  ? sysvec_apic_timer_interrupt+0x7f/0x90
[ 1388.411955]  </IRQ>
[ 1388.411955]  <TASK>
[ 1388.411956]  ? asm_sysvec_apic_timer_interrupt+0x1f/0x30
[ 1388.411958]  ? xp_free+0x1a/0x50
[ 1388.411960]  i40e_xsk_clean_rx_ring+0x5d/0x100 [i40e]
[ 1388.411968]  i40e_clean_rx_ring+0x14c/0x170 [i40e]
[ 1388.411977]  i40e_queue_pair_disable+0xda/0x260 [i40e]
[ 1388.411986]  i40e_xsk_pool_setup+0x192/0x1d0 [i40e]
[ 1388.411993]  i40e_reconfig_rss_queues+0x1f0/0x1450 [i40e]
[ 1388.412002]  xp_disable_drv_zc+0x73/0xf0
[ 1388.412004]  ? mutex_lock+0x17/0x50
[ 1388.412007]  xp_release_deferred+0x2b/0xc0
[ 1388.412010]  process_one_work+0x178/0x350
[ 1388.412011]  ? __pfx_worker_thread+0x10/0x10
[ 1388.412012]  worker_thread+0x2f7/0x420
[ 1388.412014]  ? __pfx_worker_thread+0x10/0x10
[ 1388.412015]  kthread+0xf8/0x130
[ 1388.412017]  ? __pfx_kthread+0x10/0x10
[ 1388.412019]  ret_from_fork+0x3d/0x60
[ 1388.412021]  ? __pfx_kthread+0x10/0x10
[ 1388.412023]  ret_from_fork_asm+0x1b/0x30
[ 1388.412026]  </TASK>

It comes from picking wrong ring entries when cleaning xsk buffers
during pool detach.

Remove the count_mask logic and use they boundary check when updating
next_to_process (which used to be a next_to_clean).

Fixes: c8a8ca3408dc ("i40e: remove unnecessary memory writes of the next to clean pointer")
Reported-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
Tested-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 37f41c8a682f..7d991e4d9b89 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -437,12 +437,12 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
 	u16 next_to_process = rx_ring->next_to_process;
 	u16 next_to_clean = rx_ring->next_to_clean;
-	u16 count_mask = rx_ring->count - 1;
 	unsigned int xdp_res, xdp_xmit = 0;
 	struct xdp_buff *first = NULL;
+	u32 count = rx_ring->count;
 	struct bpf_prog *xdp_prog;
+	u32 entries_to_alloc;
 	bool failure = false;
-	u16 cleaned_count;
 
 	if (next_to_process != next_to_clean)
 		first = *i40e_rx_bi(rx_ring, next_to_clean);
@@ -475,7 +475,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 						      qword);
 			bi = *i40e_rx_bi(rx_ring, next_to_process);
 			xsk_buff_free(bi);
-			next_to_process = (next_to_process + 1) & count_mask;
+			if (++next_to_process == count)
+				next_to_process = 0;
 			continue;
 		}
 
@@ -493,7 +494,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		else if (i40e_add_xsk_frag(rx_ring, first, bi, size))
 			break;
 
-		next_to_process = (next_to_process + 1) & count_mask;
+		if (++next_to_process == count)
+			next_to_process = 0;
 
 		if (i40e_is_non_eop(rx_ring, rx_desc))
 			continue;
@@ -513,10 +515,10 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 	rx_ring->next_to_clean = next_to_clean;
 	rx_ring->next_to_process = next_to_process;
-	cleaned_count = (next_to_clean - rx_ring->next_to_use - 1) & count_mask;
 
-	if (cleaned_count >= I40E_RX_BUFFER_WRITE)
-		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
+	entries_to_alloc = I40E_DESC_UNUSED(rx_ring);
+	if (entries_to_alloc >= I40E_RX_BUFFER_WRITE)
+		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, entries_to_alloc);
 
 	i40e_finalize_xdp_rx(rx_ring, xdp_xmit);
 	i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
@@ -752,14 +754,16 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
 
 void i40e_xsk_clean_rx_ring(struct i40e_ring *rx_ring)
 {
-	u16 count_mask = rx_ring->count - 1;
 	u16 ntc = rx_ring->next_to_clean;
 	u16 ntu = rx_ring->next_to_use;
 
-	for ( ; ntc != ntu; ntc = (ntc + 1)  & count_mask) {
+	while (ntc != ntu) {
 		struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);
 
 		xsk_buff_free(rx_bi);
+		ntc++;
+		if (ntc >= rx_ring->count)
+			ntc = 0;
 	}
 }
 
-- 
2.34.1

Re: [PATCH net] i40e: xsk: remove count_mask

[Jacob Keller]

On 10/18/2023 9:39 AM, Maciej Fijalkowski wrote:
> Cited commit introduced a neat way of updating next_to_clean that does
> not require boundary checks on each increment. This was done by masking
> the new value with (ring length - 1) mask. Problem is that this is
> applicable only for power of 2 ring sizes, for every other size this
> assumption can not be made. In turn, it leads to cleaning descriptors
> out of order as well as splats:
> 

I assume that since ring size isn't a constant it isn't worth trying to
check if power of 2 and then use the shortcut?

What about just enforcing ring size is a power of 2? Any reason not to
do that?

Thanks,
Jake

> [ 1388.411915] Workqueue: events xp_release_deferred
> [ 1388.411919] RIP: 0010:xp_free+0x1a/0x50
> [ 1388.411921] Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 8b 57 70 48 8d 47 70 48 89 e5 48 39 d0 74 06 <5d> c3 cc cc cc cc 48 8b 57 60 83 82 b8 00 00 00 01 48 8b 57 60 48
> [ 1388.411922] RSP: 0018:ffa0000000a83cb0 EFLAGS: 00000206
> [ 1388.411923] RAX: ff11000119aa5030 RBX: 000000000000001d RCX: ff110001129b6e50
> [ 1388.411924] RDX: ff11000119aa4fa0 RSI: 0000000055555554 RDI: ff11000119aa4fc0
> [ 1388.411925] RBP: ffa0000000a83cb0 R08: 0000000000000000 R09: 0000000000000000
> [ 1388.411926] R10: 0000000000000001 R11: 0000000000000000 R12: ff11000115829b80
> [ 1388.411927] R13: 000000000000005f R14: 0000000000000000 R15: ff11000119aa4fc0
> [ 1388.411928] FS:  0000000000000000(0000) GS:ff11000277e00000(0000) knlGS:0000000000000000
> [ 1388.411929] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1388.411930] CR2: 00007f1f564e6c14 CR3: 000000000783c005 CR4: 0000000000771ef0
> [ 1388.411931] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1388.411931] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 1388.411932] PKRU: 55555554
> [ 1388.411933] Call Trace:
> [ 1388.411934]  <IRQ>
> [ 1388.411935]  ? show_regs+0x6e/0x80
> [ 1388.411937]  ? watchdog_timer_fn+0x1d2/0x240
> [ 1388.411939]  ? __pfx_watchdog_timer_fn+0x10/0x10
> [ 1388.411941]  ? __hrtimer_run_queues+0x10e/0x290
> [ 1388.411945]  ? clockevents_program_event+0xae/0x130
> [ 1388.411947]  ? hrtimer_interrupt+0x105/0x240
> [ 1388.411949]  ? __sysvec_apic_timer_interrupt+0x54/0x150
> [ 1388.411952]  ? sysvec_apic_timer_interrupt+0x7f/0x90
> [ 1388.411955]  </IRQ>
> [ 1388.411955]  <TASK>
> [ 1388.411956]  ? asm_sysvec_apic_timer_interrupt+0x1f/0x30
> [ 1388.411958]  ? xp_free+0x1a/0x50
> [ 1388.411960]  i40e_xsk_clean_rx_ring+0x5d/0x100 [i40e]
> [ 1388.411968]  i40e_clean_rx_ring+0x14c/0x170 [i40e]
> [ 1388.411977]  i40e_queue_pair_disable+0xda/0x260 [i40e]
> [ 1388.411986]  i40e_xsk_pool_setup+0x192/0x1d0 [i40e]
> [ 1388.411993]  i40e_reconfig_rss_queues+0x1f0/0x1450 [i40e]
> [ 1388.412002]  xp_disable_drv_zc+0x73/0xf0
> [ 1388.412004]  ? mutex_lock+0x17/0x50
> [ 1388.412007]  xp_release_deferred+0x2b/0xc0
> [ 1388.412010]  process_one_work+0x178/0x350
> [ 1388.412011]  ? __pfx_worker_thread+0x10/0x10
> [ 1388.412012]  worker_thread+0x2f7/0x420
> [ 1388.412014]  ? __pfx_worker_thread+0x10/0x10
> [ 1388.412015]  kthread+0xf8/0x130
> [ 1388.412017]  ? __pfx_kthread+0x10/0x10
> [ 1388.412019]  ret_from_fork+0x3d/0x60
> [ 1388.412021]  ? __pfx_kthread+0x10/0x10
> [ 1388.412023]  ret_from_fork_asm+0x1b/0x30
> [ 1388.412026]  </TASK>
> 
> It comes from picking wrong ring entries when cleaning xsk buffers
> during pool detach.
> 
> Remove the count_mask logic and use they boundary check when updating
> next_to_process (which used to be a next_to_clean).
> 
> Fixes: c8a8ca3408dc ("i40e: remove unnecessary memory writes of the next to clean pointer")
> Reported-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
> Tested-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 37f41c8a682f..7d991e4d9b89 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -437,12 +437,12 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
>  	u16 next_to_process = rx_ring->next_to_process;
>  	u16 next_to_clean = rx_ring->next_to_clean;
> -	u16 count_mask = rx_ring->count - 1;
>  	unsigned int xdp_res, xdp_xmit = 0;
>  	struct xdp_buff *first = NULL;
> +	u32 count = rx_ring->count;
>  	struct bpf_prog *xdp_prog;
> +	u32 entries_to_alloc;
>  	bool failure = false;
> -	u16 cleaned_count;
>  
>  	if (next_to_process != next_to_clean)
>  		first = *i40e_rx_bi(rx_ring, next_to_clean);
> @@ -475,7 +475,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  						      qword);
>  			bi = *i40e_rx_bi(rx_ring, next_to_process);
>  			xsk_buff_free(bi);
> -			next_to_process = (next_to_process + 1) & count_mask;
> +			if (++next_to_process == count)
> +				next_to_process = 0;
>  			continue;
>  		}
>  
> @@ -493,7 +494,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  		else if (i40e_add_xsk_frag(rx_ring, first, bi, size))
>  			break;
>  
> -		next_to_process = (next_to_process + 1) & count_mask;
> +		if (++next_to_process == count)
> +			next_to_process = 0;
>  
>  		if (i40e_is_non_eop(rx_ring, rx_desc))
>  			continue;
> @@ -513,10 +515,10 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  
>  	rx_ring->next_to_clean = next_to_clean;
>  	rx_ring->next_to_process = next_to_process;
> -	cleaned_count = (next_to_clean - rx_ring->next_to_use - 1) & count_mask;
>  
> -	if (cleaned_count >= I40E_RX_BUFFER_WRITE)
> -		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
> +	entries_to_alloc = I40E_DESC_UNUSED(rx_ring);
> +	if (entries_to_alloc >= I40E_RX_BUFFER_WRITE)
> +		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, entries_to_alloc);
>  
>  	i40e_finalize_xdp_rx(rx_ring, xdp_xmit);
>  	i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
> @@ -752,14 +754,16 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
>  
>  void i40e_xsk_clean_rx_ring(struct i40e_ring *rx_ring)
>  {
> -	u16 count_mask = rx_ring->count - 1;
>  	u16 ntc = rx_ring->next_to_clean;
>  	u16 ntu = rx_ring->next_to_use;
>  
> -	for ( ; ntc != ntu; ntc = (ntc + 1)  & count_mask) {
> +	while (ntc != ntu) {
>  		struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);
>  
>  		xsk_buff_free(rx_bi);
> +		ntc++;
> +		if (ntc >= rx_ring->count)
> +			ntc = 0;
>  	}
>  }
>  

Re: [PATCH net] i40e: xsk: remove count_mask

[Maciej Fijalkowski]

On Wed, Oct 18, 2023 at 03:03:13PM -0700, Jacob Keller wrote:
> 
> 
> On 10/18/2023 9:39 AM, Maciej Fijalkowski wrote:
> > Cited commit introduced a neat way of updating next_to_clean that does
> > not require boundary checks on each increment. This was done by masking
> > the new value with (ring length - 1) mask. Problem is that this is
> > applicable only for power of 2 ring sizes, for every other size this
> > assumption can not be made. In turn, it leads to cleaning descriptors
> > out of order as well as splats:
> > 
> 
> I assume that since ring size isn't a constant it isn't worth trying to
> check if power of 2 and then use the shortcut?

That would kill what we were gaining from that micro optimization (lack of
branching). I was thinking about INDIRECT_CALL() for ntc update based on
ring size but I came down to conclusion it would be overengineered.

> 
> What about just enforcing ring size is a power of 2? Any reason not to
> do that?

We used to do that on ice but then customers yelled at us that they can't
use max ring size (8192). I know i40e's max is 4096 which is pow-2 but I
am not aware of all of the use cases that people have out there.

> 
> Thanks,
> Jake
> 
> > [ 1388.411915] Workqueue: events xp_release_deferred
> > [ 1388.411919] RIP: 0010:xp_free+0x1a/0x50
> > [ 1388.411921] Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 8b 57 70 48 8d 47 70 48 89 e5 48 39 d0 74 06 <5d> c3 cc cc cc cc 48 8b 57 60 83 82 b8 00 00 00 01 48 8b 57 60 48
> > [ 1388.411922] RSP: 0018:ffa0000000a83cb0 EFLAGS: 00000206
> > [ 1388.411923] RAX: ff11000119aa5030 RBX: 000000000000001d RCX: ff110001129b6e50
> > [ 1388.411924] RDX: ff11000119aa4fa0 RSI: 0000000055555554 RDI: ff11000119aa4fc0
> > [ 1388.411925] RBP: ffa0000000a83cb0 R08: 0000000000000000 R09: 0000000000000000
> > [ 1388.411926] R10: 0000000000000001 R11: 0000000000000000 R12: ff11000115829b80
> > [ 1388.411927] R13: 000000000000005f R14: 0000000000000000 R15: ff11000119aa4fc0
> > [ 1388.411928] FS:  0000000000000000(0000) GS:ff11000277e00000(0000) knlGS:0000000000000000
> > [ 1388.411929] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 1388.411930] CR2: 00007f1f564e6c14 CR3: 000000000783c005 CR4: 0000000000771ef0
> > [ 1388.411931] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [ 1388.411931] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [ 1388.411932] PKRU: 55555554
> > [ 1388.411933] Call Trace:
> > [ 1388.411934]  <IRQ>
> > [ 1388.411935]  ? show_regs+0x6e/0x80
> > [ 1388.411937]  ? watchdog_timer_fn+0x1d2/0x240
> > [ 1388.411939]  ? __pfx_watchdog_timer_fn+0x10/0x10
> > [ 1388.411941]  ? __hrtimer_run_queues+0x10e/0x290
> > [ 1388.411945]  ? clockevents_program_event+0xae/0x130
> > [ 1388.411947]  ? hrtimer_interrupt+0x105/0x240
> > [ 1388.411949]  ? __sysvec_apic_timer_interrupt+0x54/0x150
> > [ 1388.411952]  ? sysvec_apic_timer_interrupt+0x7f/0x90
> > [ 1388.411955]  </IRQ>
> > [ 1388.411955]  <TASK>
> > [ 1388.411956]  ? asm_sysvec_apic_timer_interrupt+0x1f/0x30
> > [ 1388.411958]  ? xp_free+0x1a/0x50
> > [ 1388.411960]  i40e_xsk_clean_rx_ring+0x5d/0x100 [i40e]
> > [ 1388.411968]  i40e_clean_rx_ring+0x14c/0x170 [i40e]
> > [ 1388.411977]  i40e_queue_pair_disable+0xda/0x260 [i40e]
> > [ 1388.411986]  i40e_xsk_pool_setup+0x192/0x1d0 [i40e]
> > [ 1388.411993]  i40e_reconfig_rss_queues+0x1f0/0x1450 [i40e]
> > [ 1388.412002]  xp_disable_drv_zc+0x73/0xf0
> > [ 1388.412004]  ? mutex_lock+0x17/0x50
> > [ 1388.412007]  xp_release_deferred+0x2b/0xc0
> > [ 1388.412010]  process_one_work+0x178/0x350
> > [ 1388.412011]  ? __pfx_worker_thread+0x10/0x10
> > [ 1388.412012]  worker_thread+0x2f7/0x420
> > [ 1388.412014]  ? __pfx_worker_thread+0x10/0x10
> > [ 1388.412015]  kthread+0xf8/0x130
> > [ 1388.412017]  ? __pfx_kthread+0x10/0x10
> > [ 1388.412019]  ret_from_fork+0x3d/0x60
> > [ 1388.412021]  ? __pfx_kthread+0x10/0x10
> > [ 1388.412023]  ret_from_fork_asm+0x1b/0x30
> > [ 1388.412026]  </TASK>
> > 
> > It comes from picking wrong ring entries when cleaning xsk buffers
> > during pool detach.
> > 
> > Remove the count_mask logic and use they boundary check when updating
> > next_to_process (which used to be a next_to_clean).
> > 
> > Fixes: c8a8ca3408dc ("i40e: remove unnecessary memory writes of the next to clean pointer")
> > Reported-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
> > Tested-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_xsk.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > index 37f41c8a682f..7d991e4d9b89 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > @@ -437,12 +437,12 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >  	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> >  	u16 next_to_process = rx_ring->next_to_process;
> >  	u16 next_to_clean = rx_ring->next_to_clean;
> > -	u16 count_mask = rx_ring->count - 1;
> >  	unsigned int xdp_res, xdp_xmit = 0;
> >  	struct xdp_buff *first = NULL;
> > +	u32 count = rx_ring->count;
> >  	struct bpf_prog *xdp_prog;
> > +	u32 entries_to_alloc;
> >  	bool failure = false;
> > -	u16 cleaned_count;
> >  
> >  	if (next_to_process != next_to_clean)
> >  		first = *i40e_rx_bi(rx_ring, next_to_clean);
> > @@ -475,7 +475,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >  						      qword);
> >  			bi = *i40e_rx_bi(rx_ring, next_to_process);
> >  			xsk_buff_free(bi);
> > -			next_to_process = (next_to_process + 1) & count_mask;
> > +			if (++next_to_process == count)
> > +				next_to_process = 0;
> >  			continue;
> >  		}
> >  
> > @@ -493,7 +494,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >  		else if (i40e_add_xsk_frag(rx_ring, first, bi, size))
> >  			break;
> >  
> > -		next_to_process = (next_to_process + 1) & count_mask;
> > +		if (++next_to_process == count)
> > +			next_to_process = 0;
> >  
> >  		if (i40e_is_non_eop(rx_ring, rx_desc))
> >  			continue;
> > @@ -513,10 +515,10 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >  
> >  	rx_ring->next_to_clean = next_to_clean;
> >  	rx_ring->next_to_process = next_to_process;
> > -	cleaned_count = (next_to_clean - rx_ring->next_to_use - 1) & count_mask;
> >  
> > -	if (cleaned_count >= I40E_RX_BUFFER_WRITE)
> > -		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
> > +	entries_to_alloc = I40E_DESC_UNUSED(rx_ring);
> > +	if (entries_to_alloc >= I40E_RX_BUFFER_WRITE)
> > +		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, entries_to_alloc);
> >  
> >  	i40e_finalize_xdp_rx(rx_ring, xdp_xmit);
> >  	i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
> > @@ -752,14 +754,16 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
> >  
> >  void i40e_xsk_clean_rx_ring(struct i40e_ring *rx_ring)
> >  {
> > -	u16 count_mask = rx_ring->count - 1;
> >  	u16 ntc = rx_ring->next_to_clean;
> >  	u16 ntu = rx_ring->next_to_use;
> >  
> > -	for ( ; ntc != ntu; ntc = (ntc + 1)  & count_mask) {
> > +	while (ntc != ntu) {
> >  		struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);
> >  
> >  		xsk_buff_free(rx_bi);
> > +		ntc++;
> > +		if (ntc >= rx_ring->count)
> > +			ntc = 0;
> >  	}
> >  }
> >  

Re: [PATCH net] i40e: xsk: remove count_mask

[Jacob Keller]

On 10/19/2023 5:10 AM, Maciej Fijalkowski wrote:
> On Wed, Oct 18, 2023 at 03:03:13PM -0700, Jacob Keller wrote:
>>
>>
>> On 10/18/2023 9:39 AM, Maciej Fijalkowski wrote:
>>> Cited commit introduced a neat way of updating next_to_clean that does
>>> not require boundary checks on each increment. This was done by masking
>>> the new value with (ring length - 1) mask. Problem is that this is
>>> applicable only for power of 2 ring sizes, for every other size this
>>> assumption can not be made. In turn, it leads to cleaning descriptors
>>> out of order as well as splats:
>>>
>>
>> I assume that since ring size isn't a constant it isn't worth trying to
>> check if power of 2 and then use the shortcut?
> 
> That would kill what we were gaining from that micro optimization (lack of
> branching). I was thinking about INDIRECT_CALL() for ntc update based on
> ring size but I came down to conclusion it would be overengineered.
> 

Right, thats what I imagined as well.

>>
>> What about just enforcing ring size is a power of 2? Any reason not to
>> do that?
> 
> We used to do that on ice but then customers yelled at us that they can't
> use max ring size (8192). I know i40e's max is 4096 which is pow-2 but I
> am not aware of all of the use cases that people have out there.
> 

And we've supported non-power-of-2 sizes for some time. Ok.

Sure, lets take this fix then. How recent was the bug? Looks like v5.12,
so its been a while but the default size is power of 2 so that could
explain why it hasn't been noticed much in the wild. Ok.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net] i40e: xsk: remove count_mask

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 18 Oct 2023 18:39:08 +0200 you wrote:
> Cited commit introduced a neat way of updating next_to_clean that does
> not require boundary checks on each increment. This was done by masking
> the new value with (ring length - 1) mask. Problem is that this is
> applicable only for power of 2 ring sizes, for every other size this
> assumption can not be made. In turn, it leads to cleaning descriptors
> out of order as well as splats:
> 
> [...]

Here is the summary with links:
  - [net] i40e: xsk: remove count_mask
    https://git.kernel.org/netdev/net/c/913eda2b08cc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html