[PATCH net-next 0/2] ethtool, net/mlx4_en: RSS hash function selection

[PATCH net-next 0/2] ethtool, net/mlx4_en: RSS hash function selection

[Amir Vadai]

Hi,

This patchset by Eyal adds support in set/get of RSS hash function. Current
supported functions are Toeplitz and XOR. The API is design to enable adding
new hash functions without breaking backward compatibility.
If a driver want to use this API need to implement [gs]et_rxfh_func() callbacks
in ethtool_ops.
Userspace patch will be sent after API is available in kernel.

The patchset was applied and tested over commit 30349bd ("net: phy: spi_ks8995:
remove sysfs bin file by registered attribute")

Amir

Eyal Perry (2):
  ethtool: Support for configurable RSS hash function
  net/mlx4_en: Support for configurable RSS hash function

 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 40 +++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  9 +++++
 drivers/net/ethernet/mellanox/mlx4/en_rx.c      | 11 ++++--
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  3 +-
 include/linux/ethtool.h                         | 28 +++++++++++++
 include/uapi/linux/ethtool.h                    |  6 ++-
 net/core/ethtool.c                              | 52 +++++++++++++++++--------
 7 files changed, 127 insertions(+), 22 deletions(-)

-- 
1.8.3.4

[PATCH net-next 1/2] ethtool: Support for configurable RSS hash function

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

This patch adds an RSS hash functions string-set, and two
ethtool-options for set/get current RSS hash function. User-kernel API is done
through the new hfunc mask field in the ethtool_rxfh struct. A bit set
in the hfunc is corresponding to an index in the string-set.

Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 include/linux/ethtool.h      | 28 ++++++++++++++++++++++++
 include/uapi/linux/ethtool.h |  6 ++++-
 net/core/ethtool.c           | 52 ++++++++++++++++++++++++++++++--------------
 3 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index c1a2d60..61003b1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -59,6 +59,29 @@ enum ethtool_phys_id_state {
 	ETHTOOL_ID_OFF
 };
 
+enum {
+	RSS_HASH_TOP_BIT, /* Configurable RSS hash function - Toeplitz */
+	RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
+
+	/*
+	 * Add your fresh new hash function bits above and remember to update
+	 * rss_hash_func_strings[] below
+	 */
+	RSS_HASH_FUNCS_COUNT
+};
+
+#define __RSS_HASH_BIT(bit)	((u32)1 << (bit))
+#define __RSS_HASH(name)	 __RSS_HASH_BIT(RSS_HASH_##name##_BIT)
+
+#define RSS_HASH_TOP		__RSS_HASH(TOP)
+#define RSS_HASH_XOR		__RSS_HASH(XOR)
+
+static const char
+rss_hash_func_strings[RSS_HASH_FUNCS_COUNT][ETH_GSTRING_LEN] = {
+	[RSS_HASH_TOP_BIT] =     "toeplitz",
+	[RSS_HASH_XOR_BIT] =     "xor",
+};
+
 struct net_device;
 
 /* Some generic methods drivers may use in their ethtool_ops */
@@ -158,6 +181,9 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
  *	Returns zero if not supported for this specific device.
  * @get_rxfh_indir_size: Get the size of the RX flow hash indirection table.
  *	Returns zero if not supported for this specific device.
+ * @get_rxfh_func: Get the hardware RX flow hash function.
+ * @set_rxfh_func: Set the hardware RX flow hash function. Returns a negative
+ *	error code or zero.
  * @get_rxfh: Get the contents of the RX flow hash indirection table and hash
  *	key.
  *	Will only be called if one or both of @get_rxfh_indir_size and
@@ -241,6 +267,8 @@ struct ethtool_ops {
 	int	(*reset)(struct net_device *, u32 *);
 	u32	(*get_rxfh_key_size)(struct net_device *);
 	u32	(*get_rxfh_indir_size)(struct net_device *);
+	u32	(*get_rxfh_func)(struct net_device *);
+	int	(*set_rxfh_func)(struct net_device *, u32);
 	int	(*get_rxfh)(struct net_device *, u32 *indir, u8 *key);
 	int	(*set_rxfh)(struct net_device *, const u32 *indir,
 			    const u8 *key);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index eb2095b..eb91da4 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -534,6 +534,7 @@ struct ethtool_pauseparam {
  * @ETH_SS_NTUPLE_FILTERS: Previously used with %ETHTOOL_GRXNTUPLE;
  *	now deprecated
  * @ETH_SS_FEATURES: Device feature names
+ * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
  */
 enum ethtool_stringset {
 	ETH_SS_TEST		= 0,
@@ -541,6 +542,7 @@ enum ethtool_stringset {
 	ETH_SS_PRIV_FLAGS,
 	ETH_SS_NTUPLE_FILTERS,
 	ETH_SS_FEATURES,
+	ETH_SS_RSS_HASH_FUNCS,
 };
 
 /**
@@ -900,7 +902,9 @@ struct ethtool_rxfh {
 	__u32	rss_context;
 	__u32   indir_size;
 	__u32   key_size;
-	__u32	rsvd[2];
+	__u8	hfunc;
+	__u8	rsvd8[3];
+	__u32	rsvd32;
 	__u32   rss_config[0];
 };
 #define ETH_RXFH_INDIR_NO_CHANGE	0xffffffff
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 06dfb29..4791c17 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -185,6 +185,9 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
 	if (sset == ETH_SS_FEATURES)
 		return ARRAY_SIZE(netdev_features_strings);
 
+	if (sset == ETH_SS_RSS_HASH_FUNCS)
+		return ARRAY_SIZE(rss_hash_func_strings);
+
 	if (ops->get_sset_count && ops->get_strings)
 		return ops->get_sset_count(dev, sset);
 	else
@@ -199,6 +202,9 @@ static void __ethtool_get_strings(struct net_device *dev,
 	if (stringset == ETH_SS_FEATURES)
 		memcpy(data, netdev_features_strings,
 			sizeof(netdev_features_strings));
+	else if (stringset == ETH_SS_RSS_HASH_FUNCS)
+		memcpy(data, rss_hash_func_strings,
+		       sizeof(rss_hash_func_strings));
 	else
 		/* ops->get_strings is valid because checked earlier */
 		ops->get_strings(dev, stringset, data);
@@ -682,7 +688,7 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	int ret;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	u32 user_indir_size, user_key_size;
-	u32 dev_indir_size = 0, dev_key_size = 0;
+	u32 dev_indir_size = 0, dev_key_size = 0, dev_hfunc = 0;
 	struct ethtool_rxfh rxfh;
 	u32 total_size;
 	u32 indir_bytes;
@@ -690,17 +696,18 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	u8 *hkey = NULL;
 	u8 *rss_config;
 
-	if (!(dev->ethtool_ops->get_rxfh_indir_size ||
-	      dev->ethtool_ops->get_rxfh_key_size) ||
-	      !dev->ethtool_ops->get_rxfh)
+	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size ||
+	      ops->get_rxfh_func) || !ops->get_rxfh)
 		return -EOPNOTSUPP;
 
+	if (ops->get_rxfh_func)
+		dev_hfunc = ops->get_rxfh_func(dev);
 	if (ops->get_rxfh_indir_size)
 		dev_indir_size = ops->get_rxfh_indir_size(dev);
 	if (ops->get_rxfh_key_size)
 		dev_key_size = ops->get_rxfh_key_size(dev);
 
-	if ((dev_key_size + dev_indir_size) == 0)
+	if ((dev_key_size + dev_indir_size + dev_hfunc) == 0)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&rxfh, useraddr, sizeof(rxfh)))
@@ -709,17 +716,19 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	user_key_size = rxfh.key_size;
 
 	/* Check that reserved fields are 0 for now */
