From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: netdev@vger.kernel.org, anthony.l.nguyen@intel.com,
intel-wired-lan@lists.osuosl.org,
Tushar Vyavahare <tushar.vyavahare@intel.com>,
magnus.karlsson@intel.com
Subject: Re: [Intel-wired-lan] [PATCH net] i40e: xsk: remove count_mask
Date: Thu, 19 Oct 2023 14:10:02 +0200 [thread overview]
Message-ID: <ZTEcmh2GNi0Ugn3/@boxer> (raw)
In-Reply-To: <17b4725f-b81f-4760-bb90-ac8fa9467227@intel.com>
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;
> > }
> > }
> >
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
WARNING: multiple messages have this Message-ID (diff)
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: <intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
<anthony.l.nguyen@intel.com>, <magnus.karlsson@intel.com>,
Tushar Vyavahare <tushar.vyavahare@intel.com>
Subject: Re: [PATCH net] i40e: xsk: remove count_mask
Date: Thu, 19 Oct 2023 14:10:02 +0200 [thread overview]
Message-ID: <ZTEcmh2GNi0Ugn3/@boxer> (raw)
In-Reply-To: <17b4725f-b81f-4760-bb90-ac8fa9467227@intel.com>
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;
> > }
> > }
> >
next prev parent reply other threads:[~2023-10-19 12:10 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 16:39 [Intel-wired-lan] [PATCH net] i40e: xsk: remove count_mask Maciej Fijalkowski
2023-10-18 16:39 ` Maciej Fijalkowski
2023-10-18 22:03 ` [Intel-wired-lan] " Jacob Keller
2023-10-18 22:03 ` Jacob Keller
2023-10-19 12:10 ` Maciej Fijalkowski [this message]
2023-10-19 12:10 ` Maciej Fijalkowski
2023-10-19 16:42 ` [Intel-wired-lan] " Jacob Keller
2023-10-19 16:42 ` Jacob Keller
2023-10-20 0:40 ` [Intel-wired-lan] " patchwork-bot+netdevbpf
2023-10-20 0:40 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZTEcmh2GNi0Ugn3/@boxer \
--to=maciej.fijalkowski@intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jacob.e.keller@intel.com \
--cc=magnus.karlsson@intel.com \
--cc=netdev@vger.kernel.org \
--cc=tushar.vyavahare@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.