[RFC PATCH net-next 0/3] Support Symmetric Toeplitz RSS hash

[RFC PATCH net-next 0/3] Support Symmetric Toeplitz RSS hash

[Ahmed Zaki]

We are looking for comments from the community on how to best add support
for the "Symmetric Toeplitz" hash RSS algorithm available on Intel's E800 
NICs. 

The first patch adds the support via "ethtool -X hfunc <alg>". Patches 2
and 3 add support in the ice driver. Support for the iavf driver will be
added later.

Ahmed Zaki (3):
  net: ethtool: add symmetric Toeplitz RSS hash function
  ice: fix ICE_AQ_VSI_Q_OPT_RSS_* register values
  ice: add support for symmetric Toeplitz RSS hash function

 drivers/net/ethernet/intel/ice/ice.h          |  2 +
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  8 +--
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 11 +++-
 drivers/net/ethernet/intel/ice/ice_lib.c      | 12 ++---
 drivers/net/ethernet/intel/ice/ice_main.c     | 52 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  8 ++-
 include/linux/ethtool.h                       |  4 +-
 net/ethtool/common.c                          |  1 +
 8 files changed, 80 insertions(+), 18 deletions(-)

-- 
2.39.2

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

[Ahmed Zaki]

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

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 include/linux/ethtool.h | 4 +++-
 net/ethtool/common.c    | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

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
-- 
2.39.2

[RFC PATCH net-next 2/3] ice: fix ICE_AQ_VSI_Q_OPT_RSS_* register values

[Ahmed Zaki]

Fix the values of the ICE_AQ_VSI_Q_OPT_RSS_* registers. Shifting is
already done when the values are used, no need to double shift. Bug was
not discovered earlier since only ICE_AQ_VSI_Q_OPT_RSS_TPLZ (Zero) is
currently used.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 8 ++++----
 drivers/net/ethernet/intel/ice/ice_virtchnl.c   | 8 +++-----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 29f7a9852aec..b6c66dea92cc 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -491,10 +491,10 @@ struct ice_aqc_vsi_props {
 #define ICE_AQ_VSI_Q_OPT_RSS_GBL_LUT_M		(0xF << ICE_AQ_VSI_Q_OPT_RSS_GBL_LUT_S)
 #define ICE_AQ_VSI_Q_OPT_RSS_HASH_S		6
 #define ICE_AQ_VSI_Q_OPT_RSS_HASH_M		(0x3 << ICE_AQ_VSI_Q_OPT_RSS_HASH_S)
-#define ICE_AQ_VSI_Q_OPT_RSS_TPLZ		(0x0 << ICE_AQ_VSI_Q_OPT_RSS_HASH_S)
-#define ICE_AQ_VSI_Q_OPT_RSS_SYM_TPLZ		(0x1 << ICE_AQ_VSI_Q_OPT_RSS_HASH_S)
-#define ICE_AQ_VSI_Q_OPT_RSS_XOR		(0x2 << ICE_AQ_VSI_Q_OPT_RSS_HASH_S)
-#define ICE_AQ_VSI_Q_OPT_RSS_JHASH		(0x3 << ICE_AQ_VSI_Q_OPT_RSS_HASH_S)
+#define ICE_AQ_VSI_Q_OPT_RSS_TPLZ		0x0
+#define ICE_AQ_VSI_Q_OPT_RSS_SYM_TPLZ		0x1
+#define ICE_AQ_VSI_Q_OPT_RSS_XOR		0x2
+#define ICE_AQ_VSI_Q_OPT_RSS_JHASH		0x3
 	u8 q_opt_tc;
 #define ICE_AQ_VSI_Q_OPT_TC_OVR_S		0
 #define ICE_AQ_VSI_Q_OPT_TC_OVR_M		(0x1F << ICE_AQ_VSI_Q_OPT_TC_OVR_S)
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 4a02ed91ba73..d9e1ee20d695 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -829,11 +829,9 @@ static int ice_vc_handle_rss_cfg(struct ice_vf *vf, u8 *msg, bool add)
 			goto error_param;
 		}
 