-	if (rxfh.rss_context || rxfh.rsvd[0] || rxfh.rsvd[1])
+	if (rxfh.rss_context || rxfh.rsvd8[0] || rxfh.rsvd8[1] ||
+	    rxfh.rsvd8[2] || rxfh.rsvd32)
 		return -EINVAL;
 
 	rxfh.indir_size = dev_indir_size;
 	rxfh.key_size = dev_key_size;
+	rxfh.hfunc = dev_hfunc;
 	if (copy_to_user(useraddr, &rxfh, sizeof(rxfh)))
 		return -EFAULT;
 
-	/* If the user buffer size is 0, this is just a query for the
-	 * device table size and key size.  Otherwise, if the User size is
-	 * not equal to device table size or key size it's an error.
+	/* If the user buffer size is 0, this is just a query for the device
+	 * hash function, table size and key size.  Otherwise, if the User size
+	 * is not equal to device table size or key size it's an error.
 	 */
 	if (!user_indir_size && !user_key_size)
 		return 0;
@@ -760,32 +769,43 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	struct ethtool_rxnfc rx_rings;
 	struct ethtool_rxfh rxfh;
-	u32 dev_indir_size = 0, dev_key_size = 0, i;
+	u32 dev_indir_size = 0, dev_key_size = 0, dev_hfunc = 0, i;
 	u32 *indir = NULL, indir_bytes = 0;
 	u8 *hkey = NULL;
 	u8 *rss_config;
 	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
 
-	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size) ||
-	    !ops->get_rxnfc || !ops->set_rxfh)
+	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size ||
+	      ops->get_rxfh_func) || !ops->get_rxnfc || !ops->set_rxfh)
 		return -EOPNOTSUPP;
 
+	if (ops->get_rxfh_func)
+		dev_hfunc = ops->get_rxfh_func(dev);
 	if (ops->get_rxfh_indir_size)
 		dev_indir_size = ops->get_rxfh_indir_size(dev);
 	if (ops->get_rxfh_key_size)
 		dev_key_size = dev->ethtool_ops->get_rxfh_key_size(dev);
-	if ((dev_key_size + dev_indir_size) == 0)
+	if ((dev_key_size + dev_indir_size + dev_hfunc) == 0)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&rxfh, useraddr, sizeof(rxfh)))
 		return -EFAULT;
 
 	/* Check that reserved fields are 0 for now */
-	if (rxfh.rss_context || rxfh.rsvd[0] || rxfh.rsvd[1])
+	if (rxfh.rss_context || rxfh.rsvd8[0] || rxfh.rsvd8[1] ||
+	    rxfh.rsvd8[2] || rxfh.rsvd32)
 		return -EINVAL;
 
+	if (rxfh.hfunc != dev_hfunc) {
+		if (!ops->set_rxfh_func)
+			return -EOPNOTSUPP;
+		ret = ops->set_rxfh_func(dev, rxfh.hfunc);
+		if (ret)
+			return ret;
+	}
+
 	/* If either indir or hash key is valid, proceed further.
-	 * It is not valid to request that both be unchanged.
+	 * Must request at least one change: indir size, hash key or function.
 	 */
 	if ((rxfh.indir_size &&
 	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
@@ -793,7 +813,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
 	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
 	     rxfh.key_size == 0))
-		return -EINVAL;
+		return rxfh.hfunc ? 0 : -EINVAL;
 
 	if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
 		indir_bytes = dev_indir_size * sizeof(indir[0]);
-- 
1.8.3.4

[PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

The ConnectX HW is capable of using one of the following hash functions:
Toeplitz and an XOR hash function.
This patch implements ethtool_ops {get,set}_rxfh_func callbacks for the
mlx4_en driver in order to query and configure the RSS hash function to
be used by the device.

Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 40 +++++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  9 ++++++
 drivers/net/ethernet/mellanox/mlx4/en_rx.c      | 11 ++++---
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  3 +-
 4 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 8ea4d5b..f33785a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -973,6 +973,44 @@ static u32 mlx4_en_get_rxfh_indir_size(struct net_device *dev)
 	return priv->rx_ring_num;
 }
 
+static u32 mlx4_en_get_rxfh_func(struct net_device *dev)
+{
+	struct mlx4_en_priv *priv = netdev_priv(dev);
+
+	return priv->rss_hash_fn;
+}
+
+static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
+{
+	struct mlx4_en_priv *priv = netdev_priv(dev);
+	struct mlx4_en_dev *mdev = priv->mdev;
+	u32 prev_rss_hash_fn = priv->rss_hash_fn;
+	int err = 0;
+
+	if (!(hfunc & priv->rss_hash_fn_caps))
+		return -EINVAL;
+
+	priv->rss_hash_fn = hfunc;
+	if (hfunc == RSS_HASH_TOP && !(dev->features & NETIF_F_RXHASH))
+		en_warn(priv,
+			"Toeplitz hash function should be used in conjunction with RX hashing for optimal performance\n");
+	if (hfunc == RSS_HASH_XOR && (dev->features & NETIF_F_RXHASH))
+		en_warn(priv,
+			"Enabling both XOR Hash function and RX Hashing can limit RPS functionality\n");
+
+	mutex_lock(&mdev->state_lock);
+	if (priv->port_up && priv->rss_hash_fn != prev_rss_hash_fn) {
+		mlx4_en_stop_port(dev, 1);
+		err = mlx4_en_start_port(dev);
+	}
+	mutex_unlock(&mdev->state_lock);
+
+	if (err)
+		en_err(priv, "Failed to restart port %d\n", priv->port);
+
+	return err;
+}
+
 static int mlx4_en_get_rxfh(struct net_device *dev, u32 *ring_index, u8 *key)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
@@ -1799,6 +1837,8 @@ const struct ethtool_ops mlx4_en_ethtool_ops = {
 	.get_rxnfc = mlx4_en_get_rxnfc,
 	.set_rxnfc = mlx4_en_set_rxnfc,
 	.get_rxfh_indir_size = mlx4_en_get_rxfh_indir_size,
+	.get_rxfh_func = mlx4_en_get_rxfh_func,
+	.set_rxfh_func = mlx4_en_set_rxfh_func,
 	.get_rxfh = mlx4_en_get_rxfh,
 	.set_rxfh = mlx4_en_set_rxfh,
 	.get_channels = mlx4_en_get_channels,
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 0efbae9..a9e96a6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2585,6 +2585,15 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 		dev->features    |= NETIF_F_GSO_UDP_TUNNEL;
 	}
 
+	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR) {
+		priv->rss_hash_fn_caps |= RSS_HASH_XOR;
+		priv->rss_hash_fn = RSS_HASH_XOR;
+	}
+	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP) {
+		priv->rss_hash_fn_caps |= RSS_HASH_TOP;
+		priv->rss_hash_fn = RSS_HASH_TOP;
+	}
+
 	mdev->pndev[port] = dev;
 
 	netif_carrier_off(dev);
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index b173a0c..41c1ff9 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct mlx4_en_priv *priv)
 	}
 
 	rss_context->flags = rss_mask;
