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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250906033534.76837-1-enjuk@amazon.com>
Date: Sat, 6 Sep 2025 12:35:28 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <przemyslaw.kitszel@...el.com>
CC: <aleksandr.loktionov@...el.com>, <andrew+netdev@...n.ch>,
	<anthony.l.nguyen@...el.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<enjuk@...zon.com>, <intel-wired-lan@...ts.osuosl.org>,
	<kohei.enju@...il.com>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: preserve RSS indirection table across admin down/up

On Fri, 5 Sep 2025 10:53:37 +0200, Przemek Kitszel wrote:

> 
>>>>>> +	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;)
>

Sounds good.

Considering comparing adapter->last_rss_indices and rss_i before
configuring rss_indir_tbl and saving rss_i to adapter->last_rss_indices
afterwards, I think I have to do that before the comparison. 
Is my understanding correct?

+	if (!rss_i)
+	    rss_i = 1;
+   
+	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;