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

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

[Kohei Enju]

Currently, the RSS indirection table configured by user via ethtool is
reinitialized to default values during interface resets (e.g., admin
down/up, MTU change). As for RSS hash key, commit 3dfbfc7ebb95 ("ixgbe:
Check for RSS key before setting value") made it persistent across
interface resets.

Adopt the same approach used in igc and igb drivers which reinitializes
the RSS indirection table only when the queue count changes. Since the
number of RETA entries can also change in ixgbe, let's make user
configuration persistent as long as both queue count and the number of
RETA entries remain unchanged.

Tested on Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network
Connection.

Test:
Set custom indirection table and check the value after interface down/up

  # ethtool --set-rxfh-indir ens5 equal 2
  # ethtool --show-rxfh-indir ens5 | head -5

  RX flow hash indirection table for ens5 with 12 RX ring(s):
      0:      0     1     0     1     0     1     0     1
      8:      0     1     0     1     0     1     0     1
     16:      0     1     0     1     0     1     0     1
  # ip link set dev ens5 down && ip link set dev ens5 up

Without patch:
  # ethtool --show-rxfh-indir ens5 | head -5

  RX flow hash indirection table for ens5 with 12 RX ring(s):
      0:      0     1     2     3     4     5     6     7
      8:      8     9    10    11     0     1     2     3
     16:      4     5     6     7     8     9    10    11

With patch:
  # ethtool --show-rxfh-indir ens5 | head -5

  RX flow hash indirection table for ens5 with 12 RX ring(s):
      0:      0     1     0     1     0     1     0     1
      8:      0     1     0     1     0     1     0     1
     16:      0     1     0     1     0     1     0     1

Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
Changes:
  v2->v3:
    - s/last_rss_i/last_rss_indices/ for clarity
    - use modulo instead of top-of-loop equality test
    - use ixgbe_rss_indir_tbl_entries() instead of magic number
  v1->v2: https://lore.kernel.org/intel-wired-lan/20250828160134.81286-1-enjuk@amazon.com/
    - remove pointless memset() in  ixgbe_setup_reta()
    - add check for reta_entries in addition to rss_i
    - update the commit message to reflect the additional check
  v1: https://lore.kernel.org/intel-wired-lan/20250824112037.32692-1-enjuk@amazon.com/
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 39 ++++++++++++-------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 14d275270123..3553bf659d42 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -838,6 +838,8 @@ struct ixgbe_adapter {
  */
 #define IXGBE_MAX_RETA_ENTRIES 512
 	u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
+	u32 last_reta_entries;
+	u16 last_rss_indices;
 
 #define IXGBE_RSS_KEY_SIZE     40  /* size of RSS Hash Key in bytes */
 	u32 *rss_key;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 27040076f068..395906cf7de6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4309,9 +4309,9 @@ static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
 
 static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
 {
-	u32 i, j;
 	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
+	u32 i;
 
 	/* Program table for at least 4 queues w/ SR-IOV so that VFs can
 	 * make full use of any rings they may have.  We will use the
@@ -4323,14 +4323,17 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
 	/* Fill out hash function seeds */
 	ixgbe_store_key(adapter);
 
-	/* Fill out redirection table */
-	memset(adapter->rss_indir_tbl, 0, sizeof(adapter->rss_indir_tbl));
-
-	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;
 
-		adapter->rss_indir_tbl[i] = j;
+		adapter->last_rss_indices = rss_i;
+		adapter->last_reta_entries = reta_entries;
 	}
 
 	ixgbe_store_reta(adapter);
@@ -4338,9 +4341,10 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
 
 static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
 {
-	struct ixgbe_hw *hw = &adapter->hw;
+	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
 	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
-	int i, j;
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i;
 
 	/* Fill out hash function seeds */
 	for (i = 0; i < 10; i++) {
@@ -4352,12 +4356,17 @@ static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
 					*(adapter->rss_key + i));
 	}
 
-	/* Fill out the redirection table */
-	for (i = 0, j = 0; i < 64; 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;
 
-		adapter->rss_indir_tbl[i] = j;
+		adapter->last_rss_indices = rss_i;
+		adapter->last_reta_entries = reta_entries;
 	}
 
 	ixgbe_store_vfreta(adapter);
-- 
2.51.0

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Tuesday, September 2, 2025 2:11 PM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; kohei.enju@gmail.com; Kohei Enju
> <enjuk@amazon.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: preserve RSS
> indirection table across admin down/up
> 
> Currently, the RSS indirection table configured by user via ethtool is
> reinitialized to default values during interface resets (e.g., admin
> down/up, MTU change). As for RSS hash key, commit 3dfbfc7ebb95
> ("ixgbe:
> Check for RSS key before setting value") made it persistent across
> interface resets.
> 
> Adopt the same approach used in igc and igb drivers which
> reinitializes
> the RSS indirection table only when the queue count changes. Since the
> number of RETA entries can also change in ixgbe, let's make user
> configuration persistent as long as both queue count and the number of
> RETA entries remain unchanged.
> 
> Tested on Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network
> Connection.
> 

...

>  static void ixgbe_setup_reta(struct ixgbe_adapter *adapter)
>  {
> -	u32 i, j;
>  	u32 reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>  	u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
> +	u32 i;
> 
>  	/* Program table for at least 4 queues w/ SR-IOV so that VFs
> can
>  	 * make full use of any rings they may have.  We will use the
> @@ -4323,14 +4323,17 @@ static void ixgbe_setup_reta(struct
> ixgbe_adapter *adapter)
>  	/* Fill out hash function seeds */
>  	ixgbe_store_key(adapter);
> 
> -	/* Fill out redirection table */
> -	memset(adapter->rss_indir_tbl, 0, sizeof(adapter-
> >rss_indir_tbl));
> -
> -	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.

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

[Kohei Enju]

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;

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?

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

[Kohei Enju]

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;

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?

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

[Kohei Enju]

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?

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Kohei Enju <enjuk@amazon.com>
> Sent: Tuesday, September 2, 2025 11:17 PM
> To: enjuk@amazon.com
> Cc: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>;
> andrew+netdev@lunn.ch; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; edumazet@google.com; intel-wired-
> lan@lists.osuosl.org; kohei.enju@gmail.com; kuba@kernel.org;
> netdev@vger.kernel.org; pabeni@redhat.com; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.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

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

[Przemek Kitszel]

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

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

[Kohei Enju]

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

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

[Kohei Enju]

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;