-		ctx->info.q_opt_rss = ((lut_type <<
-					ICE_AQ_VSI_Q_OPT_RSS_LUT_S) &
-				       ICE_AQ_VSI_Q_OPT_RSS_LUT_M) |
-				       (hash_type &
-					ICE_AQ_VSI_Q_OPT_RSS_HASH_M);
+		ctx->info.q_opt_rss =
+			FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_LUT_M, lut_type) |
+			FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_HASH_M, hash_type);
 
 		/* Preserve existing queueing option setting */
 		ctx->info.q_opt_rss |= (vsi->info.q_opt_rss &
-- 
2.39.2

[RFC PATCH net-next 3/3] ice: add support for symmetric Toeplitz RSS hash function

[Ahmed Zaki]

Support symmetric Toeplitz RSS hash function.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h         |  2 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 11 ++++-
 drivers/net/ethernet/intel/ice/ice_lib.c     | 12 ++---
 drivers/net/ethernet/intel/ice/ice_main.c    | 52 ++++++++++++++++++++
 4 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 5022b036ca4f..e68648c35085 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -356,6 +356,7 @@ struct ice_vsi {
 	/* RSS config */
 	u16 rss_table_size;	/* HW RSS table size */
 	u16 rss_size;		/* Allocated RSS queues */
+	u8 rss_hfunc;		/* User configured hash function */
 	u8 *rss_hkey_user;	/* User configured hash keys */
 	u8 *rss_lut_user;	/* User configured lookup table entries */
 	u8 rss_lut_type;	/* used to configure Get/Set RSS LUT AQ call */
@@ -892,6 +893,7 @@ int ice_set_rss_lut(struct ice_vsi *vsi, u8 *lut, u16 lut_size);
 int ice_get_rss_lut(struct ice_vsi *vsi, u8 *lut, u16 lut_size);
 int ice_set_rss_key(struct ice_vsi *vsi, u8 *seed);
 int ice_get_rss_key(struct ice_vsi *vsi, u8 *seed);
+int ice_set_rss_hfunc(struct ice_vsi *vsi, u8 hfunc);
 void ice_fill_rss_lut(u8 *lut, u16 rss_table_size, u16 rss_size);
 int ice_schedule_reset(struct ice_pf *pf, enum ice_reset_req reset);
 void ice_print_link_msg(struct ice_vsi *vsi, bool isup);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index ad4d4702129f..9e22e22b895d 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3148,7 +3148,7 @@ ice_get_rxfh_context(struct net_device *netdev, u32 *indir,
 	}
 
 	if (hfunc)
-		*hfunc = ETH_RSS_HASH_TOP;
+		*hfunc = vsi->rss_hfunc;
 
 	if (!indir)
 		return 0;
@@ -3215,7 +3215,8 @@ ice_set_rxfh(struct net_device *netdev, const u32 *indir, const u8 *key,
 	int err;
 
 	dev = ice_pf_to_dev(pf);
-	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
+	if (hfunc != ETH_RSS_HASH_NO_CHANGE &&
+	    hfunc != ETH_RSS_HASH_TOP && hfunc != ETH_RSS_HASH_SYM_TOP)
 		return -EOPNOTSUPP;
 
 	if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
@@ -3229,6 +3230,12 @@ ice_set_rxfh(struct net_device *netdev, const u32 *indir, const u8 *key,
 		return -EOPNOTSUPP;
 	}
 
+	if (hfunc != ETH_RSS_HASH_NO_CHANGE) {
+		err = ice_set_rss_hfunc(vsi, hfunc);
+		if (err)
+			return err;
+	}
+
 	if (key) {
 		if (!vsi->rss_hkey_user) {
 			vsi->rss_hkey_user =
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 201570cd2e0b..160bce8f68b7 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -1186,12 +1186,10 @@ static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
 	case ICE_VSI_PF:
 		/* PF VSI will inherit RSS instance of PF */
 		lut_type = ICE_AQ_VSI_Q_OPT_RSS_LUT_PF;
-		hash_type = ICE_AQ_VSI_Q_OPT_RSS_TPLZ;
 		break;
 	case ICE_VSI_VF:
 		/* VF VSI will gets a small RSS table which is a VSI LUT type */
 		lut_type = ICE_AQ_VSI_Q_OPT_RSS_LUT_VSI;
-		hash_type = ICE_AQ_VSI_Q_OPT_RSS_TPLZ;
 		break;
 	default:
 		dev_dbg(dev, "Unsupported VSI type %s\n",
@@ -1199,10 +1197,12 @@ static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
 		return;
 	}
 
-	ctxt->info.q_opt_rss = ((lut_type << ICE_AQ_VSI_Q_OPT_RSS_LUT_S) &
-				ICE_AQ_VSI_Q_OPT_RSS_LUT_M) |
-				((hash_type << ICE_AQ_VSI_Q_OPT_RSS_HASH_S) &
-				 ICE_AQ_VSI_Q_OPT_RSS_HASH_M);
+	vsi->rss_hfunc = ETH_RSS_HASH_TOP;
+	hash_type = ICE_AQ_VSI_Q_OPT_RSS_TPLZ;
+
+	ctxt->info.q_opt_rss =
+		FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_LUT_M, lut_type) |
+		FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_HASH_M, hash_type);
 }
 
 static void
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index c8286adae946..353fbc75f2c2 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -7671,6 +7671,58 @@ int ice_get_rss_key(struct ice_vsi *vsi, u8 *seed)
 	return status;
 }
 
+/**
+ * ice_set_rss_hfunc - Set RSS HASH function
+ * @vsi: Pointer to VSI structure
+ * @hfunc: ethtool hash function (ETH_RSS_HASH_*)
+ *
+ * Returns 0 on success, negative on failure
+ */
+int ice_set_rss_hfunc(struct ice_vsi *vsi, u8 hfunc)
+{
+	struct ice_vsi_ctx *ctx;
+	u8 hash_type;
+	int err;
+
+	if (hfunc == vsi->rss_hfunc)
+		return 0;
+
+	switch (hfunc) {
+	case ETH_RSS_HASH_TOP:
+		hash_type = ICE_AQ_VSI_Q_OPT_RSS_TPLZ;
+		break;
+	case ETH_RSS_HASH_SYM_TOP:
+		hash_type = ICE_AQ_VSI_Q_OPT_RSS_SYM_TPLZ;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_Q_OPT_VALID);
+	ctx->info.q_opt_rss = vsi->info.q_opt_rss;
+	ctx->info.q_opt_rss &= ~ICE_AQ_VSI_Q_OPT_RSS_HASH_M;
+	ctx->info.q_opt_rss |=
+		FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_HASH_M, hash_type);
+	ctx->info.q_opt_tc = vsi->info.q_opt_tc;
+	ctx->info.q_opt_flags = vsi->info.q_opt_rss;
+
+	err = ice_update_vsi(&vsi->back->hw, vsi->idx, ctx, NULL);
+	if (err) {
+		dev_err(ice_pf_to_dev(vsi->back), "Failed to configure RSS hash for VSI %d, error %d\n",
+			vsi->vsi_num, err);
+	} else {
+		vsi->info.q_opt_rss = ctx->info.q_opt_rss;
+		vsi->rss_hfunc = hfunc;
+	}
+
+	kfree(ctx);
+	return err;
+}
+
 /**
  * ice_bridge_getlink - Get the hardware bridge mode
  * @skb: skb buff
-- 
2.39.2

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

[Saeed Mahameed]

On 23 Aug 10:48, 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.
>

Can you please shed more light on the use case and configuration? 
Where do you expect the same flow/connection rx/tx to be received by the
same rxq in a nic driver?

>Allow ethtool to support symmetric Toeplitz algorithm. A user can set the
>RSS function of the netdevice via:
>    # ethtool -X eth0 hfunc symmetric_toeplitz
>

What is the expectation of the symmetric toeplitz hash, how do you achieve
that? by sorting packet fields? which fields?

Can you please provide a link to documentation/spec?
We should make sure all vendors agree on implementation and expectation of
the symmetric hash function.

>Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>---
> include/linux/ethtool.h | 4 +++-
> net/ethtool/common.c    | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
>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
>-- 
>2.39.2
>
>

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

[Ahmed Zaki]

On 2023-08-23 13:45, Saeed Mahameed wrote:
> On 23 Aug 10:48, 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.
>>
>
> Can you please shed more light on the use case and configuration? 
> Where do you expect the same flow/connection rx/tx to be received by the
> same rxq in a nic driver?

The use case is usually an application running on a intermediate server 
(not an endpoint of the flow) monitoring and reading both directions of 
the flow. Applications like intrusion detection systems or user-space 
state-full firewalls. For best CPU and cache efficiencies, we would need 
both flows to land on the same rx queue of that intermediate server. The 
paper in [1] gives more background on Symmetric Toeplitz (but imposes 
some restrictions on the LUT keys to get the hash symmetry).

>
>> Allow ethtool to support symmetric Toeplitz algorithm. A user can set 
>> the
>> RSS function of the netdevice via:
>>    # ethtool -X eth0 hfunc symmetric_toeplitz
>>
>
> What is the expectation of the symmetric toeplitz hash, how do you 
> achieve
> that? by sorting packet fields? which fields?
>
> Can you please provide a link to documentation/spec?
> We should make sure all vendors agree on implementation and 
> expectation of
> the symmetric hash function.

The way the Intel NICs are achieving this hash symmetry is by XORing the 
source and destination values of the IP and L4 ports and then feeding 
these values to the regular Toeplitz (in-tree) hash algorithm.

For example, for UDP/IPv4, the input fields for the Toeplitz hash would be:

(SRC_IP, DST_IP, SRC_PORT,  DST_PORT)

If symmetric Toeplitz is set, the NIC XOR the src and dst fields:

(SRC_IP^DST_IP ,  SRC_IP^DST_IP, SRC_PORT^DST_PORT, SRC_PORT^DST_PORT)

This way, the output hash would be the same for both flow directions. 
Same is applicable for IPv6, TCP and SCTP.

Regarding the documentation, the above is available in our public 
datasheets [2]. In the final version, I can add similar explanation in 
the headers (kdoc) and under "Documentation/networking/" so that there 
is a clear understanding of the algorithm.


[1] https://www.ndsl.kaist.edu/~kyoungsoo/papers/TR-symRSS.pdf

[2] E810 datasheet: 7.10.10.2 : Symmetric Hash

https://www.intel.com/content/www/us/en/content-details/613875/intel-ethernet-controller-e810-datasheet.html 



>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>> ---
>> include/linux/ethtool.h | 4 +++-
>> net/ethtool/common.c    | 1 +
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> 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
>> -- 
>> 2.39.2
>>
>>

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

[Jakub Kicinski]

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.

> 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

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

[Saeed Mahameed]

On 24 Aug 07:14, Ahmed Zaki wrote:
>
>On 2023-08-23 13:45, Saeed Mahameed wrote:
>>On 23 Aug 10:48, 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.
>>>

...

>>
>>What is the expectation of the symmetric toeplitz hash, how do you 
>>achieve
>>that? by sorting packet fields? which fields?
>>
>>Can you please provide a link to documentation/spec?
>>We should make sure all vendors agree on implementation and 
>>expectation of
>>the symmetric hash function.
>
>The way the Intel NICs are achieving this hash symmetry is by XORing 
>the source and destination values of the IP and L4 ports and then 
>feeding these values to the regular Toeplitz (in-tree) hash algorithm.
>
>For example, for UDP/IPv4, the input fields for the Toeplitz hash would be:
>
>(SRC_IP, DST_IP, SRC_PORT,  DST_PORT)
>

So you mangle the input. This is different than the paper you
referenced below which doesn't change the input but it modifies the RSS
algorithm and uses a special hash key.

>If symmetric Toeplitz is set, the NIC XOR the src and dst fields:
>
>(SRC_IP^DST_IP ,  SRC_IP^DST_IP, SRC_PORT^DST_PORT, SRC_PORT^DST_PORT)
>
>This way, the output hash would be the same for both flow directions. 
>Same is applicable for IPv6, TCP and SCTP.
>

I understand the motivation, I just want to make sure the interpretation is
clear, I agree with Jakub, we should use a clear name for the ethtool
parameter or allow users to select "xor-ed"/"sorted" fields as Jakub
suggested.

>Regarding the documentation, the above is available in our public 
>datasheets [2]. In the final version, I can add similar explanation in 
>the headers (kdoc) and under "Documentation/networking/" so that there 
>is a clear understanding of the algorithm.
>
>
>[1] https://www.ndsl.kaist.edu/~kyoungsoo/papers/TR-symRSS.pdf
>
>[2] E810 datasheet: 7.10.10.2 : Symmetric Hash
>
>https://www.intel.com/content/www/us/en/content-details/613875/intel-ethernet-controller-e810-datasheet.html
>

This document doesn't mention anything about implementation.

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

[Ahmed Zaki]

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

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

[Ahmed Zaki]

On 2023-08-24 12:36, Saeed Mahameed wrote:
> On 24 Aug 07:14, Ahmed Zaki wrote:
>>
>> On 2023-08-23 13:45, Saeed Mahameed wrote:
>>> On 23 Aug 10:48, 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.
>>>>
>
> ...
>
>>>
>>> What is the expectation of the symmetric toeplitz hash, how do you 
>>> achieve
>>> that? by sorting packet fields? which fields?
>>>
>>> Can you please provide a link to documentation/spec?
>>> We should make sure all vendors agree on implementation and 
>>> expectation of
>>> the symmetric hash function.
>>
>> The way the Intel NICs are achieving this hash symmetry is by XORing 
>> the source and destination values of the IP and L4 ports and then 
>> feeding these values to the regular Toeplitz (in-tree) hash algorithm.
>>
>> For example, for UDP/IPv4, the input fields for the Toeplitz hash 
>> would be:
>>
>> (SRC_IP, DST_IP, SRC_PORT,  DST_PORT)
>>
>
> So you mangle the input. This is different than the paper you
> referenced below which doesn't change the input but it modifies the RSS
> algorithm and uses a special hash key.
>
>> If symmetric Toeplitz is set, the NIC XOR the src and dst fields:
>>
>> (SRC_IP^DST_IP ,  SRC_IP^DST_IP, SRC_PORT^DST_PORT, SRC_PORT^DST_PORT)
>>
>> This way, the output hash would be the same for both flow directions. 
>> Same is applicable for IPv6, TCP and SCTP.
>>
>
> I understand the motivation, I just want to make sure the 
> interpretation is
> clear, I agree with Jakub, we should use a clear name for the ethtool
> parameter or allow users to select "xor-ed"/"sorted" fields as Jakub
> suggested.
>> Regarding the documentation, the above is available in our public 
>> datasheets [2]. In the final version, I can add similar explanation 
>> in the headers (kdoc) and under "Documentation/networking/" so that 
>> there is a clear understanding of the algorithm.
>>
>>
>> [1] https://www.ndsl.kaist.edu/~kyoungsoo/papers/TR-symRSS.pdf
>>
>> [2] E810 datasheet: 7.10.10.2 : Symmetric Hash
>>
>> https://www.intel.com/content/www/us/en/content-details/613875/intel-ethernet-controller-e810-datasheet.html 
>>
>>
>
> This document doesn't mention anything about implementation.


It has all the info regarding which fields are XOR'd using which 
registers and so on. The hash algorithm itself is the standard Toeplitz, 
also on section 7.10.10.2.

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

[Willem de Bruijn]

Ahmed Zaki wrote:
> 
> On 2023-08-24 12:36, Saeed Mahameed wrote:
> > On 24 Aug 07:14, Ahmed Zaki wrote:
> >>
> >> On 2023-08-23 13:45, Saeed Mahameed wrote:
> >>> On 23 Aug 10:48, 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.
> >>>>
> >
> > ...
> >
> >>>
> >>> What is the expectation of the symmetric toeplitz hash, how do you 
> >>> achieve
> >>> that? by sorting packet fields? which fields?
> >>>
> >>> Can you please provide a link to documentation/spec?
> >>> We should make sure all vendors agree on implementation and 
> >>> expectation of
> >>> the symmetric hash function.
> >>
> >> The way the Intel NICs are achieving this hash symmetry is by XORing 
> >> the source and destination values of the IP and L4 ports and then 
> >> feeding these values to the regular Toeplitz (in-tree) hash algorithm.
> >>
> >> For example, for UDP/IPv4, the input fields for the Toeplitz hash 
> >> would be:
> >>
> >> (SRC_IP, DST_IP, SRC_PORT,  DST_PORT)
> >>
> >
> > So you mangle the input. This is different than the paper you
> > referenced below which doesn't change the input but it modifies the RSS
> > algorithm and uses a special hash key.
> >
> >> If symmetric Toeplitz is set, the NIC XOR the src and dst fields:
> >>
> >> (SRC_IP^DST_IP ,  SRC_IP^DST_IP, SRC_PORT^DST_PORT, SRC_PORT^DST_PORT)
> >>
> >> This way, the output hash would be the same for both flow directions. 
> >> Same is applicable for IPv6, TCP and SCTP.
> >>
> >
> > I understand the motivation, I just want to make sure the 
> > interpretation is
> > clear, I agree with Jakub, we should use a clear name for the ethtool
> > parameter or allow users to select "xor-ed"/"sorted" fields as Jakub
> > suggested.
> >> Regarding the documentation, the above is available in our public 
> >> datasheets [2]. In the final version, I can add similar explanation 
> >> in the headers (kdoc) and under "Documentation/networking/" so that 
> >> there is a clear understanding of the algorithm.

Please do define the behavior.

When I hear symmetric Toeplitz, my initial assumption was also
sorted fields, as implemented in __flow_hash_consistentify.

If this is something else, agreed that that is good to make
crystal clear in name and somewhere in the kernel Documentation.
xor-symmetric hash?

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

[Jakub Kicinski]

On Thu, 24 Aug 2023 16:55:40 -0600 Ahmed Zaki wrote:
> 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'm just trying to help, if you want a single knob you'd need to add
new fields to the API and the RXFH API is not netlink-ified.

Using hashing algo for configuring fields feels like a dirty hack.

> 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?

Yes, the validation needs to be part of the same series. But the
semantics of selecting only src or dst need to be established, too.
You said you feed dst ^ src into the hashing twice - why?

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

[Ahmed Zaki]

On 2023-08-24 18:43, Jakub Kicinski wrote:
> On Thu, 24 Aug 2023 16:55:40 -0600 Ahmed Zaki wrote:
>> 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'm just trying to help, if you want a single knob you'd need to add
> new fields to the API and the RXFH API is not netlink-ified.
>
> Using hashing algo for configuring fields feels like a dirty hack.

Ok. Another way to add a single knob is to a flag in "struct 
ethtool_rxfh" (there are still some reserved bytes) and then:

ethtool -X eth0 --symmetric hfunc toeplitz

This will also allow drivers/NICs to implement this as they wish (XOR, 
sorted, ..etc). Better ?


>
>> 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?
> Yes, the validation needs to be part of the same series. But the
> semantics of selecting only src or dst need to be established, too.
> You said you feed dst ^ src into the hashing twice - why?

To maintain the same input length (same as the regular Toeplitz input) 
to the hash H/W block

length(src_ip , dst_ip, src_port, dst_port)  = length(src_ip ^ dst_ip , 
src_ip ^ dst_ip, src_port ^ dst_port, src_port ^ dst_port)

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

[Ahmed Zaki]

On 2023-08-24 17:30, Willem de Bruijn wrote:
> Ahmed Zaki wrote:
>> On 2023-08-24 12:36, Saeed Mahameed wrote:
>>> On 24 Aug 07:14, Ahmed Zaki wrote:
>>>> On 2023-08-23 13:45, Saeed Mahameed wrote:
>>>>> On 23 Aug 10:48, 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.
>>>>>>
>>> ...
>>>
>>>>> What is the expectation of the symmetric toeplitz hash, how do you
>>>>> achieve
>>>>> that? by sorting packet fields? which fields?
>>>>>
>>>>> Can you please provide a link to documentation/spec?
>>>>> We should make sure all vendors agree on implementation and
>>>>> expectation of
>>>>> the symmetric hash function.
>>>> The way the Intel NICs are achieving this hash symmetry is by XORing
>>>> the source and destination values of the IP and L4 ports and then
>>>> feeding these values to the regular Toeplitz (in-tree) hash algorithm.
>>>>
>>>> For example, for UDP/IPv4, the input fields for the Toeplitz hash
>>>> would be:
>>>>
>>>> (SRC_IP, DST_IP, SRC_PORT,  DST_PORT)
>>>>
>>> So you mangle the input. This is different than the paper you
>>> referenced below which doesn't change the input but it modifies the RSS
>>> algorithm and uses a special hash key.
>>>
>>>> If symmetric Toeplitz is set, the NIC XOR the src and dst fields:
>>>>
>>>> (SRC_IP^DST_IP ,  SRC_IP^DST_IP, SRC_PORT^DST_PORT, SRC_PORT^DST_PORT)
>>>>
>>>> This way, the output hash would be the same for both flow directions.
>>>> Same is applicable for IPv6, TCP and SCTP.
>>>>
>>> I understand the motivation, I just want to make sure the
>>> interpretation is
>>> clear, I agree with Jakub, we should use a clear name for the ethtool
>>> parameter or allow users to select "xor-ed"/"sorted" fields as Jakub
>>> suggested.
>>>> Regarding the documentation, the above is available in our public
>>>> datasheets [2]. In the final version, I can add similar explanation
>>>> in the headers (kdoc) and under "Documentation/networking/" so that
>>>> there is a clear understanding of the algorithm.
> Please do define the behavior.
>
> When I hear symmetric Toeplitz, my initial assumption was also
> sorted fields, as implemented in __flow_hash_consistentify.
>
> If this is something else, agreed that that is good to make
> crystal clear in name and somewhere in the kernel Documentation.
> xor-symmetric hash?


Thanks, I was wondering why everyone was assuming "sorted" fileds.

If we go with an a new algorithm (-X hfunc) I agree we should name it 
"xor-symmetric".

I also just suggested to Jakub to use a flag instead of a new algorithm, 
since the underlying algorithm is really just the regular Toeplitz.

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

[Jakub Kicinski]

On Fri, 25 Aug 2023 14:46:42 -0600 Ahmed Zaki wrote:
> > I'm just trying to help, if you want a single knob you'd need to add
> > new fields to the API and the RXFH API is not netlink-ified.
> >
> > Using hashing algo for configuring fields feels like a dirty hack.  
> 
> Ok. Another way to add a single knob is to a flag in "struct 
> ethtool_rxfh" (there are still some reserved bytes) and then:

Sorry we do have ETHTOOL_MSG_RSS_GET. It just doesn't cover the flow
config now. But you can add the new field there without a problem.

> ethtool -X eth0 --symmetric hfunc toeplitz
> 
> This will also allow drivers/NICs to implement this as they wish (XOR, 
> sorted, ..etc). Better ?

We should specify the fields, I reckon, something like:

ethtool -X eth0 --symmetric sdfn hfunc toeplitz

So that the driver can make sure the user expects symmetry on fields
the device supports.

> >> 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?  
> > Yes, the validation needs to be part of the same series. But the
> > semantics of selecting only src or dst need to be established, too.
> > You said you feed dst ^ src into the hashing twice - why?  
> 
> To maintain the same input length (same as the regular Toeplitz input) 
> to the hash H/W block

But that's a choice, right? We're configuring the input we could as
well choose to make it shorter? v4 and v6 use the same key with
different input lengths, right?

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

[Ahmed Zaki]

On 2023-08-25 18:49, Jakub Kicinski wrote:
> On Fri, 25 Aug 2023 14:46:42 -0600 Ahmed Zaki wrote:
>>> I'm just trying to help, if you want a single knob you'd need to add
>>> new fields to the API and the RXFH API is not netlink-ified.
>>>
>>> Using hashing algo for configuring fields feels like a dirty hack.
>> Ok. Another way to add a single knob is to a flag in "struct
>> ethtool_rxfh" (there are still some reserved bytes) and then:
> Sorry we do have ETHTOOL_MSG_RSS_GET. It just doesn't cover the flow
> config now. But you can add the new field there without a problem.
>
>> ethtool -X eth0 --symmetric hfunc toeplitz
>>
>> This will also allow drivers/NICs to implement this as they wish (XOR,
>> sorted, ..etc). Better ?
> We should specify the fields, I reckon, something like:
>
> ethtool -X eth0 --symmetric sdfn hfunc toeplitz
>
> So that the driver can make sure the user expects symmetry on fields
> the device supports.

Seems fair. I will prepare this and the per-flow based config code 
("-U|-N") and re-send.


>
>>>> 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?
>>> Yes, the validation needs to be part of the same series. But the
>>> semantics of selecting only src or dst need to be established, too.
>>> You said you feed dst ^ src into the hashing twice - why?
>> To maintain the same input length (same as the regular Toeplitz input)
>> to the hash H/W block
> But that's a choice, right? We're configuring the input we could as
> well choose to make it shorter? v4 and v6 use the same key with
> different input lengths, right?

Correct. All RSS fields' offsets and lengths are configurable. The 
example I gave before was from the datasheet, but it seems we can feed 
the Xored values once.

Thanks,