-	rss_context->hash_fn = MLX4_RSS_HASH_TOP;
-	for (i = 0; i < 10; i++)
-		rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
-
+	if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
+		rss_context->hash_fn = MLX4_RSS_HASH_XOR;
+	} else {
+		rss_context->hash_fn = MLX4_RSS_HASH_TOP;
+		for (i = 0; i < 10; i++)
+			rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
+	}
 	err = mlx4_qp_to_ready(mdev->dev, &priv->res.mtt, &context,
 			       &rss_map->indir_qp, &rss_map->indir_state);
 	if (err)
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index ef83d12..e1c3364 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -375,7 +375,6 @@ struct mlx4_en_port_profile {
 };
 
 struct mlx4_en_profile {
-	int rss_xor;
 	int udp_rss;
 	u8 rss_mask;
 	u32 active_ports;
@@ -615,6 +614,8 @@ struct mlx4_en_priv {
 	__be16 vxlan_port;
 
 	u32 pflags;
+	u8 rss_hash_fn;
+	u8 rss_hash_fn_caps;
 };
 
 enum mlx4_en_wol {
-- 
1.8.3.4

Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

[Or Gerlitz]

On 11/5/2014 1:59 PM, Amir Vadai wrote:
> From: Eyal Perry <eyalpe@mellanox.com>
>
> The ConnectX HW is capable of using one of the following hash functions:
> Toeplitz and an XOR hash function.
> This patch implements ethtool_ops {get,set}_rxfh_func callbacks for the
> mlx4_en driver in order to query and configure the RSS hash function to
> be used by the device.
>
> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 40 +++++++++++++++++++++++++
>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  9 ++++++
>   drivers/net/ethernet/mellanox/mlx4/en_rx.c      | 11 ++++---
>   drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  3 +-
>   4 files changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 8ea4d5b..f33785a 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -973,6 +973,44 @@ static u32 mlx4_en_get_rxfh_indir_size(struct net_device *dev)
>   	return priv->rx_ring_num;
>   }
>   
> +static u32 mlx4_en_get_rxfh_func(struct net_device *dev)
> +{
> +	struct mlx4_en_priv *priv = netdev_priv(dev);
> +
> +	return priv->rss_hash_fn;
> +}
> +
> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
> +{
> +	struct mlx4_en_priv *priv = netdev_priv(dev);
> +	struct mlx4_en_dev *mdev = priv->mdev;
> +	u32 prev_rss_hash_fn = priv->rss_hash_fn;
> +	int err = 0;
> +
> +	if (!(hfunc & priv->rss_hash_fn_caps))
> +		return -EINVAL;
> +
> +	priv->rss_hash_fn = hfunc;

Seems that you are blindly setting here priv->rss_hash_fn to the 
function (xor or top) requested
by the user w.o prior checking if the firmware actually supports that 
(e.g test it against priv->rss_hash_fn_caps, right?

> +	if (hfunc == RSS_HASH_TOP && !(dev->features & NETIF_F_RXHASH))
> +		en_warn(priv,
> +			"Toeplitz hash function should be used in conjunction with RX hashing for optimal performance\n");
> +	if (hfunc == RSS_HASH_XOR && (dev->features & NETIF_F_RXHASH))
> +		en_warn(priv,
> +			"Enabling both XOR Hash function and RX Hashing can limit RPS functionality\n");
> +
> +	mutex_lock(&mdev->state_lock);
> +	if (priv->port_up && priv->rss_hash_fn != prev_rss_hash_fn) {
> +		mlx4_en_stop_port(dev, 1);
> +		err = mlx4_en_start_port(dev);
> +	}
> +	mutex_unlock(&mdev->state_lock);
> +
> +	if (err)
> +		en_err(priv, "Failed to restart port %d\n", priv->port);
> +
> +	return err;
> +}
> +
>   static int mlx4_en_get_rxfh(struct net_device *dev, u32 *ring_index, u8 *key)
>   {
>   	struct mlx4_en_priv *priv = netdev_priv(dev);
> @@ -1799,6 +1837,8 @@ const struct ethtool_ops mlx4_en_ethtool_ops = {
>   	.get_rxnfc = mlx4_en_get_rxnfc,
>   	.set_rxnfc = mlx4_en_set_rxnfc,
>   	.get_rxfh_indir_size = mlx4_en_get_rxfh_indir_size,
> +	.get_rxfh_func = mlx4_en_get_rxfh_func,
> +	.set_rxfh_func = mlx4_en_set_rxfh_func,
>   	.get_rxfh = mlx4_en_get_rxfh,
>   	.set_rxfh = mlx4_en_set_rxfh,
>   	.get_channels = mlx4_en_get_channels,
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 0efbae9..a9e96a6 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2585,6 +2585,15 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
>   		dev->features    |= NETIF_F_GSO_UDP_TUNNEL;
>   	}
>   
> +	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR) {
> +		priv->rss_hash_fn_caps |= RSS_HASH_XOR;
> +		priv->rss_hash_fn = RSS_HASH_XOR;
> +	}
> +	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP) {
> +		priv->rss_hash_fn_caps |= RSS_HASH_TOP;
> +		priv->rss_hash_fn = RSS_HASH_TOP;
> +	}
> +
>   	mdev->pndev[port] = dev;
>   
>   	netif_carrier_off(dev);
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index b173a0c..41c1ff9 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct mlx4_en_priv *priv)
>   	}
>   
>   	rss_context->flags = rss_mask;
> -	rss_context->hash_fn = MLX4_RSS_HASH_TOP;
> -	for (i = 0; i < 10; i++)
> -		rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
> -
> +	if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
> +		rss_context->hash_fn = MLX4_RSS_HASH_XOR;
> +	} else {
> +		rss_context->hash_fn = MLX4_RSS_HASH_TOP;
> +		for (i = 0; i < 10; i++)
> +			rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
> +	}

So this patch effectively changes the default from top to xor, right? is 
that intentional? if yes, spare few words on the change log.


