netdev - Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: preserve RSS indirection table across admin down/up

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]

Message-ID: <75f4d389-eaff-4d61-880a-ccb05cd55123@intel.com>
Date: Fri, 5 Sep 2025 10:53:37 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>, Kohei Enju
	<enjuk@...zon.com>, "kohei.enju@...il.com" <kohei.enju@...il.com>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"kuba@...nel.org" <kuba@...nel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: preserve RSS
 indirection table across admin down/up


>>>>> +	if (adapter->last_rss_indices != rss_i ||
>>>>> +	    adapter->last_reta_entries != reta_entries) {
>>>>> +		for (i = 0; i < reta_entries; i++)
>>>>> +			adapter->rss_indir_tbl[i] = i % rss_i;
>>>> Are you sure rss_i never ever can be a 0?
>>>> This is the only thing I'm worrying about.
>>>
>>> Oops, you're exactly right. Good catch!
>>>
>>> I see the original code assigns 0 to rss_indir_tbl[i] when rss_i is
>> 0,
>>> like:
>>>   adapter->rss_indir_tbl[i] = 0;
>>
>> Ahh, that's not true, my brain was not working... Sorry for messing
>> up.
>> Anyway, in a situation where rss_i == 0, we should handle it somehow
>> to avoid zero-divisor.
>>
>>>
>>> To handle this with keeping the behavior when rss_i == 0, I'm
>>> considering Option 1:
>>>   adapter->rss_indir_tbl[i] = rss_i ? i % rss_i : 0;
>>>
>>> Option 2:
>>>   if (rss_i)
>>>       for (i = 0; i < reta_entries; i++)
>>>           adapter->rss_indir_tbl[i] = i % rss_i;
>>>   else
>>>       memset(adapter->rss_indir_tbl, 0, reta_entries);
>>>
>>> Since this is not in the data path, the overhead of checking rss_i in
>>> each iteration might be acceptable. Therefore I'd like to adopt the
>>> option 1 for simplicity.
>>>
>>> Do you have any preference or other suggestions?
> 
> I lean toward option 2, as the explicit if (rss_i) guard makes the logic clearer and easier to follow.
> 
> Handling the simplified case first with:
> if (unlikely(!rss_i))
>      memset(adapter->rss_indir_tbl, 0, reta_entries);
> else
>      for (i = 0; i < reta_entries; i++)
>          adapter->rss_indir_tbl[i] = i % rss_i;
> 
> Improves readability and separates the edge case from the main logic.
> 
> While it's possible to use a ternary expression like adapter->rss_indir_tbl[i] = rss_i ? i % rss_i : 0;,
> I find the conditional block more maintainable, especially if this logic evolves later.
> 
> Regarding unlikely(), unless there's profiling data showing a performance benefit,
> I'd avoid it here - this isn't in the fast path, and clarity should take precedence.
> With the best regards Alex

I would make it even simpler (than if/else paths):

if (!rss_i)
	rss_i = 1;

(which looks better than "should be obvious" oneliner, rss_i += !rss_i;)