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: <IA3PR11MB8986B31F209249FFC30F0A35E501A@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Wed, 3 Sep 2025 06:21:05 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Kohei Enju <enjuk@...zon.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>,
	"kohei.enju@...il.com" <kohei.enju@...il.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: preserve RSS
 indirection table across admin down/up



> -----Original Message-----
> From: Kohei Enju <enjuk@...zon.com>
> Sent: Tuesday, September 2, 2025 11:17 PM
> To: enjuk@...zon.com
> Cc: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>;
> andrew+netdev@...n.ch; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> davem@...emloft.net; edumazet@...gle.com; intel-wired-
> lan@...ts.osuosl.org; kohei.enju@...il.com; kuba@...nel.org;
> netdev@...r.kernel.org; pabeni@...hat.com; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>
> 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