From: Kohei Enju <enjuk@amazon.com>
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>
Tested-by: Rinitha S <sx.rinitha@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 ++
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 45 ++++++++++++++++++---------
2 files changed, 33 insertions(+), 14 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 ca1ccc630001..351b6b82fa6b 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,21 @@ 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));
+ /* Ensure rss_i is non-zero to avoid division by zero */
+ if (!rss_i)
+ rss_i = 1;
- 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 +4345,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 +4360,21 @@ 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;
+ /* Ensure rss_i is non-zero to avoid division by zero */
+ if (!rss_i)
+ rss_i = 1;
- adapter->rss_indir_tbl[i] = j;
+ /* 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->last_rss_indices = rss_i;
+ adapter->last_reta_entries = reta_entries;
}
ixgbe_store_vfreta(adapter);
--
2.51.0.rc1.197.g6d975e95c9d7On Thu, 16 Oct 2025 23:08:42 -0700 Jacob Keller wrote:
> 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.
We should take this a step further and also not reinitialize if
netif_is_rxfh_configured(). Or am I missing something?
On Mon, 20 Oct 2025 18:32:46 -0700, Jakub Kicinski wrote:
>On Thu, 16 Oct 2025 23:08:42 -0700 Jacob Keller wrote:
>> 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.
>
>We should take this a step further and also not reinitialize if
>netif_is_rxfh_configured(). Or am I missing something?
Hi Jakub, thank you for reviewing.
Actually, you raise a good point about netif_is_rxfh_configured().
However, we can't determine whether we should reinitialize or not based
solely on netif_is_rxfh_configured(), since the RETA table is determined
by (1) the queue count and (2) the size of RETA table.
So, simply skipping reinitialization on netif_is_rxfh_configured() would
leave invalid RETA entries when those parameters are to be changed.
For example, consider a scenario where the queue count is 8 with user
configuration containing values from 0 to 7. When queue count changes
from 8 to 4 and we skip the reinitialization in this scenario, entries
pointing to queues 4-7 become invalid. The same issue applies when the
RETA table size changes.
Furthermore, IIUC, adding netif_is_rxfh_configured() to the current
condition wouldn't provide additional benefit. When parameters remain
unchanged, regardless of netif_is_rxfh_configured(), we already preserve
the RETA entries which might be user-configured or default values,
Therefore I believe the current logic is correct as is, and we should
reinitialize the RETA entries when either parameter changes, regardless
of netif_is_rxfh_configured().
On Tue, 21 Oct 2025 12:59:34 +0900 Kohei Enju wrote: > For example, consider a scenario where the queue count is 8 with user > configuration containing values from 0 to 7. When queue count changes > from 8 to 4 and we skip the reinitialization in this scenario, entries > pointing to queues 4-7 become invalid. The same issue applies when the > RETA table size changes. Core should reject this. See ethtool_check_max_channel() > Furthermore, IIUC, adding netif_is_rxfh_configured() to the current > condition wouldn't provide additional benefit. When parameters remain > unchanged, regardless of netif_is_rxfh_configured(), we already preserve > the RETA entries which might be user-configured or default values, User may decide to "isolate" (take out of RSS) a lower queue, to configure it for AF_XDP or other form of zero-copy. Install explicit rules to direct traffic to that queue. If you reset the RSS table random traffic will get stranded in the ZC queue (== dropped).
On Tue, 21 Oct 2025 16:10:06 -0700, Jakub Kicinski wrote:
>On Tue, 21 Oct 2025 12:59:34 +0900 Kohei Enju wrote:
>> For example, consider a scenario where the queue count is 8 with user
>> configuration containing values from 0 to 7. When queue count changes
>> from 8 to 4 and we skip the reinitialization in this scenario, entries
>> pointing to queues 4-7 become invalid. The same issue applies when the
>> RETA table size changes.
>
>Core should reject this. See ethtool_check_max_channel()
Indeed, you're right that the situation above will be rejected. I missed
it.
BTW, I think reinitializing the RETA table when queue count changes or
RETA table size changes is reasonable for predictability and safety.
Does this approach make sense to you?
>
>> Furthermore, IIUC, adding netif_is_rxfh_configured() to the current
>> condition wouldn't provide additional benefit. When parameters remain
>> unchanged, regardless of netif_is_rxfh_configured(), we already preserve
>> the RETA entries which might be user-configured or default values,
>
>User may decide to "isolate" (take out of RSS) a lower queue,
>to configure it for AF_XDP or other form of zero-copy. Install
>explicit rules to direct traffic to that queue. If you reset
>the RSS table random traffic will get stranded in the ZC queue
>(== dropped).
>
You're correct about the ZC queue scenario. The original implementation
(before this patch) would indeed cause this problem by unconditionally
reinitializing.
I believe this patch addresses that issue - it preserves the user
configuration since neither queue count nor RETA table size changes in
that case. If I'm misunderstanding your scenario, please let me know.
I could update the logic to explicitly check netif_is_rxfh_configured()
as in [1], though the actual behavior would be the same as [2] since
the default RETA table is a deterministic function of (rss_indices,
reta_entries):
[1] Check user configuration explicitly:
if (!netif_is_rxfh_configured(adapter->netdev) ||
adapter->last_rss_indices != rss_i ||
adapter->last_reta_entries != reta_entries) {
// reinitialize
}
[2] Current patch:
if (adapter->last_rss_indices != rss_i ||
adapter->last_reta_entries != reta_entries) {
// reinitialize
}
Do you have any preference between these approaches, or would you
recommend a different solution?
On Wed, 22 Oct 2025 12:40:45 +0900 Kohei Enju wrote:
> On Tue, 21 Oct 2025 16:10:06 -0700, Jakub Kicinski wrote:
> >On Tue, 21 Oct 2025 12:59:34 +0900 Kohei Enju wrote:
> >> For example, consider a scenario where the queue count is 8 with user
> >> configuration containing values from 0 to 7. When queue count changes
> >> from 8 to 4 and we skip the reinitialization in this scenario, entries
> >> pointing to queues 4-7 become invalid. The same issue applies when the
> >> RETA table size changes.
> >
> >Core should reject this. See ethtool_check_max_channel()
>
> Indeed, you're right that the situation above will be rejected. I missed
> it.
>
> BTW, I think reinitializing the RETA table when queue count changes or
> RETA table size changes is reasonable for predictability and safety.
> Does this approach make sense to you?
Yes, if !netif_is_rxfh_configured() re-initializing is expected.
> >> Furthermore, IIUC, adding netif_is_rxfh_configured() to the current
> >> condition wouldn't provide additional benefit. When parameters remain
> >> unchanged, regardless of netif_is_rxfh_configured(), we already preserve
> >> the RETA entries which might be user-configured or default values,
> >
> >User may decide to "isolate" (take out of RSS) a lower queue,
> >to configure it for AF_XDP or other form of zero-copy. Install
> >explicit rules to direct traffic to that queue. If you reset
> >the RSS table random traffic will get stranded in the ZC queue
> >(== dropped).
>
> You're correct about the ZC queue scenario. The original implementation
> (before this patch) would indeed cause this problem by unconditionally
> reinitializing.
>
> I believe this patch addresses that issue - it preserves the user
> configuration since neither queue count nor RETA table size changes in
> that case. If I'm misunderstanding your scenario, please let me know.
>
> I could update the logic to explicitly check netif_is_rxfh_configured()
> as in [1], though the actual behavior would be the same as [2] since
> the default RETA table is a deterministic function of (rss_indices,
> reta_entries):
>
> [1] Check user configuration explicitly:
> if (!netif_is_rxfh_configured(adapter->netdev) ||
> adapter->last_rss_indices != rss_i ||
> adapter->last_reta_entries != reta_entries) {
> // reinitialize
> }
>
> [2] Current patch:
> if (adapter->last_rss_indices != rss_i ||
> adapter->last_reta_entries != reta_entries) {
> // reinitialize
> }
>
> Do you have any preference between these approaches, or would you
> recommend a different solution?
I was expecting something like:
if (netif_is_rxfh_configured(adapter->netdev)) {
if (!check_that_rss_is_okay()) {
/* This should never happen, barring FW errors etc */
warn("user configuration lost due to XYZ");
reinit();
}
} else if (...rss_ind != rss_id ||
...reta_entries != reta_entries) {
reinit();
}
On Wed, 22 Oct 2025 17:26:49 -0700, Jakub Kicinski wrote:
>On Wed, 22 Oct 2025 12:40:45 +0900 Kohei Enju wrote:
>> On Tue, 21 Oct 2025 16:10:06 -0700, Jakub Kicinski wrote:
>> >On Tue, 21 Oct 2025 12:59:34 +0900 Kohei Enju wrote:
>> >> For example, consider a scenario where the queue count is 8 with user
>> >> configuration containing values from 0 to 7. When queue count changes
>> >> from 8 to 4 and we skip the reinitialization in this scenario, entries
>> >> pointing to queues 4-7 become invalid. The same issue applies when the
>> >> RETA table size changes.
>> >
>> >Core should reject this. See ethtool_check_max_channel()
>>
>> Indeed, you're right that the situation above will be rejected. I missed
>> it.
>>
>> BTW, I think reinitializing the RETA table when queue count changes or
>> RETA table size changes is reasonable for predictability and safety.
>> Does this approach make sense to you?
>
>Yes, if !netif_is_rxfh_configured() re-initializing is expected.
I got it.
>
>> >> Furthermore, IIUC, adding netif_is_rxfh_configured() to the current
>> >> condition wouldn't provide additional benefit. When parameters remain
>> >> unchanged, regardless of netif_is_rxfh_configured(), we already preserve
>> >> the RETA entries which might be user-configured or default values,
>> >
>> >User may decide to "isolate" (take out of RSS) a lower queue,
>> >to configure it for AF_XDP or other form of zero-copy. Install
>> >explicit rules to direct traffic to that queue. If you reset
>> >the RSS table random traffic will get stranded in the ZC queue
>> >(== dropped).
>>
>> You're correct about the ZC queue scenario. The original implementation
>> (before this patch) would indeed cause this problem by unconditionally
>> reinitializing.
>>
>> I believe this patch addresses that issue - it preserves the user
>> configuration since neither queue count nor RETA table size changes in
>> that case. If I'm misunderstanding your scenario, please let me know.
>>
>> I could update the logic to explicitly check netif_is_rxfh_configured()
>> as in [1], though the actual behavior would be the same as [2] since
>> the default RETA table is a deterministic function of (rss_indices,
>> reta_entries):
>>
>> [1] Check user configuration explicitly:
>> if (!netif_is_rxfh_configured(adapter->netdev) ||
>> adapter->last_rss_indices != rss_i ||
>> adapter->last_reta_entries != reta_entries) {
>> // reinitialize
>> }
>>
>> [2] Current patch:
>> if (adapter->last_rss_indices != rss_i ||
>> adapter->last_reta_entries != reta_entries) {
>> // reinitialize
>> }
>>
>> Do you have any preference between these approaches, or would you
>> recommend a different solution?
>
>I was expecting something like:
>
>if (netif_is_rxfh_configured(adapter->netdev)) {
> if (!check_that_rss_is_okay()) {
> /* This should never happen, barring FW errors etc */
> warn("user configuration lost due to XYZ");
> reinit();
> }
>} else if (...rss_ind != rss_id ||
> ...reta_entries != reta_entries) {
> reinit();
>}
Thank you for clarification.
At first glance, noting that check_that_rss_is_okay() would return false
when the RETA table size is larger than the previous one, since
user-configuration doesn't exist for the expanded portion of the RETA
table. This should happen in realistic scenarios even though there are
no hardware-related or HW errors.
Anyway I'll refine the patch using netif_is_rxfh_configured() and then
submit to iwl-next first.
© 2016 - 2025 Red Hat, Inc.