> -----Original Message-----
> From: Kohei Enju <[email protected]>
> Sent: Tuesday, September 2, 2025 11:17 PM
> To: [email protected]
> Cc: Loktionov, Aleksandr <[email protected]>;
> [email protected]; Nguyen, Anthony L <[email protected]>;
> [email protected]; [email protected]; intel-wired-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Kitszel, Przemyslaw
> <[email protected]>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: preserve RSS
> indirection table across admin down/up
>
> 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.
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
