> -----Original Message-----
> From: Kohei Enju <[email protected]>
> Sent: Thursday, August 28, 2025 7:41 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: [email protected]; Nguyen, Anthony L
> <[email protected]>; [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 v2] ixgbe: preserve
> RSS indirection table across admin down/up
>
> On Thu, 28 Aug 2025 16:42:31 +0000, Loktionov, Aleksandr wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: Intel-wired-lan <[email protected]> On
> Behalf
> > > Of Kohei Enju
> > > Sent: Thursday, August 28, 2025 6:01 PM
> > > To: [email protected]; [email protected]
> > > Cc: Nguyen, Anthony L <[email protected]>; Kitszel,
> > > Przemyslaw <[email protected]>; Andrew Lunn
> > > <[email protected]>; David S. Miller <[email protected]>;
> Eric
> > > Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> > > Paolo Abeni <[email protected]>; [email protected]; Kohei
> Enju
> > > <[email protected]>
> > > Subject: [Intel-wired-lan] [PATCH iwl-next v2] 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.
> > >
> > > 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 <[email protected]>
> > > ---
> > > Changes:
> > > v1->v2:
> > > - 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-
> > > [email protected]/
> > > ---
> > > drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +
> > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 41
> +++++++++++++---
> > > ---
> > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > > b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > > index 14d275270123..da3b23bdcce1 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_i;
> > last_rss_i is cryptic; please, consider last_rss_indices (or
> similar)
>
> Sure, I'll rename it to last_rss_indices for clarity.
>
> >
> > >
> > > #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..05dfb06173d4 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > @@ -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));
> > > + /* 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_i != rss_i ||
> > > + adapter->last_reta_entries != reta_entries) {
> > > + for (i = 0, j = 0; i < reta_entries; i++, j++) {
> > You can avoid the top-of-loop equality test by using modulo, which
> reads easier, like:
> > for (i = 0, j = 0; i < reta_entries; i++, j++)
> > adapter->rss_indir_tbl[i] = j % rss_i;
>
> I got it. I'll use modulo and then j can be removed like:
> for (i = 0; i < reta_entries; i++)
> adapter->rss_indir_tbl[i] = i % rss_i;
>
> >
> > > + if (j == rss_i)
> > > + j = 0;
> > >
> > > - for (i = 0, j = 0; i < reta_entries; i++, j++) {
> > > - if (j == rss_i)
> > > - j = 0;
> > > + adapter->rss_indir_tbl[i] = j;
> > > + }
> > >
> > > - adapter->rss_indir_tbl[i] = j;
> > > + adapter->last_rss_i = rss_i;
> > > + adapter->last_reta_entries = reta_entries;
> > > }
> > >
> > > ixgbe_store_reta(adapter);
> > > @@ -4338,8 +4345,9 @@ static void ixgbe_setup_reta(struct
> > > ixgbe_adapter *adapter)
> > >
> > > static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter)
> {
> > > - struct ixgbe_hw *hw = &adapter->hw;
> > > u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
> > > + struct ixgbe_hw *hw = &adapter->hw;
> > > + u32 reta_entries = 64;
> > Magic number. Can you #define IXGBE_VFRETA_ENTRIES 64 ?
>
> You're right about the magic number.
> I see it was introduced in commit 0f9b232b176d ("ixgbe: add support
> for
> X550 extended RSS support").
>
> I'm considering using ixgbe_rss_indir_tbl_entries() instead of
> #define to avoid the magic number, since ixgbe_store_vfreta()
> already uses it.
> This would ensure consistency between the two functions. Would that
> be acceptable, or would you prefer a #define?
>
Good catch!
ixgbe_rss_indir_tbl_entries() is proper solution ,because it's h/w and
configuration dependent.
> >
> > > int i, j;
> > >
> > > /* Fill out hash function seeds */ @@ -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;
> > > + /* 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_i != rss_i ||
> > > + adapter->last_reta_entries != reta_entries) {
> > > + for (i = 0, j = 0; i < reta_entries; i++, j++) {
> > > + if (j == rss_i)
> > > + j = 0;
> > > +
> > > + adapter->rss_indir_tbl[i] = j;
> > > + }
> > >
> > > - adapter->rss_indir_tbl[i] = j;
> > > + adapter->last_rss_i = rss_i;
> > > + adapter->last_reta_entries = reta_entries;
> > > }
> > >
> > > ixgbe_store_vfreta(adapter);
> > > --
> > > 2.51.0