>   	err = mlx4_qp_to_ready(mdev->dev, &priv->res.mtt, &context,
>   			       &rss_map->indir_qp, &rss_map->indir_state);
>   	if (err)
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> index ef83d12..e1c3364 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> @@ -375,7 +375,6 @@ struct mlx4_en_port_profile {
>   };
>   
>   struct mlx4_en_profile {
> -	int rss_xor;
>   	int udp_rss;
>   	u8 rss_mask;
>   	u32 active_ports;
> @@ -615,6 +614,8 @@ struct mlx4_en_priv {
>   	__be16 vxlan_port;
>   
>   	u32 pflags;
> +	u8 rss_hash_fn;
> +	u8 rss_hash_fn_caps;
>   };
>   
>   enum mlx4_en_wol {

Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

[Eyal perry]

On 11/5/2014 2:27 PM, Or Gerlitz wrote:
> On 11/5/2014 1:59 PM, Amir Vadai wrote:
>> From: Eyal Perry <eyalpe@mellanox.com>
>>
>> The ConnectX HW is capable of using one of the following hash functions:
>> Toeplitz and an XOR hash function.
>> This patch implements ethtool_ops {get,set}_rxfh_func callbacks for the
>> mlx4_en driver in order to query and configure the RSS hash function to
>> be used by the device.
>>
>> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 40
>> +++++++++++++++++++++++++
>>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  9 ++++++
>>   drivers/net/ethernet/mellanox/mlx4/en_rx.c      | 11 ++++---
>>   drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  3 +-
>>   4 files changed, 58 insertions(+), 5 deletions(-)
>>

[...]

>> +
>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>> +{
>> +    struct mlx4_en_priv *priv = netdev_priv(dev);
>> +    struct mlx4_en_dev *mdev = priv->mdev;
>> +    u32 prev_rss_hash_fn = priv->rss_hash_fn;
>> +    int err = 0;
>> +
>> +    if (!(hfunc & priv->rss_hash_fn_caps))
>> +        return -EINVAL;
>> +
>> +    priv->rss_hash_fn = hfunc;
> 
> Seems that you are blindly setting here priv->rss_hash_fn to the
> function (xor or top) requested
> by the user w.o prior checking if the firmware actually supports that
> (e.g test it against priv->rss_hash_fn_caps, right?
> 
That exactly what I do 3 lines above, priv->rss_hash_fn_caps is derived
from firmware capabilities.

[...]

>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>> mlx4_en_priv *priv)
>>       }
>>         rss_context->flags = rss_mask;
>> -    rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>> -    for (i = 0; i < 10; i++)
>> -        rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>> -
>> +    if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>> +        rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>> +    } else {
>> +        rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>> +        for (i = 0; i < 10; i++)
>> +            rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>> +    }
> 
> So this patch effectively changes the default from top to xor, right? is
> that intentional? if yes, spare few words on the change log.
> 
No, in general the default shouldn't be affected by the patch (unless
there's a bug). The default value is set in mlx4_en_init_netdev() and it
will remain Toeplitz unless firmware is supports only XOR hash function.

[...]

Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

[Or Gerlitz]

On 11/5/2014 3:05 PM, Eyal perry wrote:
> On 11/5/2014 2:27 PM, Or Gerlitz wrote:
>>> +
>>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>>> +{
>>> +    struct mlx4_en_priv *priv = netdev_priv(dev);
>>> +    struct mlx4_en_dev *mdev = priv->mdev;
>>> +    u32 prev_rss_hash_fn = priv->rss_hash_fn;
>>> +    int err = 0;
>>> +
>>> +    if (!(hfunc & priv->rss_hash_fn_caps))
>>> +        return -EINVAL;
>>> +
>>> +    priv->rss_hash_fn = hfunc;
>> Seems that you are blindly setting here priv->rss_hash_fn to the function (xor or top) requested by the user w.o prior checking if the firmware actually supports that
>> (e.g test it against priv->rss_hash_fn_caps, right?
>>
> That exactly what I do 3 lines above, priv->rss_hash_fn_caps is derived from firmware capabilities.

got it, thanks.

>
> [...]
>
>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>>> mlx4_en_priv *priv)
>>>        }
>>>          rss_context->flags = rss_mask;
>>> -    rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>> -    for (i = 0; i < 10; i++)
>>> -        rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>> -
>>> +    if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>>> +        rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>>> +    } else {
>>> +        rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>> +        for (i = 0; i < 10; i++)
>>> +            rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>> +    }
>> So this patch effectively changes the default from top to xor, right? is
>> that intentional? if yes, spare few words on the change log.
>>
> No, in general the default shouldn't be affected by the patch (unless
> there's a bug). The default value is set in mlx4_en_init_netdev() and it
> will remain Toeplitz unless firmware is supports only XOR hash function.
>

Oh, I see why I was confused and what I think need to be fixed. You are 
using both MLX4_RSS_HASH_TOP/XOR and RSS_HASH_TOP/XOR, any reason for that?

Also, priv->rss_hash_fn should equal one value, right? so this code "if 
(priv->rss_hash_fn & something)" is confusing, should be "if 
(priv->rss_hash_fn == something)"

Or.

Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

[Eyal perry]

On 11/5/2014 3:30 PM, Or Gerlitz wrote:
> On 11/5/2014 3:05 PM, Eyal perry wrote:
>> On 11/5/2014 2:27 PM, Or Gerlitz wrote:
>>>> +
>>>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>>>> +{
>>>> +    struct mlx4_en_priv *priv = netdev_priv(dev);
>>>> +    struct mlx4_en_dev *mdev = priv->mdev;
>>>> +    u32 prev_rss_hash_fn = priv->rss_hash_fn;
>>>> +    int err = 0;
>>>> +
>>>> +    if (!(hfunc & priv->rss_hash_fn_caps))
>>>> +        return -EINVAL;
>>>> +
>>>> +    priv->rss_hash_fn = hfunc;
>>> Seems that you are blindly setting here priv->rss_hash_fn to the
>>> function (xor or top) requested by the user w.o prior checking if the
>>> firmware actually supports that
>>> (e.g test it against priv->rss_hash_fn_caps, right?
>>>
>> That exactly what I do 3 lines above, priv->rss_hash_fn_caps is
>> derived from firmware capabilities.
> 
> got it, thanks.
> 
>>
>> [...]
>>
>>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>>>> mlx4_en_priv *priv)
>>>>        }
>>>>          rss_context->flags = rss_mask;
>>>> -    rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>>> -    for (i = 0; i < 10; i++)
>>>> -        rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>>> -
>>>> +    if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>>>> +        rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>>>> +    } else {
>>>> +        rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>>> +        for (i = 0; i < 10; i++)
>>>> +            rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>>> +    }
>>> So this patch effectively changes the default from top to xor, right? is
>>> that intentional? if yes, spare few words on the change log.
>>>
>> No, in general the default shouldn't be affected by the patch (unless
>> there's a bug). The default value is set in mlx4_en_init_netdev() and it
>> will remain Toeplitz unless firmware is supports only XOR hash function.
>>
> 
> Oh, I see why I was confused and what I think need to be fixed. You are
> using both MLX4_RSS_HASH_TOP/XOR and RSS_HASH_TOP/XOR, any reason for that?
Yes, RSS_HASH_TOP/XOR are bits on a generic bitmask while the
MLX4_RSS_HASH_TOP/XOR are HW specific values.
> 
> Also, priv->rss_hash_fn should equal one value, right? so this code "if
> (priv->rss_hash_fn & something)" is confusing, should be "if
> (priv->rss_hash_fn == something)"
That's right, I'll fix it for V1.
Thanks.
> 
> Or.

Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

[Or Gerlitz]

