From: Jacob Keller <jacob.e.keller@intel.com>
To: Maciej Fijalkowski <maciej.fijalkowski@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 09:42:20 -0700 [thread overview]
Message-ID: <bcf47a78-da23-41d6-b2d6-5df5b5d73e66@intel.com> (raw)
In-Reply-To: <ZTEcmh2GNi0Ugn3/@boxer>
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>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
next prev parent reply other threads:[~2023-10-19 16:42 UTC|newest]
Thread overview: 5+ 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 22:03 ` Jacob Keller
2023-10-19 12:10 ` Maciej Fijalkowski
2023-10-19 16:42 ` Jacob Keller [this message]
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=bcf47a78-da23-41d6-b2d6-5df5b5d73e66@intel.com \
--to=jacob.e.keller@intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=maciej.fijalkowski@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox