| Message ID | 20231018163908.40841-1-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 913eda2b08cc49d31f382579e2be34c2709eb789 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] i40e: xsk: remove count_mask | expand |
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; > } > } >
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; > > } > > } > >
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>
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!
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; } }
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(-)