Re: [RFC PATCH net-next 1/3] net: ethtool: add symmetric Toeplitz RSS hash function

[Ahmed Zaki <ahmed.zaki@intel.com>]

On 2023-08-24 12:14, Jakub Kicinski wrote:
> CC Willem
>
> On Wed, 23 Aug 2023 10:48:29 -0600 Ahmed Zaki wrote:
>> Symmetric RSS hash functions are beneficial in applications that monitor
>> both Tx and Rx packets of the same flow (IDS, software firewalls, ..etc).
>> Getting all traffic of the same flow on the same RX queue results in
>> higher CPU cache efficiency.
>>
>> Allow ethtool to support symmetric Toeplitz algorithm. A user can set the
>> RSS function of the netdevice via:
>>      # ethtool -X eth0 hfunc symmetric_toeplitz
> Looks fairly reasonable, but there are two questions we need to answer:
>   - what do we do if RXH config includes fields which are by definition
>     not symmetric (l2 DA or in the future flow label)?
>   - my initial thought was the same as Saeed's - that the fields are
>     sorted, so how do we inform user about the exact implementation?
>
> One way to fix both problems would be to, instead of changing the hash
> function, change the RXH config. Add new "xor-ed" fields there.
>
> Another would be to name the function "XORSYM_TOP" and make the core
> check that it cannot be combined with uni-dir fields?
>
> I like the first option more.
>
> Either way, please make sure to add docs, and extend the toeplitz test
> for this.

When "Symmetric Toeplitz" is set in the NIC, the H/W will yield the same 
hash as the regular Toeplitz for protocol types that do not have such 
symmetric fields in both directions (i.e. there will be no RSS hash 
symmetry and the TX/RX traffic will land on different Rx queues).

The goal of this series is to enable the "default" behavior of the whole 
device ("-X hfunc") to be the symmetric hash (again, only for protocols 
that have symmetric src/dst counterparts). If I understand the first 
option correctly, the user would need to manually configure all RXH 
fields for all flow types (tcp4, udp4, sctp4, tcp6, ..etc), to get 
symmetric RSS on them, instead of the proposed single "-X" command? The 
second option is closer to what I had in mind. We can re-name and 
provide any details.

I agree that we will need to take care of some cases like if the user 
removes only "source IP" or "destination port" from the hash fields, 
without that field's counterpart (we can prevent this, or show a 
warning, ..etc). I was planning to address that in a follow-up series; 
ie. handling the "ethtool -U rx-flow-hash". Do you want that to be 
included in the same series as well?


>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 62b61527bcc4..9a8e1fb7170d 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -60,10 +60,11 @@ enum {
>>   	ETH_RSS_HASH_TOP_BIT, /* Configurable RSS hash function - Toeplitz */
>>   	ETH_RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
>>   	ETH_RSS_HASH_CRC32_BIT, /* Configurable RSS hash function - Crc32 */
>> +	ETH_RSS_HASH_SYM_TOP_BIT, /* Configurable RSS hash function - Symmetric Toeplitz */
>>   
>>   	/*
>>   	 * Add your fresh new hash function bits above and remember to update
>> -	 * rss_hash_func_strings[] in ethtool.c
>> +	 * rss_hash_func_strings[] in ethtool/common.c
>>   	 */
>>   	ETH_RSS_HASH_FUNCS_COUNT
>>   };
>> @@ -108,6 +109,7 @@ enum ethtool_supported_ring_param {
>>   #define __ETH_RSS_HASH(name)	__ETH_RSS_HASH_BIT(ETH_RSS_HASH_##name##_BIT)
>>   
>>   #define ETH_RSS_HASH_TOP	__ETH_RSS_HASH(TOP)
>> +#define ETH_RSS_HASH_SYM_TOP	__ETH_RSS_HASH(SYM_TOP)
>>   #define ETH_RSS_HASH_XOR	__ETH_RSS_HASH(XOR)
>>   #define ETH_RSS_HASH_CRC32	__ETH_RSS_HASH(CRC32)
>>   
>> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>> index f5598c5f50de..a0e0c6b2980e 100644
>> --- a/net/ethtool/common.c
>> +++ b/net/ethtool/common.c
>> @@ -81,6 +81,7 @@ rss_hash_func_strings[ETH_RSS_HASH_FUNCS_COUNT][ETH_GSTRING_LEN] = {
>>   	[ETH_RSS_HASH_TOP_BIT] =	"toeplitz",
>>   	[ETH_RSS_HASH_XOR_BIT] =	"xor",
>>   	[ETH_RSS_HASH_CRC32_BIT] =	"crc32",
>> +	[ETH_RSS_HASH_SYM_TOP_BIT] =	"symmetric_toeplitz",
>>   };
>>   
>>   const char

Thanks,

Ahmed