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

[Kohei Enju <enjuk@amazon.com>]

On Wed, 3 Sep 2025 06:21:05 +0000, Loktionov, Aleksandr wrote:

> [...]
>> On Wed, 3 Sep 2025 06:04:43 +0900, Kohei Enju wrote:
>> 
>> >On Tue, 2 Sep 2025 13:25:56 +0000, Loktionov, Aleksandr wrote:
>> >
>> >> [...]
>> >>> -
>> >>> -	for (i = 0, j = 0; i < reta_entries; i++, j++) {
>> >>> -		if (j == rss_i)
>> >>> -			j = 0;
>> >>> +	/* Update redirection table in memory on first init, queue
>> >>> count change,
>> >>> +	 * or reta entries change, otherwise preserve user
>> >>> configurations. Then
>> >>> +	 * always write to hardware.
>> >>> +	 */
>> >>> +	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.

Okay, I got it.

>
>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.

Yes, I agree on that this isn't in the fast path therefore unlikely is
not always necessary.

Thank you again for reviewing and suggesting!

>With the best regards Alex