| 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: <20250825113743.56559-1-enjuk@amazon.com>
Date: Mon, 25 Aug 2025 20:36:54 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <przemyslaw.kitszel@...el.com>
CC: <andrew+netdev@...n.ch>, <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>,
<pmenzel@...gen.mpg.de>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1] ixgbe: preserve RSS
indirection table across admin down/up
On Mon, 25 Aug 2025 11:48:21 +0200, Przemek Kitszel wrote:
>On 8/24/25 13:20, Kohei Enju 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.
>>
>> By adopting the same approach used in igc and igb drivers which
>> reinitializes the RSS indirection table only when the queue count
>> changes, let's make user configuration persistent as long as queue count
>> remains 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>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 37 +++++++++++++------
>> 2 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 14d275270123..d8b088c90b05 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -838,6 +838,7 @@ struct ixgbe_adapter {
>> */
>> #define IXGBE_MAX_RETA_ENTRIES 512
>> u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>> + u16 last_rss_i;
>>
>> #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 80e6a2ef1350..dc5a8373b0c3 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -4318,14 +4318,22 @@ 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 or queue count
>> + * change, otherwise preserve user configurations. Then always
>> + * write to hardware.
>> + */
>> + if (adapter->last_rss_i != rss_i) {
>> + memset(adapter->rss_indir_tbl, 0,
>> + sizeof(adapter->rss_indir_tbl));
>
>Thank you for the patch,
>I see no point in the memset() above, especially given 0 as a valid
>entry in the table.
Indeed, you're right. I'll remove that.
Thank you for reviewing.
BTW, I noticed that this patch would unintentionally skip
reinitialization when rss_i remains unchanged but reta_entries changes.
I apologize for overlooking this in my initial patch.
For example, on some devices reta_entries changes according to its
SR-IOV status (IXGBE_FLAG_SRIOV_ENABLED). By setting queue count in
advance, we may be able to create a situation where rss_i remains
unchanged but reta_entries changes. In this case, this patch would cause
an unintentional skip of reinitialization.
Unlike igb/igc, ixgbe's reta_entries may change, so IIUC I need to check
reta_entries as well in ixgbe_setup_reta() and ixgbe_setup_vfreta()
like:
/* 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->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;
>>
>> - 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;
>> }
>>
>> ixgbe_store_reta(adapter);
>> @@ -4347,12 +4355,19 @@ 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 or queue count
>> + * change, otherwise preserve user configurations. Then always
>> + * write to hardware.
>> + */
>> + if (adapter->last_rss_i != rss_i) {
>> + for (i = 0, j = 0; i < 64; 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;
>> }
>>
>> ixgbe_store_vfreta(adapter);
>
Powered by blists - more mailing lists