netdev - Re: [PATCH net] i40e: xsk: remove count

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 19 Oct 2023 09:42:20 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<anthony.l.nguyen@...el.com>, <magnus.karlsson@...el.com>, Tushar Vyavahare
	<tushar.vyavahare@...el.com>
Subject: Re: [PATCH net] i40e: xsk: remove count_mask



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@...el.com>