On 11/5/2014 1:59 PM, Amir Vadai wrote:
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2585,6 +2585,15 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
>   		dev->features    |= NETIF_F_GSO_UDP_TUNNEL;
>   	}
>   
> +	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR) {
> +		priv->rss_hash_fn_caps |= RSS_HASH_XOR;
> +		priv->rss_hash_fn = RSS_HASH_XOR;
> +	}
> +	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP) {
> +		priv->rss_hash_fn_caps |= RSS_HASH_TOP;
> +		priv->rss_hash_fn = RSS_HASH_TOP;
> +	}
> +

can we somehow change this code to be more clear and assign 
priv->rss_hash_fn once?

>   	mdev->pndev[port] = dev;
>   
>   	netif_carrier_off(dev);

Re: [PATCH net-next 1/2] ethtool: Support for configurable RSS hash function

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 7383 bytes --]

On Wed, 2014-11-05 at 13:59 +0200, Amir Vadai wrote:
> From: Eyal Perry <eyalpe@mellanox.com>
> 
> This patch adds an RSS hash functions string-set, and two
> ethtool-options for set/get current RSS hash function. User-kernel API is done
> through the new hfunc mask field in the ethtool_rxfh struct. A bit set
> in the hfunc is corresponding to an index in the string-set.
> 
> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>  include/linux/ethtool.h      | 28 ++++++++++++++++++++++++
>  include/uapi/linux/ethtool.h |  6 ++++-
>  net/core/ethtool.c           | 52 ++++++++++++++++++++++++++++++--------------
>  3 files changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c1a2d60..61003b1 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -59,6 +59,29 @@ enum ethtool_phys_id_state {
>  	ETHTOOL_ID_OFF
>  };
>  
> +enum {
> +	RSS_HASH_TOP_BIT, /* Configurable RSS hash function - Toeplitz */
> +	RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
> +
> +	/*
> +	 * Add your fresh new hash function bits above and remember to update
> +	 * rss_hash_func_strings[] below
> +	 */
> +	RSS_HASH_FUNCS_COUNT
> +};
> +
> +#define __RSS_HASH_BIT(bit)	((u32)1 << (bit))
> +#define __RSS_HASH(name)	 __RSS_HASH_BIT(RSS_HASH_##name##_BIT)
> +
> +#define RSS_HASH_TOP		__RSS_HASH(TOP)
> +#define RSS_HASH_XOR		__RSS_HASH(XOR)

I think #define RSS_HASH_UNKNOWN 0 might also be useful.

And I think all of these names should get an ETH_ prefix.

> +static const char
> +rss_hash_func_strings[RSS_HASH_FUNCS_COUNT][ETH_GSTRING_LEN] = {
> +	[RSS_HASH_TOP_BIT] =     "toeplitz",
> +	[RSS_HASH_XOR_BIT] =     "xor",
> +};

This belongs in net/core/ethtool.c.

