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

[Jacob Keller <jacob.e.keller@intel.com>]

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>