| 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
| ||
|
Message-ID: <IA3PR11MB8986220F78939C9A26D6D40CE53BA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Thu, 28 Aug 2025 19:34:41 +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 v2] ixgbe: preserve RSS
indirection table across admin down/up
> -----Original Message-----
> From: Kohei Enju <enjuk@...zon.com>
> Sent: Thursday, August 28, 2025 7:41 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
> Cc: andrew+netdev@...n.ch; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; davem@...emloft.net;
> edumazet@...gle.com; enjuk@...zon.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 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 <intel-wired-lan-bounces@...osl.org> On
> Behalf
> > > Of Kohei Enju
> > > Sent: Thursday, August 28, 2025 6:01 PM
> > > To: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org
> > > Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Kitszel,
> > > Przemyslaw <przemyslaw.kitszel@...el.com>; Andrew Lunn
> > > <andrew+netdev@...n.ch>; David S. Miller <davem@...emloft.net>;
> Eric
> > > Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>;
> > > Paolo Abeni <pabeni@...hat.com>; kohei.enju@...il.com; Kohei
> Enju
> > > <enjuk@...zon.com>
> > > 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 <enjuk@...zon.com>
> > > ---
> > > 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-
> > > 1-enjuk@...zon.com/
> > > ---
> > > 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
Powered by blists - more mailing lists