>  struct net_device;
>  
>  /* Some generic methods drivers may use in their ethtool_ops */
> @@ -158,6 +181,9 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>   *	Returns zero if not supported for this specific device.
>   * @get_rxfh_indir_size: Get the size of the RX flow hash indirection table.
>   *	Returns zero if not supported for this specific device.
> + * @get_rxfh_func: Get the hardware RX flow hash function.
> + * @set_rxfh_func: Set the hardware RX flow hash function. Returns a negative
> + *	error code or zero.
>   * @get_rxfh: Get the contents of the RX flow hash indirection table and hash
>   *	key.
>   *	Will only be called if one or both of @get_rxfh_indir_size and
> @@ -241,6 +267,8 @@ struct ethtool_ops {
>  	int	(*reset)(struct net_device *, u32 *);
>  	u32	(*get_rxfh_key_size)(struct net_device *);
>  	u32	(*get_rxfh_indir_size)(struct net_device *);
> +	u32	(*get_rxfh_func)(struct net_device *);
> +	int	(*set_rxfh_func)(struct net_device *, u32);

Why not another parameter to get_rxfh/set_rxfh?  I know it's a pain to
update all the implementations, but changing algorithm potentially
changes the supported indirection table and key lengths.  They have to
be validated together.

>  	int	(*get_rxfh)(struct net_device *, u32 *indir, u8 *key);
>  	int	(*set_rxfh)(struct net_device *, const u32 *indir,
>  			    const u8 *key);
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index eb2095b..eb91da4 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -534,6 +534,7 @@ struct ethtool_pauseparam {
>   * @ETH_SS_NTUPLE_FILTERS: Previously used with %ETHTOOL_GRXNTUPLE;
>   *	now deprecated
>   * @ETH_SS_FEATURES: Device feature names
> + * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
>   */
>  enum ethtool_stringset {
>  	ETH_SS_TEST		= 0,
> @@ -541,6 +542,7 @@ enum ethtool_stringset {
>  	ETH_SS_PRIV_FLAGS,
>  	ETH_SS_NTUPLE_FILTERS,
>  	ETH_SS_FEATURES,
> +	ETH_SS_RSS_HASH_FUNCS,
>  };
>  
>  /**
> @@ -900,7 +902,9 @@ struct ethtool_rxfh {
>  	__u32	rss_context;
>  	__u32   indir_size;
>  	__u32   key_size;
> -	__u32	rsvd[2];
> +	__u8	hfunc;

Missing kernel-doc.  This needs to be very clear about what the valid
values are.

> +	__u8	rsvd8[3];
> +	__u32	rsvd32;
>  	__u32   rss_config[0];
>  };
>  #define ETH_RXFH_INDIR_NO_CHANGE	0xffffffff
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 06dfb29..4791c17 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -185,6 +185,9 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
>  	if (sset == ETH_SS_FEATURES)
>  		return ARRAY_SIZE(netdev_features_strings);
>  
> +	if (sset == ETH_SS_RSS_HASH_FUNCS)
> +		return ARRAY_SIZE(rss_hash_func_strings);
> +
>  	if (ops->get_sset_count && ops->get_strings)
>  		return ops->get_sset_count(dev, sset);
>  	else
[...]
> @@ -760,32 +769,43 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
>  	struct ethtool_rxnfc rx_rings;
>  	struct ethtool_rxfh rxfh;
> -	u32 dev_indir_size = 0, dev_key_size = 0, i;
> +	u32 dev_indir_size = 0, dev_key_size = 0, dev_hfunc = 0, i;
>  	u32 *indir = NULL, indir_bytes = 0;
>  	u8 *hkey = NULL;
>  	u8 *rss_config;
>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
>  
> -	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size) ||
> -	    !ops->get_rxnfc || !ops->set_rxfh)
> +	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size ||
> +	      ops->get_rxfh_func) || !ops->get_rxnfc || !ops->set_rxfh)
>  		return -EOPNOTSUPP;
>  
> +	if (ops->get_rxfh_func)
> +		dev_hfunc = ops->get_rxfh_func(dev);
>  	if (ops->get_rxfh_indir_size)
>  		dev_indir_size = ops->get_rxfh_indir_size(dev);
>  	if (ops->get_rxfh_key_size)
>  		dev_key_size = dev->ethtool_ops->get_rxfh_key_size(dev);
> -	if ((dev_key_size + dev_indir_size) == 0)
> +	if ((dev_key_size + dev_indir_size + dev_hfunc) == 0)
>  		return -EOPNOTSUPP;
>  
>  	if (copy_from_user(&rxfh, useraddr, sizeof(rxfh)))
>  		return -EFAULT;
>  
>  	/* Check that reserved fields are 0 for now */
> -	if (rxfh.rss_context || rxfh.rsvd[0] || rxfh.rsvd[1])
> +	if (rxfh.rss_context || rxfh.rsvd8[0] || rxfh.rsvd8[1] ||
> +	    rxfh.rsvd8[2] || rxfh.rsvd32)
>  		return -EINVAL;
>  
> +	if (rxfh.hfunc != dev_hfunc) {
> +		if (!ops->set_rxfh_func)
> +			return -EOPNOTSUPP;
> +		ret = ops->set_rxfh_func(dev, rxfh.hfunc);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* If either indir or hash key is valid, proceed further.
> -	 * It is not valid to request that both be unchanged.
> +	 * Must request at least one change: indir size, hash key or function.
>  	 */
>  	if ((rxfh.indir_size &&
>  	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
> @@ -793,7 +813,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
>  	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>  	     rxfh.key_size == 0))
> -		return -EINVAL;
> +		return rxfh.hfunc ? 0 : -EINVAL;

Shouldn't the condition be rxfh.hfunc != dev_hfunc ?

Ben.

>  	if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
>  		indir_bytes = dev_indir_size * sizeof(indir[0]);

-- 
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net-next 1/2] ethtool: Support for configurable RSS hash function

[Eyal perry]

On 11/5/2014 11:51 PM, Ben Hutchings wrote:
> On Wed, 2014-11-05 at 13:59 +0200, Amir Vadai wrote:
>> From: Eyal Perry <eyalpe@mellanox.com>
>>
>> This patch adds an RSS hash functions string-set, and two
>> ethtool-options for set/get current RSS hash function. User-kernel API is done
>> through the new hfunc mask field in the ethtool_rxfh struct. A bit set
>> in the hfunc is corresponding to an index in the string-set.
>>
>> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>  include/linux/ethtool.h      | 28 ++++++++++++++++++++++++
>>  include/uapi/linux/ethtool.h |  6 ++++-
>>  net/core/ethtool.c           | 52 ++++++++++++++++++++++++++++++--------------
>>  3 files changed, 69 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index c1a2d60..61003b1 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -59,6 +59,29 @@ enum ethtool_phys_id_state {
>>  	ETHTOOL_ID_OFF
>>  };
>>  
>> +enum {
>> +	RSS_HASH_TOP_BIT, /* Configurable RSS hash function - Toeplitz */
>> +	RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
>> +
>> +	/*
>> +	 * Add your fresh new hash function bits above and remember to update
>> +	 * rss_hash_func_strings[] below
>> +	 */
>> +	RSS_HASH_FUNCS_COUNT
>> +};
>> +
>> +#define __RSS_HASH_BIT(bit)	((u32)1 << (bit))
>> +#define __RSS_HASH(name)	 __RSS_HASH_BIT(RSS_HASH_##name##_BIT)
>> +
>> +#define RSS_HASH_TOP		__RSS_HASH(TOP)
>> +#define RSS_HASH_XOR		__RSS_HASH(XOR)
> 
> I think #define RSS_HASH_UNKNOWN 0 might also be useful.
> 
> And I think all of these names should get an ETH_ prefix.
> 
I'll add it in V1.
>> +static const char
>> +rss_hash_func_strings[RSS_HASH_FUNCS_COUNT][ETH_GSTRING_LEN] = {
>> +	[RSS_HASH_TOP_BIT] =     "toeplitz",
>> +	[RSS_HASH_XOR_BIT] =     "xor",
>> +};
> 
> This belongs in net/core/ethtool.c.
> 
I agree, will be fixed in V1.
>>  struct net_device;
>>  
>>  /* Some generic methods drivers may use in their ethtool_ops */
>> @@ -158,6 +181,9 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>>   *	Returns zero if not supported for this specific device.
>>   * @get_rxfh_indir_size: Get the size of the RX flow hash indirection table.
>>   *	Returns zero if not supported for this specific device.
>> + * @get_rxfh_func: Get the hardware RX flow hash function.
>> + * @set_rxfh_func: Set the hardware RX flow hash function. Returns a negative
>> + *	error code or zero.
>>   * @get_rxfh: Get the contents of the RX flow hash indirection table and hash
>>   *	key.
>>   *	Will only be called if one or both of @get_rxfh_indir_size and
>> @@ -241,6 +267,8 @@ struct ethtool_ops {
>>  	int	(*reset)(struct net_device *, u32 *);
>>  	u32	(*get_rxfh_key_size)(struct net_device *);
>>  	u32	(*get_rxfh_indir_size)(struct net_device *);
>> +	u32	(*get_rxfh_func)(struct net_device *);
>> +	int	(*set_rxfh_func)(struct net_device *, u32);
> 
> Why not another parameter to get_rxfh/set_rxfh?  I know it's a pain to
> update all the implementations, but changing algorithm potentially
> changes the supported indirection table and key lengths.  They have to
> be validated together.
> 
OK, I'll do it for V1.
>>  	int	(*get_rxfh)(struct net_device *, u32 *indir, u8 *key);
>>  	int	(*set_rxfh)(struct net_device *, const u32 *indir,
>>  			    const u8 *key);
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index eb2095b..eb91da4 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -534,6 +534,7 @@ struct ethtool_pauseparam {
>>   * @ETH_SS_NTUPLE_FILTERS: Previously used with %ETHTOOL_GRXNTUPLE;
>>   *	now deprecated
>>   * @ETH_SS_FEATURES: Device feature names
>> + * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
>>   */
>>  enum ethtool_stringset {
>>  	ETH_SS_TEST		= 0,
>> @@ -541,6 +542,7 @@ enum ethtool_stringset {
>>  	ETH_SS_PRIV_FLAGS,
>>  	ETH_SS_NTUPLE_FILTERS,
>>  	ETH_SS_FEATURES,
>> +	ETH_SS_RSS_HASH_FUNCS,
>>  };
>>  
>>  /**
>> @@ -900,7 +902,9 @@ struct ethtool_rxfh {
>>  	__u32	rss_context;
>>  	__u32   indir_size;
>>  	__u32   key_size;
>> -	__u32	rsvd[2];
>> +	__u8	hfunc;
> 
> Missing kernel-doc.  This needs to be very clear about what the valid
> values are.
> 
I'll add it in V1.
>> +	__u8	rsvd8[3];
>> +	__u32	rsvd32;
>>  	__u32   rss_config[0];
>>  };
>>  #define ETH_RXFH_INDIR_NO_CHANGE	0xffffffff
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 06dfb29..4791c17 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -185,6 +185,9 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
>>  	if (sset == ETH_SS_FEATURES)
>>  		return ARRAY_SIZE(netdev_features_strings);
>>  
>> +	if (sset == ETH_SS_RSS_HASH_FUNCS)
>> +		return ARRAY_SIZE(rss_hash_func_strings);
>> +
>>  	if (ops->get_sset_count && ops->get_strings)
>>  		return ops->get_sset_count(dev, sset);
>>  	else
> [...]
>> @@ -760,32 +769,43 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>>  	const struct ethtool_ops *ops = dev->ethtool_ops;
>>  	struct ethtool_rxnfc rx_rings;
>>  	struct ethtool_rxfh rxfh;
>> -	u32 dev_indir_size = 0, dev_key_size = 0, i;
>> +	u32 dev_indir_size = 0, dev_key_size = 0, dev_hfunc = 0, i;
>>  	u32 *indir = NULL, indir_bytes = 0;
>>  	u8 *hkey = NULL;
>>  	u8 *rss_config;
>>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
>>  
>> -	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size) ||
>> -	    !ops->get_rxnfc || !ops->set_rxfh)
>> +	if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size ||
>> +	      ops->get_rxfh_func) || !ops->get_rxnfc || !ops->set_rxfh)
>>  		return -EOPNOTSUPP;
>>  
>> +	if (ops->get_rxfh_func)
>> +		dev_hfunc = ops->get_rxfh_func(dev);
>>  	if (ops->get_rxfh_indir_size)
>>  		dev_indir_size = ops->get_rxfh_indir_size(dev);
>>  	if (ops->get_rxfh_key_size)
>>  		dev_key_size = dev->ethtool_ops->get_rxfh_key_size(dev);
>> -	if ((dev_key_size + dev_indir_size) == 0)
>> +	if ((dev_key_size + dev_indir_size + dev_hfunc) == 0)
>>  		return -EOPNOTSUPP;
>>  
>>  	if (copy_from_user(&rxfh, useraddr, sizeof(rxfh)))
>>  		return -EFAULT;
>>  
>>  	/* Check that reserved fields are 0 for now */
>> -	if (rxfh.rss_context || rxfh.rsvd[0] || rxfh.rsvd[1])
>> +	if (rxfh.rss_context || rxfh.rsvd8[0] || rxfh.rsvd8[1] ||
>> +	    rxfh.rsvd8[2] || rxfh.rsvd32)
>>  		return -EINVAL;
>>  
>> +	if (rxfh.hfunc != dev_hfunc) {
>> +		if (!ops->set_rxfh_func)
>> +			return -EOPNOTSUPP;
>> +		ret = ops->set_rxfh_func(dev, rxfh.hfunc);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	/* If either indir or hash key is valid, proceed further.
>> -	 * It is not valid to request that both be unchanged.
>> +	 * Must request at least one change: indir size, hash key or function.
>>  	 */
>>  	if ((rxfh.indir_size &&
>>  	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
>> @@ -793,7 +813,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>>  	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
>>  	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>>  	     rxfh.key_size == 0))
>> -		return -EINVAL;
>> +		return rxfh.hfunc ? 0 : -EINVAL;
> 
> Shouldn't the condition be rxfh.hfunc != dev_hfunc ?
> 
> Ben.
>
On current implementation it's not necessary since we have already
checked that above. However, according to your suggestion, after I'll
remove the set/get_rxfh_func() callbacks I wouldn't be able to check
that condition at this point of the code.

Best Regards,
Eyal.