[dpdk-dev] [PATCH 0/3] Virtio PMD RSS support & RSS fixes

[dpdk-dev] [PATCH 0/3] Virtio PMD RSS support & RSS fixes

[Maxime Coquelin]

This series is mainly adding support for RSS to Virtio PMD
driver. The two last patches are fixing an issue in testpmd
that prevents to configure the RSS hash types, and fix
an issue spotted in the mlx5 driver while looking for
inspiration.

The first motivation for this series is to eventually
support RSS down to the Vhost-user library, so that OVS can
benefit from it. But it will be also useful with vDPA
devices in the future.

Regarding the testing, I have tested it with qemu v5.2 from
Fedora 34. Since libvirt does not support yet enabling RSS
feature in the Qemu virtio-net device, and this feature is
disabled by default, the tester can either rebuild the qemu
package to enable it by default or use the qemu cmdline to
do the same.

The tester can use testpmd in icmpecho mode in the guest
and scapy on the host to inject random traffic on the tap
interface, e.g.:
sendp(Ether(src=RandMAC()) / IP(src=RandIP(), dst='192.168.123.9') / UDP(sport=RandShort(), dport=RandShort()), loop=True, iface='vnet7')

Then it can play with RSS config in testpmd to change the
reta, or hash type and see traffic being steered
accordingly by checking the Rx xstats.

Ferruh, Andrew, I have updated the driver feature list,
but I have one doubt about whether I should set RSS hash,
since the driver don't support hash reporting yet, but
the device performs RSS hashing for queue selection. WDYT?

Maxime Coquelin (3):
  net/virtio: add initial RSS support
  app/testpmd: fix RSS hash type update
  net/mlx5: Fix RSS RETA update

 app/test-pmd/config.c                  |   8 +-
 doc/guides/nics/features/virtio.ini    |   2 +
 doc/guides/nics/virtio.rst             |   3 +
 doc/guides/rel_notes/release_21_11.rst |   5 +
 drivers/net/mlx5/mlx5_rss.c            |   2 +-
 drivers/net/virtio/virtio.h            |  31 ++-
 drivers/net/virtio/virtio_ethdev.c     | 367 ++++++++++++++++++++++++-
 drivers/net/virtio/virtio_ethdev.h     |   3 +-
 drivers/net/virtio/virtqueue.h         |  21 ++
 9 files changed, 429 insertions(+), 13 deletions(-)

-- 
2.31.1

[dpdk-dev] [PATCH 1/3] net/virtio: add initial RSS support

[Maxime Coquelin]

This patch adds RSS support to the Virtio PMD. It provides
the capability to update the hash key, hash types and reta
table on the fly (without needing to stop/start the
device). However, the key length and the number of reta
entries are fixed to 40B and 128 entries respectively. This
is done in order to simplify the design, but may be
revisited later as the Virtio spec provides this
flexibility.

Note that only VIRTIO_NET_F_RSS support is implemented,
VIRTIO_NET_F_HASH_REPORT, which would enable reporting the
packet RSS hash calculated by the device into mbuf.rss, is
not yet supported.

Regarding the default RSS configuration, it has been
chosen to use the default Intel ixgbe key as default key,
and default reta is a simple modulo between the hash and
the number of Rx queues.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/nics/features/virtio.ini    |   2 +
 doc/guides/nics/virtio.rst             |   3 +
 doc/guides/rel_notes/release_21_11.rst |   5 +
 drivers/net/virtio/virtio.h            |  31 ++-
 drivers/net/virtio/virtio_ethdev.c     | 367 ++++++++++++++++++++++++-
 drivers/net/virtio/virtio_ethdev.h     |   3 +-
 drivers/net/virtio/virtqueue.h         |  21 ++
 7 files changed, 426 insertions(+), 6 deletions(-)

diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
index 48f6f393b1..883dd1426c 100644
--- a/doc/guides/nics/features/virtio.ini
+++ b/doc/guides/nics/features/virtio.ini
@@ -14,6 +14,8 @@ Promiscuous mode     = Y
 Allmulticast mode    = Y
 Unicast MAC filter   = Y
 Multicast MAC filter = Y
+RSS key update       = Y
+RSS reta update      = Y
 VLAN filter          = Y
 Basic stats          = Y
 Stats per queue      = Y
diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
index 82ce7399ce..6e451cd67c 100644
--- a/doc/guides/nics/virtio.rst
+++ b/doc/guides/nics/virtio.rst
@@ -73,6 +73,9 @@ In this release, the virtio PMD driver provides the basic functionality of packe
 
 *   Virtio supports using port IO to get PCI resource when UIO module is not available.
 
+*   Virtio supports RSS Rx mode with 40B configurable hash key len, 128
+    configurable reta entries and configurable hash types.
+
 Prerequisites
 -------------
 
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index d707a554ef..21d2b09838 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -55,6 +55,11 @@ New Features
      Also, make sure to start the actual text at the margin.
      =======================================================
 
+   * **Added initial RSS support to Virtio PMD.**
+
+     Initial support for RSS receive mode has been added to the Virtio PMD,
+     with the capability for the application to configure the hash Key, the
+     reta and the hash types. Virtio hash reporting is yet to be added.
 
 Removed Items
 -------------
diff --git a/drivers/net/virtio/virtio.h b/drivers/net/virtio/virtio.h
index 525e2dad4c..b4f21dc0c7 100644
--- a/drivers/net/virtio/virtio.h
+++ b/drivers/net/virtio/virtio.h
@@ -30,6 +30,7 @@
 #define VIRTIO_NET_F_GUEST_ANNOUNCE 21	/* Guest can announce device on the network */
 #define VIRTIO_NET_F_MQ		22	/* Device supports Receive Flow Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
+#define VIRTIO_NET_F_RSS	60	/* RSS supported */
 
 /*
  * Do we get callbacks when the ring is completely used,
@@ -100,6 +101,29 @@
  */
 #define VIRTIO_MAX_INDIRECT ((int)(rte_mem_page_size() / 16))
 
+/*  Virtio RSS hash types */
+#define VIRTIO_NET_HASH_TYPE_IPV4	(1 << 0)
+#define VIRTIO_NET_HASH_TYPE_TCPV4	(1 << 1)
+#define VIRTIO_NET_HASH_TYPE_UDPV4	(1 << 2)
+#define VIRTIO_NET_HASH_TYPE_IPV6	(1 << 3)
+#define VIRTIO_NET_HASH_TYPE_TCPV6	(1 << 4)
+#define VIRTIO_NET_HASH_TYPE_UDPV6	(1 << 5)
+#define VIRTIO_NET_HASH_TYPE_IP_EX	(1 << 6)
+#define VIRTIO_NET_HASH_TYPE_TCP_EX	(1 << 7)
+#define VIRTIO_NET_HASH_TYPE_UDP_EX	(1 << 8)
+
+#define VIRTIO_NET_HASH_TYPE_MASK ( \
+	VIRTIO_NET_HASH_TYPE_IPV4 | \
+	VIRTIO_NET_HASH_TYPE_TCPV4 | \
+	VIRTIO_NET_HASH_TYPE_UDPV4 | \
+	VIRTIO_NET_HASH_TYPE_IPV6 | \
+	VIRTIO_NET_HASH_TYPE_TCPV6 | \
+	VIRTIO_NET_HASH_TYPE_UDPV6 | \
+	VIRTIO_NET_HASH_TYPE_IP_EX | \
+	VIRTIO_NET_HASH_TYPE_TCP_EX | \
+	VIRTIO_NET_HASH_TYPE_UDP_EX)
+
+
 /*
  * Maximum number of virtqueues per device.
  */
@@ -157,7 +181,9 @@ struct virtio_net_config {
 	 * Any other value stands for unknown.
 	 */
 	uint8_t duplex;
-
+	uint8_t rss_max_key_size;
+	uint16_t rss_max_indirection_table_length;
+	uint32_t supported_hash_types;
 } __rte_packed;
 
 struct virtio_hw {
@@ -190,6 +216,9 @@ struct virtio_hw {
 	rte_spinlock_t state_lock;
 	struct rte_mbuf **inject_pkts;
 	uint16_t max_queue_pairs;
+	uint32_t rss_hash_types;
+	uint16_t *rss_reta;
+	uint8_t *rss_key;
 	uint64_t req_guest_features;
 	struct virtnet_ctl *cvq;
 };
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e58085a2c9..dece2e9629 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -50,6 +50,16 @@ static int virtio_dev_info_get(struct rte_eth_dev *dev,
 static int virtio_dev_link_update(struct rte_eth_dev *dev,
 	int wait_to_complete);
 static int virtio_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
+static int virtio_dev_rss_hash_update(struct rte_eth_dev *dev,
+		struct rte_eth_rss_conf *rss_conf);
+static int virtio_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
+		struct rte_eth_rss_conf *rss_conf);
+static int virtio_dev_rss_reta_update(struct rte_eth_dev *dev,
+			 struct rte_eth_rss_reta_entry64 *reta_conf,
+			 uint16_t reta_size);
+static int virtio_dev_rss_reta_query(struct rte_eth_dev *dev,
+			 struct rte_eth_rss_reta_entry64 *reta_conf,
+			 uint16_t reta_size);
 
 static void virtio_set_hwaddr(struct virtio_hw *hw);
 static void virtio_get_hwaddr(struct virtio_hw *hw);
@@ -346,7 +356,38 @@ virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 }
 
 static int
-virtio_set_multiple_queues(struct rte_eth_dev *dev, uint16_t nb_queues)
+virtio_set_multiple_queues_rss(struct rte_eth_dev *dev, uint16_t nb_queues)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+	struct virtio_pmd_ctrl ctrl;
+	struct virtio_net_ctrl_rss rss;
+	int dlen[1], ret;
+
+	rss.hash_types = hw->rss_hash_types & VIRTIO_NET_HASH_TYPE_MASK;
+	rss.indirection_table_mask = VIRTIO_NET_RSS_RETA_SIZE - 1;
+	rss.unclassified_queue = 0;
+	memcpy(rss.indirection_table, hw->rss_reta, VIRTIO_NET_RSS_RETA_SIZE * sizeof(uint16_t));
+	rss.max_tx_vq = nb_queues;
+	rss.hash_key_length = VIRTIO_NET_RSS_KEY_SIZE;
+	memcpy(rss.hash_key_data, hw->rss_key, VIRTIO_NET_RSS_KEY_SIZE);
+
+	ctrl.hdr.class = VIRTIO_NET_CTRL_MQ;
+	ctrl.hdr.cmd = VIRTIO_NET_CTRL_MQ_RSS_CONFIG;
+	memcpy(ctrl.data, &rss, sizeof(rss));
+
+	dlen[0] = sizeof(rss);
+
+	ret = virtio_send_command(hw->cvq, &ctrl, dlen, 1);
+	if (ret) {
+		PMD_INIT_LOG(ERR, "RSS multiqueue configured but send command failed");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
+virtio_set_multiple_queues_auto(struct rte_eth_dev *dev, uint16_t nb_queues)
 {
 	struct virtio_hw *hw = dev->data->dev_private;
 	struct virtio_pmd_ctrl ctrl;
@@ -369,6 +410,17 @@ virtio_set_multiple_queues(struct rte_eth_dev *dev, uint16_t nb_queues)
 	return 0;
 }
 
+static int
+virtio_set_multiple_queues(struct rte_eth_dev *dev, uint16_t nb_queues)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+
+	if (virtio_with_feature(hw, VIRTIO_NET_F_RSS))
+		return virtio_set_multiple_queues_rss(dev, nb_queues);
+	else
+		return virtio_set_multiple_queues_auto(dev, nb_queues);
+}
+
 static void
 virtio_dev_queue_release(void *queue __rte_unused)
 {
@@ -699,6 +751,16 @@ virtio_alloc_queues(struct rte_eth_dev *dev)
 
 static void virtio_queues_unbind_intr(struct rte_eth_dev *dev);
 
+static void
+virtio_free_rss(struct virtio_hw *hw)
+{
+	rte_free(hw->rss_key);
+	hw->rss_key = NULL;
+
+	rte_free(hw->rss_reta);
+	hw->rss_reta = NULL;
+}
+
 int
 virtio_dev_close(struct rte_eth_dev *dev)
 {
@@ -729,6 +791,7 @@ virtio_dev_close(struct rte_eth_dev *dev)
 	virtio_reset(hw);
 	virtio_dev_free_mbufs(dev);
 	virtio_free_queues(hw);
+	virtio_free_rss(hw);
 
 	return VIRTIO_OPS(hw)->dev_close(hw);
 }
@@ -969,6 +1032,10 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
 	.rx_queue_release        = virtio_dev_queue_release,
 	.tx_queue_setup          = virtio_dev_tx_queue_setup,
 	.tx_queue_release        = virtio_dev_queue_release,
+	.rss_hash_update         = virtio_dev_rss_hash_update,
+	.rss_hash_conf_get       = virtio_dev_rss_hash_conf_get,
+	.reta_update             = virtio_dev_rss_reta_update,
+	.reta_query              = virtio_dev_rss_reta_query,
 	/* collect stats per queue */
 	.queue_stats_mapping_set = virtio_dev_queue_stats_mapping_set,
 	.vlan_filter_set         = virtio_vlan_filter_set,
@@ -1710,6 +1777,270 @@ virtio_configure_intr(struct rte_eth_dev *dev)
 
 	return 0;
 }
+
+static uint64_t
+virtio_to_ethdev_rss_offloads(uint64_t virtio_hash_types)
+{
+	uint64_t rss_offloads = 0;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_IPV4)
+		rss_offloads |= ETH_RSS_IPV4 | ETH_RSS_FRAG_IPV4 | ETH_RSS_NONFRAG_IPV4_OTHER;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_TCPV4)
+		rss_offloads |= ETH_RSS_NONFRAG_IPV4_TCP;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_UDPV4)
+		rss_offloads |= ETH_RSS_NONFRAG_IPV4_UDP;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_IPV6)
+		rss_offloads |= ETH_RSS_IPV6 | ETH_RSS_FRAG_IPV6 | ETH_RSS_NONFRAG_IPV6_OTHER;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_TCPV6)
+		rss_offloads |= ETH_RSS_NONFRAG_IPV6_TCP;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_UDPV6)
+		rss_offloads |= ETH_RSS_NONFRAG_IPV6_UDP;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_IP_EX)
+		rss_offloads |= ETH_RSS_IPV6_EX;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_TCP_EX)
+		rss_offloads |= ETH_RSS_IPV6_TCP_EX;
+
+	if (virtio_hash_types & VIRTIO_NET_HASH_TYPE_UDP_EX)
+		rss_offloads |= ETH_RSS_IPV6_UDP_EX;
+
+	return rss_offloads;
+}
+
+static int
+virtio_dev_get_rss_config(struct virtio_hw *hw, uint32_t *rss_hash_types)
+{
+	struct virtio_net_config local_config;
+	struct virtio_net_config *config = &local_config;
+
+	virtio_read_dev_config(hw,
+			offsetof(struct virtio_net_config, rss_max_key_size),
+			&config->rss_max_key_size,
+			sizeof(config->rss_max_key_size));
+	if (config->rss_max_key_size < VIRTIO_NET_RSS_KEY_SIZE) {
+		PMD_INIT_LOG(ERR, "Invalid device RSS max key size (%u)",
+				config->rss_max_key_size);
+		return -1;
+	}
+
+	virtio_read_dev_config(hw,
+			offsetof(struct virtio_net_config,
+				rss_max_indirection_table_length),
+			&config->rss_max_indirection_table_length,
+			sizeof(config->rss_max_indirection_table_length));
+	if (config->rss_max_indirection_table_length < VIRTIO_NET_RSS_RETA_SIZE) {
+		PMD_INIT_LOG(ERR, "Invalid device RSS max reta size (%u)",
+				config->rss_max_indirection_table_length);
+		return -1;
+	}
+
+	virtio_read_dev_config(hw,
+			offsetof(struct virtio_net_config, supported_hash_types),
+			&config->supported_hash_types,
+			sizeof(config->supported_hash_types));
+	if ((config->supported_hash_types & VIRTIO_NET_HASH_TYPE_MASK) == 0) {
+		PMD_INIT_LOG(ERR, "Invalid device RSS hash types (%u)",
+				config->supported_hash_types);
+		return -1;
+	}
+
+	*rss_hash_types = config->supported_hash_types & VIRTIO_NET_HASH_TYPE_MASK;
+
+	PMD_INIT_LOG(DEBUG, "Device RSS config:");
+	PMD_INIT_LOG(DEBUG, "\t-Max key size: %u", config->rss_max_key_size);
+	PMD_INIT_LOG(DEBUG, "\t-Max reta size: %u", config->rss_max_indirection_table_length);
+	PMD_INIT_LOG(DEBUG, "\t-Supported hash types: 0x%x", *rss_hash_types);
+
+	return 0;
+}
+
+static int
+virtio_dev_rss_hash_update(struct rte_eth_dev *dev,
+		struct rte_eth_rss_conf *rss_conf)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+	uint16_t nb_queues;
+
+	if (!virtio_with_feature(hw, VIRTIO_NET_F_RSS))
+		return -ENOTSUP;
+
+	if (rss_conf->rss_hf & ~virtio_to_ethdev_rss_offloads(VIRTIO_NET_HASH_TYPE_MASK))
+		return -EINVAL;
+
+	hw->rss_hash_types = 0;
+
+	if (rss_conf->rss_hf & (ETH_RSS_IPV4 | ETH_RSS_FRAG_IPV4 | ETH_RSS_NONFRAG_IPV4_OTHER))
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_IPV4;
+
+	if (rss_conf->rss_hf & ETH_RSS_NONFRAG_IPV4_TCP)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_TCPV4;
+
+	if (rss_conf->rss_hf & ETH_RSS_NONFRAG_IPV4_UDP)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_UDPV4;
+
+	if (rss_conf->rss_hf & (ETH_RSS_IPV6 | ETH_RSS_FRAG_IPV6 | ETH_RSS_NONFRAG_IPV6_OTHER))
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_IPV6;
+
+	if (rss_conf->rss_hf & ETH_RSS_NONFRAG_IPV6_TCP)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_TCPV6;
+
+	if (rss_conf->rss_hf & ETH_RSS_NONFRAG_IPV6_UDP)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_UDPV6;
+
+	if (rss_conf->rss_hf & ETH_RSS_IPV6_EX)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_IP_EX;
+
+	if (rss_conf->rss_hf & ETH_RSS_IPV6_TCP_EX)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_TCP_EX;
+
+	if (rss_conf->rss_hf & ETH_RSS_IPV6_UDP_EX)
+		hw->rss_hash_types |= VIRTIO_NET_HASH_TYPE_UDP_EX;
+
+	if (rss_conf->rss_key && rss_conf->rss_key_len) {
+		if (rss_conf->rss_key_len != VIRTIO_NET_RSS_KEY_SIZE) {
+			PMD_INIT_LOG(ERR, "Driver only supports %u RSS key length",
+					VIRTIO_NET_RSS_KEY_SIZE);
+			return -EINVAL;
+		}
+		memcpy(hw->rss_key, rss_conf->rss_key, VIRTIO_NET_RSS_KEY_SIZE);
+	}
+
+	nb_queues = RTE_MAX(dev->data->nb_rx_queues, dev->data->nb_tx_queues);
+	return virtio_set_multiple_queues_rss(dev, nb_queues);
+}
+
+static int
+virtio_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
+		struct rte_eth_rss_conf *rss_conf)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+
+	if (!virtio_with_feature(hw, VIRTIO_NET_F_RSS))
+		return -ENOTSUP;
+
+	if (!rss_conf)
+		return -EINVAL;
+
+	if (rss_conf->rss_key && rss_conf->rss_key_len >= VIRTIO_NET_RSS_KEY_SIZE)
+		memcpy(rss_conf->rss_key, hw->rss_key, VIRTIO_NET_RSS_KEY_SIZE);
+	rss_conf->rss_key_len = VIRTIO_NET_RSS_KEY_SIZE;
+	rss_conf->rss_hf = virtio_to_ethdev_rss_offloads(hw->rss_hash_types);
+
+	return 0;
+}
+
+static int virtio_dev_rss_reta_update(struct rte_eth_dev *dev,
+			 struct rte_eth_rss_reta_entry64 *reta_conf,
+			 uint16_t reta_size)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+	uint16_t nb_queues;
+	int idx, pos, i;
+
+	if (!virtio_with_feature(hw, VIRTIO_NET_F_RSS))
+		return -ENOTSUP;
+
+	if (!reta_conf)
+		return -EINVAL;
+
+	if (reta_size != VIRTIO_NET_RSS_RETA_SIZE)
+		return -EINVAL;
+
+	for (idx = 0, i = 0; i < reta_size; i++) {
+		idx = i / RTE_RETA_GROUP_SIZE;
+		pos = i % RTE_RETA_GROUP_SIZE;
+
+		if (((reta_conf[idx].mask >> pos) & 0x1) == 0)
+			continue;
+
+		hw->rss_reta[i] = reta_conf[idx].reta[pos];
+	}
+
+	nb_queues = RTE_MAX(dev->data->nb_rx_queues, dev->data->nb_tx_queues);
+	return virtio_set_multiple_queues_rss(dev, nb_queues);
+}
+
+static int virtio_dev_rss_reta_query(struct rte_eth_dev *dev,
+			 struct rte_eth_rss_reta_entry64 *reta_conf,
+			 uint16_t reta_size)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+	int idx, i;
+
+	if (!virtio_with_feature(hw, VIRTIO_NET_F_RSS))
+		return -ENOTSUP;
+
+	if (!reta_conf)
+		return -EINVAL;
+
+	if (reta_size != VIRTIO_NET_RSS_RETA_SIZE)
+		return -EINVAL;
+
+	for (idx = 0, i = 0; i < reta_size; i++) {
+		idx = i / RTE_RETA_GROUP_SIZE;
+		reta_conf[idx].reta[i % RTE_RETA_GROUP_SIZE] = hw->rss_reta[i];
+	}
+
+	return 0;
+}
+
+/*
+ * As default RSS hash key, it uses the default key of the
+ * Intel IXGBE devices. It can be updated by the application
+ * with any 40B key value.
+ */
+static uint8_t rss_intel_key[VIRTIO_NET_RSS_KEY_SIZE] = {
+	0x6D, 0x5A, 0x56, 0xDA, 0x25, 0x5B, 0x0E, 0xC2,
+	0x41, 0x67, 0x25, 0x3D, 0x43, 0xA3, 0x8F, 0xB0,
+	0xD0, 0xCA, 0x2B, 0xCB, 0xAE, 0x7B, 0x30, 0xB4,
+	0x77, 0xCB, 0x2D, 0xA3, 0x80, 0x30, 0xF2, 0x0C,
+	0x6A, 0x42, 0xB7, 0x3B, 0xBE, 0xAC, 0x01, 0xFA,
+};
+
+static int
+virtio_dev_rss_init(struct rte_eth_dev *eth_dev)
+{
+	struct virtio_hw *hw = eth_dev->data->dev_private;
+	uint16_t nb_rx_queues = eth_dev->data->nb_rx_queues;
+	int i;
+
+	if (virtio_dev_get_rss_config(hw, &hw->rss_hash_types))
+		return -1;
+
+	if (!hw->rss_key) {
+		/* Setup default RSS key if not already setup by the user */
+		hw->rss_key = rte_malloc_socket("rss_key",
+				VIRTIO_NET_RSS_KEY_SIZE, 0,
+				eth_dev->device->numa_node);
+		if (!hw->rss_key) {
+			PMD_INIT_LOG(ERR, "Failed to allocate RSS key");
+			return -1;
+		}
+		rte_memcpy(hw->rss_key, rss_intel_key, VIRTIO_NET_RSS_KEY_SIZE);
+	}
+
+	if (!hw->rss_reta) {
+		/* Setup default RSS reta if not already setup by the user */
+		hw->rss_reta = rte_malloc_socket("rss_reta",
+				VIRTIO_NET_RSS_RETA_SIZE * sizeof(uint16_t), 0,
+				eth_dev->device->numa_node);
+		if (!hw->rss_reta) {
+			PMD_INIT_LOG(ERR, "Failed to allocate RSS reta");
+			return -1;
+		}
+		for (i = 0; i < VIRTIO_NET_RSS_RETA_SIZE; i++)
+			hw->rss_reta[i] = i % nb_rx_queues;
+	}
+
+	return 0;
+}
+
 #define DUPLEX_UNKNOWN   0xff
 /* reset device and renegotiate features if needed */
 static int
@@ -1797,14 +2128,15 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 			config->status = 0;
 		}
 
-		if (virtio_with_feature(hw, VIRTIO_NET_F_MQ)) {
+		if (virtio_with_feature(hw, VIRTIO_NET_F_MQ) ||
+				virtio_with_feature(hw, VIRTIO_NET_F_RSS)) {
 			virtio_read_dev_config(hw,
 				offsetof(struct virtio_net_config, max_virtqueue_pairs),
 				&config->max_virtqueue_pairs,
 				sizeof(config->max_virtqueue_pairs));
 		} else {
 			PMD_INIT_LOG(DEBUG,
-				     "VIRTIO_NET_F_MQ is not supported");
+				     "Neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are supported");
 			config->max_virtqueue_pairs = 1;
 		}
 
@@ -1836,6 +2168,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 				VLAN_TAG_LEN - hw->vtnet_hdr_size;
 		}
 
+		hw->rss_hash_types = 0;
+		if (virtio_with_feature(hw, VIRTIO_NET_F_RSS))
+			if (virtio_dev_rss_init(eth_dev))
+				return -1;
+
 		PMD_INIT_LOG(DEBUG, "config->max_virtqueue_pairs=%d",
 				config->max_virtqueue_pairs);
 		PMD_INIT_LOG(DEBUG, "config->status=%d", config->status);
@@ -2079,7 +2416,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	PMD_INIT_LOG(DEBUG, "configure");
 	req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
 
-	if (rxmode->mq_mode != ETH_MQ_RX_NONE) {
+	if (rxmode->mq_mode != ETH_MQ_RX_NONE && rxmode->mq_mode != ETH_MQ_RX_RSS) {
 		PMD_DRV_LOG(ERR,
 			"Unsupported Rx multi queue mode %d",
 			rxmode->mq_mode);
@@ -2099,6 +2436,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return ret;
 	}
 
+	if (rxmode->mq_mode == ETH_MQ_RX_RSS)
+		req_features |= (1ULL << VIRTIO_NET_F_RSS);
+
 	if (rxmode->max_rx_pkt_len > hw->max_mtu + ether_hdr_len)
 		req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
 
@@ -2129,6 +2469,12 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return ret;
 	}
 
+	if ((rxmode->mq_mode & ETH_MQ_RX_RSS_FLAG) &&
+			!virtio_with_feature(hw, VIRTIO_NET_F_RSS)) {
+		PMD_DRV_LOG(ERR, "RSS support requested but not supported by the device");
+		return -ENOTSUP;
+	}
+
 	if ((rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
 			    DEV_RX_OFFLOAD_TCP_CKSUM)) &&
 		!virtio_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM)) {
@@ -2495,6 +2841,7 @@ static int
 virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 {
 	uint64_t tso_mask, host_features;
+	uint32_t rss_hash_types = 0;
 	struct virtio_hw *hw = dev->data->dev_private;
 	dev_info->speed_capa = virtio_dev_speed_capa_get(hw->speed);
 
@@ -2535,6 +2882,18 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		(1ULL << VIRTIO_NET_F_HOST_TSO6);
 	if ((host_features & tso_mask) == tso_mask)
 		dev_info->tx_offload_capa |= DEV_TX_OFFLOAD_TCP_TSO;
+	if (host_features & (1ULL << VIRTIO_NET_F_RSS)) {
+		virtio_dev_get_rss_config(hw, &rss_hash_types);
+		dev_info->hash_key_size = VIRTIO_NET_RSS_KEY_SIZE;
+		dev_info->reta_size = VIRTIO_NET_RSS_RETA_SIZE;
+		dev_info->flow_type_rss_offloads =
+			virtio_to_ethdev_rss_offloads(rss_hash_types);
+	} else {
+		dev_info->hash_key_size = 0;
+		dev_info->reta_size = 0;
+		dev_info->flow_type_rss_offloads = 0;
+	}
+
 
 	return 0;
 }
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 2f63ef2b2d..3fe30d8784 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -45,7 +45,8 @@
 	 1u << VIRTIO_NET_F_GUEST_TSO6     |	\
 	 1u << VIRTIO_NET_F_CSUM           |	\
 	 1u << VIRTIO_NET_F_HOST_TSO4      |	\
-	 1u << VIRTIO_NET_F_HOST_TSO6)
+	 1u << VIRTIO_NET_F_HOST_TSO6      |	\
+	 1ULL << VIRTIO_NET_F_RSS)
 
 extern const struct eth_dev_ops virtio_user_secondary_eth_dev_ops;
 
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 03957b2bd0..99a0e68d38 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -182,6 +182,24 @@ struct virtio_net_ctrl_mac {
 #define VIRTIO_NET_CTRL_VLAN_ADD 0
 #define VIRTIO_NET_CTRL_VLAN_DEL 1
 
+/**
+ * RSS control
+ *
+ * The RSS feature <todo>
+ */
+#define VIRTIO_NET_RSS_RETA_SIZE 128
+#define VIRTIO_NET_RSS_KEY_SIZE       40
+
+struct virtio_net_ctrl_rss {
+	uint32_t hash_types;
+	uint16_t indirection_table_mask;
+	uint16_t unclassified_queue;
+	uint16_t indirection_table[VIRTIO_NET_RSS_RETA_SIZE];
+	uint16_t max_tx_vq;
+	uint8_t hash_key_length;
+	uint8_t hash_key_data[VIRTIO_NET_RSS_KEY_SIZE];
+};
+
 /*
  * Control link announce acknowledgement
  *
@@ -272,7 +290,10 @@ struct virtqueue {
 
 /* If multiqueue is provided by host, then we suppport it. */
 #define VIRTIO_NET_CTRL_MQ   4
+
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET        0
+#define VIRTIO_NET_CTRL_MQ_RSS_CONFIG          1
+
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
 
-- 
2.31.1

Re: [dpdk-dev] [PATCH 1/3] net/virtio: add initial RSS support

[Andrew Rybchenko]

On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> This patch adds RSS support to the Virtio PMD. It provides

I'd drop the first sentence since summary already says so and
suggest to start the second from "Provide ..."

> the capability to update the hash key, hash types and reta

reta -> RETA (see devtools/words-case.txt)

> table on the fly (without needing to stop/start the
> device). However, the key length and the number of reta

reta -> RETA

> entries are fixed to 40B and 128 entries respectively. This
> is done in order to simplify the design, but may be
> revisited later as the Virtio spec provides this
> flexibility.

I've failed to find F_RSS definion in virtio spec.
Could you share the reference, please.

Without the sepc it is almost impossible to review the code.

> 
> Note that only VIRTIO_NET_F_RSS support is implemented,
> VIRTIO_NET_F_HASH_REPORT, which would enable reporting the
> packet RSS hash calculated by the device into mbuf.rss, is
> not yet supported.
> 
> Regarding the default RSS configuration, it has been
> chosen to use the default Intel ixgbe key as default key,
> and default reta is a simple modulo between the hash and

reta -> RETA

> the number of Rx queues.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  doc/guides/nics/features/virtio.ini    |   2 +
>  doc/guides/nics/virtio.rst             |   3 +
>  doc/guides/rel_notes/release_21_11.rst |   5 +
>  drivers/net/virtio/virtio.h            |  31 ++-
>  drivers/net/virtio/virtio_ethdev.c     | 367 ++++++++++++++++++++++++-
>  drivers/net/virtio/virtio_ethdev.h     |   3 +-
>  drivers/net/virtio/virtqueue.h         |  21 ++
>  7 files changed, 426 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
> index 48f6f393b1..883dd1426c 100644
> --- a/doc/guides/nics/features/virtio.ini
> +++ b/doc/guides/nics/features/virtio.ini
> @@ -14,6 +14,8 @@ Promiscuous mode     = Y
>  Allmulticast mode    = Y
>  Unicast MAC filter   = Y
>  Multicast MAC filter = Y

I'd say that
RSS has                = P
should be added here.

> +RSS key update       = Y
> +RSS reta update      = Y
>  VLAN filter          = Y
>  Basic stats          = Y
>  Stats per queue      = Y
> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
> index 82ce7399ce..6e451cd67c 100644
> --- a/doc/guides/nics/virtio.rst
> +++ b/doc/guides/nics/virtio.rst
> @@ -73,6 +73,9 @@ In this release, the virtio PMD driver provides the basic functionality of packe
>  
>  *   Virtio supports using port IO to get PCI resource when UIO module is not available.
>  
> +*   Virtio supports RSS Rx mode with 40B configurable hash key len, 128

len -> length

> +    configurable reta entries and configurable hash types.

reta -> RETA

> +
>  Prerequisites
>  -------------
>  
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index d707a554ef..21d2b09838 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -55,6 +55,11 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +   * **Added initial RSS support to Virtio PMD.**
> +
> +     Initial support for RSS receive mode has been added to the Virtio PMD,
> +     with the capability for the application to configure the hash Key, the
> +     reta and the hash types. Virtio hash reporting is yet to be added.

reta -> RETA

Please, add one more empty line to have two empty lines
before the next section.

>  
>  Removed Items
>  -------------

[snip]

Re: [dpdk-dev] [PATCH 1/3] net/virtio: add initial RSS support

[Maxime Coquelin]

Thanks for the review!

On 9/10/21 12:06 PM, Andrew Rybchenko wrote:
> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>> This patch adds RSS support to the Virtio PMD. It provides
> 
> I'd drop the first sentence since summary already says so and
> suggest to start the second from "Provide ..."
> 
>> the capability to update the hash key, hash types and reta
> 
> reta -> RETA (see devtools/words-case.txt)
> 
>> table on the fly (without needing to stop/start the
>> device). However, the key length and the number of reta
> 
> reta -> RETA

Got it, I'll fix it here and everywhere else.

>> entries are fixed to 40B and 128 entries respectively. This
>> is done in order to simplify the design, but may be
>> revisited later as the Virtio spec provides this
>> flexibility.
> 
> I've failed to find F_RSS definion in virtio spec.
> Could you share the reference, please.
> 
> Without the sepc it is almost impossible to review the code.

Sorry, I forgot to add the link, you can find it on the Virtio-spec
github master branch:

https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex

For readability, I would suggest you to clone it and build the HTML or
PDF versions.

>>
>> Note that only VIRTIO_NET_F_RSS support is implemented,
>> VIRTIO_NET_F_HASH_REPORT, which would enable reporting the
>> packet RSS hash calculated by the device into mbuf.rss, is
>> not yet supported.
>>
>> Regarding the default RSS configuration, it has been
>> chosen to use the default Intel ixgbe key as default key,
>> and default reta is a simple modulo between the hash and
> 
> reta -> RETA
> 
>> the number of Rx queues.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  doc/guides/nics/features/virtio.ini    |   2 +
>>  doc/guides/nics/virtio.rst             |   3 +
>>  doc/guides/rel_notes/release_21_11.rst |   5 +
>>  drivers/net/virtio/virtio.h            |  31 ++-
>>  drivers/net/virtio/virtio_ethdev.c     | 367 ++++++++++++++++++++++++-
>>  drivers/net/virtio/virtio_ethdev.h     |   3 +-
>>  drivers/net/virtio/virtqueue.h         |  21 ++
>>  7 files changed, 426 insertions(+), 6 deletions(-)
>>
>> diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
>> index 48f6f393b1..883dd1426c 100644
>> --- a/doc/guides/nics/features/virtio.ini
>> +++ b/doc/guides/nics/features/virtio.ini
>> @@ -14,6 +14,8 @@ Promiscuous mode     = Y
>>  Allmulticast mode    = Y
>>  Unicast MAC filter   = Y
>>  Multicast MAC filter = Y
> 
> I'd say that
> RSS has                = P
> should be added here.

OK, as I mentionned in the cover letter, I was not sure becasue the doc
says "RSS hash" uses DEV_RX_OFFLOAD_RSS_HASH and provides mbuf.rss,
which this patch does not since it does not support (yet)
VIRTIO_NET_F_HASH_REPORT.
http://doc.dpdk.org/guides/nics/features.html#rss-hash

I can try to add VIRTIO_NET_F_HASH_REPORT in v2, so that we can set RSS
hash = Y without any doubts :)

Regards,
Maxime

Re: [dpdk-dev] [PATCH 1/3] net/virtio: add initial RSS support

[Andrew Rybchenko]

On 9/10/21 2:44 PM, Maxime Coquelin wrote:
> Thanks for the review!
> 
> On 9/10/21 12:06 PM, Andrew Rybchenko wrote:
>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>
>> I've failed to find F_RSS definion in virtio spec.
>> Could you share the reference, please.
>>
>> Without the sepc it is almost impossible to review the code.
> 
> Sorry, I forgot to add the link, you can find it on the Virtio-spec
> github master branch:
> 
> https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex
> 
> For readability, I would suggest you to clone it and build the HTML or
> PDF versions.

Many thanks.

>>> diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
>>> index 48f6f393b1..883dd1426c 100644
>>> --- a/doc/guides/nics/features/virtio.ini
>>> +++ b/doc/guides/nics/features/virtio.ini
>>> @@ -14,6 +14,8 @@ Promiscuous mode     = Y
>>>  Allmulticast mode    = Y
>>>  Unicast MAC filter   = Y
>>>  Multicast MAC filter = Y
>>
>> I'd say that
>> RSS has                = P
>> should be added here.
> 
> OK, as I mentionned in the cover letter, I was not sure becasue the doc
> says "RSS hash" uses DEV_RX_OFFLOAD_RSS_HASH and provides mbuf.rss,
> which this patch does not since it does not support (yet)
> VIRTIO_NET_F_HASH_REPORT.
> http://doc.dpdk.org/guides/nics/features.html#rss-hash

Yes, I've seen question in cover letter. It is some kind of gap
in features to claim overall RSS support - just hashing and
distribution across queues without hash forwarding to user.
The item definition covers both generic support and offload to
deliver RSS hash. That's why I suggest P (partial).

> I can try to add VIRTIO_NET_F_HASH_REPORT in v2, so that we can set RSS
> hash = Y without any doubts :)
> 
> Regards,
> Maxime
> 

Re: [dpdk-dev] [PATCH 1/3] net/virtio: add initial RSS support

[Maxime Coquelin]

On 9/10/21 2:58 PM, Andrew Rybchenko wrote:
> On 9/10/21 2:44 PM, Maxime Coquelin wrote:
>> Thanks for the review!
>>
>> On 9/10/21 12:06 PM, Andrew Rybchenko wrote:
>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>
>>> I've failed to find F_RSS definion in virtio spec.
>>> Could you share the reference, please.
>>>
>>> Without the sepc it is almost impossible to review the code.
>>
>> Sorry, I forgot to add the link, you can find it on the Virtio-spec
>> github master branch:
>>
>> https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex
>>
>> For readability, I would suggest you to clone it and build the HTML or
>> PDF versions.
> 
> Many thanks.
> 
>>>> diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
>>>> index 48f6f393b1..883dd1426c 100644
>>>> --- a/doc/guides/nics/features/virtio.ini
>>>> +++ b/doc/guides/nics/features/virtio.ini
>>>> @@ -14,6 +14,8 @@ Promiscuous mode     = Y
>>>>  Allmulticast mode    = Y
>>>>  Unicast MAC filter   = Y
>>>>  Multicast MAC filter = Y
>>>
>>> I'd say that
>>> RSS has                = P
>>> should be added here.
>>
>> OK, as I mentionned in the cover letter, I was not sure becasue the doc
>> says "RSS hash" uses DEV_RX_OFFLOAD_RSS_HASH and provides mbuf.rss,
>> which this patch does not since it does not support (yet)
>> VIRTIO_NET_F_HASH_REPORT.
>> http://doc.dpdk.org/guides/nics/features.html#rss-hash
> 
> Yes, I've seen question in cover letter. It is some kind of gap
> in features to claim overall RSS support - just hashing and
> distribution across queues without hash forwarding to user.
> The item definition covers both generic support and offload to
> deliver RSS hash. That's why I suggest P (partial).

Ok, got it!

Thanks,
Maxime

>> I can try to add VIRTIO_NET_F_HASH_REPORT in v2, so that we can set RSS
>> hash = Y without any doubts :)
>>
>> Regards,
>> Maxime
>>
> 

[dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Maxime Coquelin]

port_rss_hash_key_update() initializes rss_conf with the
RSS hash type and key provided by the user, but it calls
rte_eth_dev_rss_hash_conf_get() before calling
rte_eth_dev_rss_hash_update(), which overides the parsed
config with current NIC's config.

While the RSS key value is set again after, this is not
the case of the key length and the type of hash.

There is no need to read the RSS config from the NIC, let's
just try to set the user defined one.

Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
Cc: stable@dpdk.org
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 app/test-pmd/config.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 31d8ba1b91..451bda53b1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
 	int diag;
 	unsigned int i;
 
-	rss_conf.rss_key = NULL;
+	rss_conf.rss_key = hash_key;
 	rss_conf.rss_key_len = hash_key_len;
 	rss_conf.rss_hf = 0;
 	for (i = 0; rss_type_table[i].str; i++) {
 		if (!strcmp(rss_type_table[i].str, rss_type))
 			rss_conf.rss_hf = rss_type_table[i].rss_type;
 	}
-	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
-	if (diag == 0) {
-		rss_conf.rss_key = hash_key;
-		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
-	}
+	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
 	if (diag == 0)
 		return;
 
-- 
2.31.1

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Andrew Rybchenko]

On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> port_rss_hash_key_update() initializes rss_conf with the
> RSS hash type and key provided by the user, but it calls
> rte_eth_dev_rss_hash_conf_get() before calling
> rte_eth_dev_rss_hash_update(), which overides the parsed
> config with current NIC's config.
> 
> While the RSS key value is set again after, this is not
> the case of the key length and the type of hash.
> 
> There is no need to read the RSS config from the NIC, let's
> just try to set the user defined one.
> 
> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> Cc: stable@dpdk.org
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  app/test-pmd/config.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 31d8ba1b91..451bda53b1 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>  	int diag;
>  	unsigned int i;
>  
> -	rss_conf.rss_key = NULL;
> +	rss_conf.rss_key = hash_key;
>  	rss_conf.rss_key_len = hash_key_len;
>  	rss_conf.rss_hf = 0;
>  	for (i = 0; rss_type_table[i].str; i++) {
>  		if (!strcmp(rss_type_table[i].str, rss_type))
>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>  	}
> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> -	if (diag == 0) {
> -		rss_conf.rss_key = hash_key;
> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> -	}
> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);

I'm not 100% sure, but I'd say the intent above could be
to update key only as the function name says. I.e. keep
rss_hf as is. That could be the reason to get first.

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Maxime Coquelin]

On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>> port_rss_hash_key_update() initializes rss_conf with the
>> RSS hash type and key provided by the user, but it calls
>> rte_eth_dev_rss_hash_conf_get() before calling
>> rte_eth_dev_rss_hash_update(), which overides the parsed
>> config with current NIC's config.
>>
>> While the RSS key value is set again after, this is not
>> the case of the key length and the type of hash.
>>
>> There is no need to read the RSS config from the NIC, let's
>> just try to set the user defined one.
>>
>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
>> Cc: stable@dpdk.org
>> Cc: nelio.laranjeiro@6wind.com
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  app/test-pmd/config.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 31d8ba1b91..451bda53b1 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>>  	int diag;
>>  	unsigned int i;
>>  
>> -	rss_conf.rss_key = NULL;
>> +	rss_conf.rss_key = hash_key;
>>  	rss_conf.rss_key_len = hash_key_len;
>>  	rss_conf.rss_hf = 0;
>>  	for (i = 0; rss_type_table[i].str; i++) {
>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>  	}
>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>> -	if (diag == 0) {
>> -		rss_conf.rss_key = hash_key;
>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>> -	}
>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> 
> I'm not 100% sure, but I'd say the intent above could be
> to update key only as the function name says. I.e. keep
> rss_hf as is. That could be the reason to get first.
> 

I think that was the intial purpose of the command, but patch
8205e241b2b0 added setting the hash type as mandatory. There are
no other command to configure the hash type from testpmd AFAICT.

Also, even without 8205e241b2b0, the function was broken because the
key length was overiden.

Regards,
Maxime

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Andrew Rybchenko]

On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> 
> 
> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>> port_rss_hash_key_update() initializes rss_conf with the
>>> RSS hash type and key provided by the user, but it calls
>>> rte_eth_dev_rss_hash_conf_get() before calling
>>> rte_eth_dev_rss_hash_update(), which overides the parsed
>>> config with current NIC's config.
>>>
>>> While the RSS key value is set again after, this is not
>>> the case of the key length and the type of hash.
>>>
>>> There is no need to read the RSS config from the NIC, let's
>>> just try to set the user defined one.
>>>
>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
>>> Cc: stable@dpdk.org
>>> Cc: nelio.laranjeiro@6wind.com
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  app/test-pmd/config.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 31d8ba1b91..451bda53b1 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>>>  	int diag;
>>>  	unsigned int i;
>>>  
>>> -	rss_conf.rss_key = NULL;
>>> +	rss_conf.rss_key = hash_key;
>>>  	rss_conf.rss_key_len = hash_key_len;
>>>  	rss_conf.rss_hf = 0;
>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>  	}
>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>> -	if (diag == 0) {
>>> -		rss_conf.rss_key = hash_key;
>>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>> -	}
>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>
>> I'm not 100% sure, but I'd say the intent above could be
>> to update key only as the function name says. I.e. keep
>> rss_hf as is. That could be the reason to get first.
>>
> 
> I think that was the intial purpose of the command, but patch
> 8205e241b2b0 added setting the hash type as mandatory. There are
> no other command to configure the hash type from testpmd AFAICT.
> 
> Also, even without 8205e241b2b0, the function was broken because the
> key length was overiden.

I see, many thanks for explanations.

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Nélio Laranjeiro]

On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> > 
> > 
> > On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>> port_rss_hash_key_update() initializes rss_conf with the
> >>> RSS hash type and key provided by the user, but it calls
> >>> rte_eth_dev_rss_hash_conf_get() before calling
> >>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>> config with current NIC's config.
> >>>
> >>> While the RSS key value is set again after, this is not
> >>> the case of the key length and the type of hash.
> >>>
> >>> There is no need to read the RSS config from the NIC, let's
> >>> just try to set the user defined one.
> >>>
> >>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> >>> Cc: stable@dpdk.org
> >>> Cc: nelio.laranjeiro@6wind.com
> >>>
> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> ---
> >>>  app/test-pmd/config.c | 8 ++------
> >>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>> index 31d8ba1b91..451bda53b1 100644
> >>> --- a/app/test-pmd/config.c
> >>> +++ b/app/test-pmd/config.c
> >>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
> >>>  	int diag;
> >>>  	unsigned int i;
> >>>  
> >>> -	rss_conf.rss_key = NULL;
> >>> +	rss_conf.rss_key = hash_key;
> >>>  	rss_conf.rss_key_len = hash_key_len;
> >>>  	rss_conf.rss_hf = 0;
> >>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>  	}
> >>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>> -	if (diag == 0) {
> >>> -		rss_conf.rss_key = hash_key;
> >>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>> -	}
> >>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>
> >> I'm not 100% sure, but I'd say the intent above could be
> >> to update key only as the function name says. I.e. keep
> >> rss_hf as is. That could be the reason to get first.

True,

> > I think that was the intial purpose of the command, but patch
> > 8205e241b2b0 added setting the hash type as mandatory. There are
> > no other command to configure the hash type from testpmd AFAICT.

Also for the same initial purpose, some NIC have an hash key per
protocol, by default it uses the same key for all of them but it can be
configured individually making for example key0 for all protocols expect
IPv4 which uses key1.

> > Also, even without 8205e241b2b0, the function was broken because the
> > key length was overiden.
> 
> I see, many thanks for explanations.

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Maxime Coquelin]

Hi Nélio,

On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>> port_rss_hash_key_update() initializes rss_conf with the
>>>>> RSS hash type and key provided by the user, but it calls
>>>>> rte_eth_dev_rss_hash_conf_get() before calling
>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
>>>>> config with current NIC's config.
>>>>>
>>>>> While the RSS key value is set again after, this is not
>>>>> the case of the key length and the type of hash.
>>>>>
>>>>> There is no need to read the RSS config from the NIC, let's
>>>>> just try to set the user defined one.
>>>>>
>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
>>>>> Cc: stable@dpdk.org
>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>>
>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> ---
>>>>>  app/test-pmd/config.c | 8 ++------
>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>>> index 31d8ba1b91..451bda53b1 100644
>>>>> --- a/app/test-pmd/config.c
>>>>> +++ b/app/test-pmd/config.c
>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>>>>>  	int diag;
>>>>>  	unsigned int i;
>>>>>  
>>>>> -	rss_conf.rss_key = NULL;
>>>>> +	rss_conf.rss_key = hash_key;
>>>>>  	rss_conf.rss_key_len = hash_key_len;
>>>>>  	rss_conf.rss_hf = 0;
>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>>  	}
>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>> -	if (diag == 0) {
>>>>> -		rss_conf.rss_key = hash_key;
>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>> -	}
>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>
>>>> I'm not 100% sure, but I'd say the intent above could be
>>>> to update key only as the function name says. I.e. keep
>>>> rss_hf as is. That could be the reason to get first.
> 
> True,
> 
>>> I think that was the intial purpose of the command, but patch
>>> 8205e241b2b0 added setting the hash type as mandatory. There are
>>> no other command to configure the hash type from testpmd AFAICT.
> 
> Also for the same initial purpose, some NIC have an hash key per
> protocol, by default it uses the same key for all of them but it can be
> configured individually making for example key0 for all protocols expect
> IPv4 which uses key1.

Thanks for the info, I have looked at most drivers but didn't found one
that support this feature, could you give some pointer?

Given how the drivers implément the callback, do you agree with the fix,
or do you have something else in mind?

Thanks,
Maxime

>>> Also, even without 8205e241b2b0, the function was broken because the
>>> key length was overiden.
>>
>> I see, many thanks for explanations.
> 

Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Nélio Laranjeiro]

+Shahaf,

Hi Maxime,

On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> Hi Nélio,
> 
> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> > On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>
> >>>
> >>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>> port_rss_hash_key_update() initializes rss_conf with the
> >>>>> RSS hash type and key provided by the user, but it calls
> >>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>>>> config with current NIC's config.
> >>>>>
> >>>>> While the RSS key value is set again after, this is not
> >>>>> the case of the key length and the type of hash.
> >>>>>
> >>>>> There is no need to read the RSS config from the NIC, let's
> >>>>> just try to set the user defined one.
> >>>>>
> >>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> >>>>> Cc: stable@dpdk.org
> >>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>>
> >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>> ---
> >>>>>  app/test-pmd/config.c | 8 ++------
> >>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>> index 31d8ba1b91..451bda53b1 100644
> >>>>> --- a/app/test-pmd/config.c
> >>>>> +++ b/app/test-pmd/config.c
> >>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
> >>>>>  	int diag;
> >>>>>  	unsigned int i;
> >>>>>  
> >>>>> -	rss_conf.rss_key = NULL;
> >>>>> +	rss_conf.rss_key = hash_key;
> >>>>>  	rss_conf.rss_key_len = hash_key_len;
> >>>>>  	rss_conf.rss_hf = 0;
> >>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>>  	}
> >>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>> -	if (diag == 0) {
> >>>>> -		rss_conf.rss_key = hash_key;
> >>>>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>> -	}
> >>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>
> >>>> I'm not 100% sure, but I'd say the intent above could be
> >>>> to update key only as the function name says. I.e. keep
> >>>> rss_hf as is. That could be the reason to get first.
> > 
> > True,
> > 
> >>> I think that was the intial purpose of the command, but patch
> >>> 8205e241b2b0 added setting the hash type as mandatory. There are
> >>> no other command to configure the hash type from testpmd AFAICT.
> > 
> > Also for the same initial purpose, some NIC have an hash key per
> > protocol, by default it uses the same key for all of them but it can be
> > configured individually making for example key0 for all protocols expect
> > IPv4 which uses key1.
> 
> Thanks for the info, I have looked at most drivers but didn't found one
> that support this feature, could you give some pointer?

Well, I have done the modification at that time for MLX5 PMD, since I
left DPDK in 2018 I don't know if they still support such configuration
from this API or if they fully moved to rte_flow.

> Given how the drivers implément the callback, do you agree with the fix,
> or do you have something else in mind?

I cannot answer if this get() is mandatory, this predates my arrival on
DPDK (original commit written in 2014), looking at DPDK state on 
 commit f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
Maybe someone from Intel can help, eventually you can contact PMD
maintainers to review this patch.

Regards,
Nélio

> Thanks,
> Maxime
> 
> >>> Also, even without 8205e241b2b0, the function was broken because the
> >>> key length was overiden.
> >>
> >> I see, many thanks for explanations.
> > 
> 

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
> Sent: Tuesday, September 14, 2021 15:20
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
> 
> +Shahaf,
> 
> Hi Maxime,
> 
> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> > Hi Nélio,
> >
> > On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> > > On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> > >> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> > >>>
> > >>>
> > >>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> > >>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> > >>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
> > >>>>> hash type and key provided by the user, but it calls
> > >>>>> rte_eth_dev_rss_hash_conf_get() before calling
> > >>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
> > >>>>> with current NIC's config.
> > >>>>>
> > >>>>> While the RSS key value is set again after, this is not the case
> > >>>>> of the key length and the type of hash.
> > >>>>>
> > >>>>> There is no need to read the RSS config from the NIC, let's just
> > >>>>> try to set the user defined one.
> > >>>>>
> > >>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
> > >>>>> commands")
> > >>>>> Cc: stable@dpdk.org
> > >>>>> Cc: nelio.laranjeiro@6wind.com
> > >>>>>
> > >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >>>>> ---
> > >>>>>  app/test-pmd/config.c | 8 ++------
> > >>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> > >>>>>
> > >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > >>>>> 31d8ba1b91..451bda53b1 100644
> > >>>>> --- a/app/test-pmd/config.c
> > >>>>> +++ b/app/test-pmd/config.c
> > >>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
> port_id, char rss_type[], uint8_t *hash_key,
> > >>>>>  	int diag;
> > >>>>>  	unsigned int i;
> > >>>>>
> > >>>>> -	rss_conf.rss_key = NULL;
> > >>>>> +	rss_conf.rss_key = hash_key;
> > >>>>>  	rss_conf.rss_key_len = hash_key_len;
> > >>>>>  	rss_conf.rss_hf = 0;
> > >>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> > >>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> > >>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> > >>>>>  	}
> > >>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> > >>>>> -	if (diag == 0) {
> > >>>>> -		rss_conf.rss_key = hash_key;
> > >>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
> &rss_conf);
> > >>>>> -	}
> > >>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> > >>>>
> > >>>> I'm not 100% sure, but I'd say the intent above could be to
> > >>>> update key only as the function name says. I.e. keep rss_hf as
> > >>>> is. That could be the reason to get first.

+1
The intent is only updating rss key. That's why to get_rss_conf first.
At least for all intel devices. RSS key is a global value for all rss_hf.

And since the intent is to only update the key value. I don't think this patch makes sense since you're changing rss_hf which will break current test cases.
And before 8205e241b2b01c, the command is what we want. port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates key.

But if mlx has the configuration of config key for each rss_type then the code should remain to the current code in which keylen and rss_hf will be overridden to the correct value intel wants and mlx has their own configuration for specific rss type.
But to be honest, I checked mlx5. I don't see they have this kind of configuration. Need to confirm with their driver maintainer though.

But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0 does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have key update for specific rss type. Otherwise, remain what it's like wight now.

BRs
Xiaoyun

> > >
> > > True,
> > >
> > >>> I think that was the intial purpose of the command, but patch
> > >>> 8205e241b2b0 added setting the hash type as mandatory. There are
> > >>> no other command to configure the hash type from testpmd AFAICT.
> > >
> > > Also for the same initial purpose, some NIC have an hash key per
> > > protocol, by default it uses the same key for all of them but it can
> > > be configured individually making for example key0 for all protocols
> > > expect
> > > IPv4 which uses key1.
> >
> > Thanks for the info, I have looked at most drivers but didn't found
> > one that support this feature, could you give some pointer?
> 
> Well, I have done the modification at that time for MLX5 PMD, since I left DPDK
> in 2018 I don't know if they still support such configuration from this API or if
> they fully moved to rte_flow.
> 
> > Given how the drivers implément the callback, do you agree with the
> > fix, or do you have something else in mind?
> 
> I cannot answer if this get() is mandatory, this predates my arrival on DPDK
> (original commit written in 2014), looking at DPDK state on  commit
> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
> Maybe someone from Intel can help, eventually you can contact PMD
> maintainers to review this patch.
> 
> Regards,
> Nélio
> 
> > Thanks,
> > Maxime
> >
> > >>> Also, even without 8205e241b2b0, the function was broken because
> > >>> the key length was overiden.
> > >>
> > >> I see, many thanks for explanations.
> > >
> >
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Maxime Coquelin]

Hi Xiaoyun,

On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
>> Sent: Tuesday, September 14, 2021 15:20
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>
>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
>> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
>> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
>>
>> +Shahaf,
>>
>> Hi Maxime,
>>
>> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
>>> Hi Nélio,
>>>
>>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
>>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
>>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
>>>>>>
>>>>>>
>>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
>>>>>>>> hash type and key provided by the user, but it calls
>>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
>>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
>>>>>>>> with current NIC's config.
>>>>>>>>
>>>>>>>> While the RSS key value is set again after, this is not the case
>>>>>>>> of the key length and the type of hash.
>>>>>>>>
>>>>>>>> There is no need to read the RSS config from the NIC, let's just
>>>>>>>> try to set the user defined one.
>>>>>>>>
>>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
>>>>>>>> commands")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>>>>>
>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> ---
>>>>>>>>  app/test-pmd/config.c | 8 ++------
>>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>>>>> 31d8ba1b91..451bda53b1 100644
>>>>>>>> --- a/app/test-pmd/config.c
>>>>>>>> +++ b/app/test-pmd/config.c
>>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
>> port_id, char rss_type[], uint8_t *hash_key,
>>>>>>>>  	int diag;
>>>>>>>>  	unsigned int i;
>>>>>>>>
>>>>>>>> -	rss_conf.rss_key = NULL;
>>>>>>>> +	rss_conf.rss_key = hash_key;
>>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
>>>>>>>>  	rss_conf.rss_hf = 0;
>>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>>>>>  	}
>>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>>>>> -	if (diag == 0) {
>>>>>>>> -		rss_conf.rss_key = hash_key;
>>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
>> &rss_conf);
>>>>>>>> -	}
>>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>>>>
>>>>>>> I'm not 100% sure, but I'd say the intent above could be to
>>>>>>> update key only as the function name says. I.e. keep rss_hf as
>>>>>>> is. That could be the reason to get first.
> 
> +1
> The intent is only updating rss key. That's why to get_rss_conf first.
> At least for all intel devices. RSS key is a global value for all rss_hf.
> 
> And since the intent is to only update the key value. I don't think this patch makes sense since you're changing rss_hf which will break current test cases.
> And before 8205e241b2b01c, the command is what we want. port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates key.
> 
> But if mlx has the configuration of config key for each rss_type then the code should remain to the current code in which keylen and rss_hf will be overridden to the correct value intel wants and mlx has their own configuration for specific rss type.
> But to be honest, I checked mlx5. I don't see they have this kind of configuration. Need to confirm with their driver maintainer though.
> 
> But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0 does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have key update for specific rss type. Otherwise, remain what it's like wight now.

For the main branch, we could revert 8205e241b2b01c0, but in my opinion,
we need to keep the hash_key_length otherwise it could lead to out of
bounds accesses if the key passed by the user is shorter than the key
length in use by the driver.

Note that it would also imply changes in the DTS and CIs tests that make
use of this command. We would also need to introduce a new command to be
able to set the rss hash types, and rename the existing one to be key-
specific. Otherwise we have no way with testpmd to configure RSS
properly. Given there does not seem to have any driver that leverages
RSS HF/Key pairs, maybe the simple thing is to just do what I did in
this patch.

For stable, I don't think this is a good idea to change testpmd
commands, as it could break users setups.

Thanks,
Maxime

> 
> BRs
> Xiaoyun
> 
>>>>
>>>> True,
>>>>
>>>>>> I think that was the intial purpose of the command, but patch
>>>>>> 8205e241b2b0 added setting the hash type as mandatory. There are
>>>>>> no other command to configure the hash type from testpmd AFAICT.
>>>>
>>>> Also for the same initial purpose, some NIC have an hash key per
>>>> protocol, by default it uses the same key for all of them but it can
>>>> be configured individually making for example key0 for all protocols
>>>> expect
>>>> IPv4 which uses key1.
>>>
>>> Thanks for the info, I have looked at most drivers but didn't found
>>> one that support this feature, could you give some pointer?
>>
>> Well, I have done the modification at that time for MLX5 PMD, since I left DPDK
>> in 2018 I don't know if they still support such configuration from this API or if
>> they fully moved to rte_flow.
>>
>>> Given how the drivers implément the callback, do you agree with the
>>> fix, or do you have something else in mind?
>>
>> I cannot answer if this get() is mandatory, this predates my arrival on DPDK
>> (original commit written in 2014), looking at DPDK state on  commit
>> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
>> Maybe someone from Intel can help, eventually you can contact PMD
>> maintainers to review this patch.
>>
>> Regards,
>> Nélio
>>
>>> Thanks,
>>> Maxime
>>>
>>>>>> Also, even without 8205e241b2b0, the function was broken because
>>>>>> the key length was overiden.
>>>>>
>>>>> I see, many thanks for explanations.
>>>>
>>>
>>
>> --
>> Nélio Laranjeiro
>> 6WIND
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, September 16, 2021 15:34
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
> 
> Hi Xiaoyun,
> 
> On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
> >> Sent: Tuesday, September 14, 2021 15:20
> >> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> >> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> >> david.marchand@redhat.com; michaelba@nvidia.com;
> >> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
> >> <shahafs@nvidia.com>
> >> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type
> >> update
> >>
> >> +Shahaf,
> >>
> >> Hi Maxime,
> >>
> >> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> >>> Hi Nélio,
> >>>
> >>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> >>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
> >>>>>>>> hash type and key provided by the user, but it calls
> >>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
> >>>>>>>> with current NIC's config.
> >>>>>>>>
> >>>>>>>> While the RSS key value is set again after, this is not the
> >>>>>>>> case of the key length and the type of hash.
> >>>>>>>>
> >>>>>>>> There is no need to read the RSS config from the NIC, let's
> >>>>>>>> just try to set the user defined one.
> >>>>>>>>
> >>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
> >>>>>>>> commands")
> >>>>>>>> Cc: stable@dpdk.org
> >>>>>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>>>>>
> >>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>>>> ---
> >>>>>>>>  app/test-pmd/config.c | 8 ++------
> >>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>>>>> index
> >>>>>>>> 31d8ba1b91..451bda53b1 100644
> >>>>>>>> --- a/app/test-pmd/config.c
> >>>>>>>> +++ b/app/test-pmd/config.c
> >>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
> >> port_id, char rss_type[], uint8_t *hash_key,
> >>>>>>>>  	int diag;
> >>>>>>>>  	unsigned int i;
> >>>>>>>>
> >>>>>>>> -	rss_conf.rss_key = NULL;
> >>>>>>>> +	rss_conf.rss_key = hash_key;
> >>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
> >>>>>>>>  	rss_conf.rss_hf = 0;
> >>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>>>>>  	}
> >>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>>>>> -	if (diag == 0) {
> >>>>>>>> -		rss_conf.rss_key = hash_key;
> >>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
> >> &rss_conf);
> >>>>>>>> -	}
> >>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>>>>
> >>>>>>> I'm not 100% sure, but I'd say the intent above could be to
> >>>>>>> update key only as the function name says. I.e. keep rss_hf as
> >>>>>>> is. That could be the reason to get first.
> >
> > +1
> > The intent is only updating rss key. That's why to get_rss_conf first.
> > At least for all intel devices. RSS key is a global value for all rss_hf.
> >
> > And since the intent is to only update the key value. I don't think this patch
> makes sense since you're changing rss_hf which will break current test cases.
> > And before 8205e241b2b01c, the command is what we want.
> port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates
> key.
> >
> > But if mlx has the configuration of config key for each rss_type then the code
> should remain to the current code in which keylen and rss_hf will be overridden
> to the correct value intel wants and mlx has their own configuration for specific
> rss type.
> > But to be honest, I checked mlx5. I don't see they have this kind of
> configuration. Need to confirm with their driver maintainer though.
> >
> > But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0
> does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have
> key update for specific rss type. Otherwise, remain what it's like wight now.
> 
> For the main branch, we could revert 8205e241b2b01c0, but in my opinion, we
> need to keep the hash_key_length otherwise it could lead to out of bounds
> accesses if the key passed by the user is shorter than the key length in use by the
> driver.
> 
> Note that it would also imply changes in the DTS and CIs tests that make use of
> this command. We would also need to introduce a new command to be able to
> set the rss hash types, and rename the existing one to be key- specific.
> Otherwise we have no way with testpmd to configure RSS properly. Given there
> does not seem to have any driver that leverages RSS HF/Key pairs, maybe the
> simple thing is to just do what I did in this patch.
> 
> For stable, I don't think this is a good idea to change testpmd commands, as it
> could break users setups.

I don’t think so. This command is used for key setting only like the name says. And users use this command only setting key. What you did in this patch actually will break test cases results.

And change hash type you can use command "port config all rss eth|vlan|tcp|...".

> 
> Thanks,
> Maxime
> 
> >
> > BRs
> > Xiaoyun
> >
> >>>>
> >>>> True,
> >>>>
> >>>>>> I think that was the intial purpose of the command, but patch
> >>>>>> 8205e241b2b0 added setting the hash type as mandatory. There are
> >>>>>> no other command to configure the hash type from testpmd AFAICT.
> >>>>
> >>>> Also for the same initial purpose, some NIC have an hash key per
> >>>> protocol, by default it uses the same key for all of them but it
> >>>> can be configured individually making for example key0 for all
> >>>> protocols expect
> >>>> IPv4 which uses key1.
> >>>
> >>> Thanks for the info, I have looked at most drivers but didn't found
> >>> one that support this feature, could you give some pointer?
> >>
> >> Well, I have done the modification at that time for MLX5 PMD, since I
> >> left DPDK in 2018 I don't know if they still support such
> >> configuration from this API or if they fully moved to rte_flow.
> >>
> >>> Given how the drivers implément the callback, do you agree with the
> >>> fix, or do you have something else in mind?
> >>
> >> I cannot answer if this get() is mandatory, this predates my arrival
> >> on DPDK (original commit written in 2014), looking at DPDK state on
> >> commit
> >> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
> >> Maybe someone from Intel can help, eventually you can contact PMD
> >> maintainers to review this patch.
> >>
> >> Regards,
> >> Nélio
> >>
> >>> Thanks,
> >>> Maxime
> >>>
> >>>>>> Also, even without 8205e241b2b0, the function was broken because
> >>>>>> the key length was overiden.
> >>>>>
> >>>>> I see, many thanks for explanations.
> >>>>
> >>>
> >>
> >> --
> >> Nélio Laranjeiro
> >> 6WIND
> >

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Maxime Coquelin]

On 9/16/21 9:41 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, September 16, 2021 15:34
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
>> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
>> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
>> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
>>
>> Hi Xiaoyun,
>>
>> On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
>>>> Sent: Tuesday, September 14, 2021 15:20
>>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>
>>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
>>>> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>>>> david.marchand@redhat.com; michaelba@nvidia.com;
>>>> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
>>>> <shahafs@nvidia.com>
>>>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type
>>>> update
>>>>
>>>> +Shahaf,
>>>>
>>>> Hi Maxime,
>>>>
>>>> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
>>>>> Hi Nélio,
>>>>>
>>>>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
>>>>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
>>>>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>>>>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
>>>>>>>>>> hash type and key provided by the user, but it calls
>>>>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
>>>>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
>>>>>>>>>> with current NIC's config.
>>>>>>>>>>
>>>>>>>>>> While the RSS key value is set again after, this is not the
>>>>>>>>>> case of the key length and the type of hash.
>>>>>>>>>>
>>>>>>>>>> There is no need to read the RSS config from the NIC, let's
>>>>>>>>>> just try to set the user defined one.
>>>>>>>>>>
>>>>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
>>>>>>>>>> commands")
>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  app/test-pmd/config.c | 8 ++------
>>>>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>>>>>>>> index
>>>>>>>>>> 31d8ba1b91..451bda53b1 100644
>>>>>>>>>> --- a/app/test-pmd/config.c
>>>>>>>>>> +++ b/app/test-pmd/config.c
>>>>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
>>>> port_id, char rss_type[], uint8_t *hash_key,
>>>>>>>>>>  	int diag;
>>>>>>>>>>  	unsigned int i;
>>>>>>>>>>
>>>>>>>>>> -	rss_conf.rss_key = NULL;
>>>>>>>>>> +	rss_conf.rss_key = hash_key;
>>>>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
>>>>>>>>>>  	rss_conf.rss_hf = 0;
>>>>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>>>>>>>  	}
>>>>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>>>>>>> -	if (diag == 0) {
>>>>>>>>>> -		rss_conf.rss_key = hash_key;
>>>>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
>>>> &rss_conf);
>>>>>>>>>> -	}
>>>>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>>>>>>
>>>>>>>>> I'm not 100% sure, but I'd say the intent above could be to
>>>>>>>>> update key only as the function name says. I.e. keep rss_hf as
>>>>>>>>> is. That could be the reason to get first.
>>>
>>> +1
>>> The intent is only updating rss key. That's why to get_rss_conf first.
>>> At least for all intel devices. RSS key is a global value for all rss_hf.
>>>
>>> And since the intent is to only update the key value. I don't think this patch
>> makes sense since you're changing rss_hf which will break current test cases.
>>> And before 8205e241b2b01c, the command is what we want.
>> port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates
>> key.
>>>
>>> But if mlx has the configuration of config key for each rss_type then the code
>> should remain to the current code in which keylen and rss_hf will be overridden
>> to the correct value intel wants and mlx has their own configuration for specific
>> rss type.
>>> But to be honest, I checked mlx5. I don't see they have this kind of
>> configuration. Need to confirm with their driver maintainer though.
>>>
>>> But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0
>> does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have
>> key update for specific rss type. Otherwise, remain what it's like wight now.
>>
>> For the main branch, we could revert 8205e241b2b01c0, but in my opinion, we
>> need to keep the hash_key_length otherwise it could lead to out of bounds
>> accesses if the key passed by the user is shorter than the key length in use by the
>> driver.
>>
>> Note that it would also imply changes in the DTS and CIs tests that make use of
>> this command. We would also need to introduce a new command to be able to
>> set the rss hash types, and rename the existing one to be key- specific.
>> Otherwise we have no way with testpmd to configure RSS properly. Given there
>> does not seem to have any driver that leverages RSS HF/Key pairs, maybe the
>> simple thing is to just do what I did in this patch.
>>
>> For stable, I don't think this is a good idea to change testpmd commands, as it
>> could break users setups.
> 
> I don’t think so. This command is used for key setting only like the name says. And users use this command only setting key. What you did in this patch actually will break test cases results.
> 
> And change hash type you can use command "port config all rss eth|vlan|tcp|...".

Ok, I did not find this command as I was trying completion with 'port
config 0 rss'. It might be good to to change it so that we can configure
it per port, but not a big deal.

Remains the key length that should be passed, otherwise can have out of
bounds accesses, no? And we still need to update DTS if we revert
8205e241b2b01c0.

Thanks,
Maxime

> 
>>
>> Thanks,
>> Maxime
>>
>>>
>>> BRs
>>> Xiaoyun
>>>
>>>>>>
>>>>>> True,
>>>>>>
>>>>>>>> I think that was the intial purpose of the command, but patch
>>>>>>>> 8205e241b2b0 added setting the hash type as mandatory. There are
>>>>>>>> no other command to configure the hash type from testpmd AFAICT.
>>>>>>
>>>>>> Also for the same initial purpose, some NIC have an hash key per
>>>>>> protocol, by default it uses the same key for all of them but it
>>>>>> can be configured individually making for example key0 for all
>>>>>> protocols expect
>>>>>> IPv4 which uses key1.
>>>>>
>>>>> Thanks for the info, I have looked at most drivers but didn't found
>>>>> one that support this feature, could you give some pointer?
>>>>
>>>> Well, I have done the modification at that time for MLX5 PMD, since I
>>>> left DPDK in 2018 I don't know if they still support such
>>>> configuration from this API or if they fully moved to rte_flow.
>>>>
>>>>> Given how the drivers implément the callback, do you agree with the
>>>>> fix, or do you have something else in mind?
>>>>
>>>> I cannot answer if this get() is mandatory, this predates my arrival
>>>> on DPDK (original commit written in 2014), looking at DPDK state on
>>>> commit
>>>> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
>>>> Maybe someone from Intel can help, eventually you can contact PMD
>>>> maintainers to review this patch.
>>>>
>>>> Regards,
>>>> Nélio
>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>>
>>>>>>>> Also, even without 8205e241b2b0, the function was broken because
>>>>>>>> the key length was overiden.
>>>>>>>
>>>>>>> I see, many thanks for explanations.
>>>>>>
>>>>>
>>>>
>>>> --
>>>> Nélio Laranjeiro
>>>> 6WIND
>>>
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, September 16, 2021 16:09
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
> 
> 
> 
> On 9/16/21 9:41 AM, Li, Xiaoyun wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Thursday, September 16, 2021 15:34
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
> >> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> >> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> >> david.marchand@redhat.com; michaelba@nvidia.com;
> >> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
> >> <shahafs@nvidia.com>
> >> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type
> >> update
> >>
> >> Hi Xiaoyun,
> >>
> >> On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio
> >>>> Laranjeiro
> >>>> Sent: Tuesday, September 14, 2021 15:20
> >>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
> >>>> <ferruh.yigit@intel.com>
> >>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> >>>> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> >>>> david.marchand@redhat.com; michaelba@nvidia.com;
> >>>> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
> >>>> <shahafs@nvidia.com>
> >>>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash
> >>>> type update
> >>>>
> >>>> +Shahaf,
> >>>>
> >>>> Hi Maxime,
> >>>>
> >>>> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> >>>>> Hi Nélio,
> >>>>>
> >>>>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> >>>>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >>>>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
> >>>>>>>>>> hash type and key provided by the user, but it calls
> >>>>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>>>>>>>>> config with current NIC's config.
> >>>>>>>>>>
> >>>>>>>>>> While the RSS key value is set again after, this is not the
> >>>>>>>>>> case of the key length and the type of hash.
> >>>>>>>>>>
> >>>>>>>>>> There is no need to read the RSS config from the NIC, let's
> >>>>>>>>>> just try to set the user defined one.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS
> >>>>>>>>>> hash
> >>>>>>>>>> commands")
> >>>>>>>>>> Cc: stable@dpdk.org
> >>>>>>>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  app/test-pmd/config.c | 8 ++------
> >>>>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>>>>>>> index
> >>>>>>>>>> 31d8ba1b91..451bda53b1 100644
> >>>>>>>>>> --- a/app/test-pmd/config.c
> >>>>>>>>>> +++ b/app/test-pmd/config.c
> >>>>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
> >>>> port_id, char rss_type[], uint8_t *hash_key,
> >>>>>>>>>>  	int diag;
> >>>>>>>>>>  	unsigned int i;
> >>>>>>>>>>
> >>>>>>>>>> -	rss_conf.rss_key = NULL;
> >>>>>>>>>> +	rss_conf.rss_key = hash_key;
> >>>>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
> >>>>>>>>>>  	rss_conf.rss_hf = 0;
> >>>>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>>>>>>>  	}
> >>>>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>>>>>>> -	if (diag == 0) {
> >>>>>>>>>> -		rss_conf.rss_key = hash_key;
> >>>>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
> >>>> &rss_conf);
> >>>>>>>>>> -	}
> >>>>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>>>>>>
> >>>>>>>>> I'm not 100% sure, but I'd say the intent above could be to
> >>>>>>>>> update key only as the function name says. I.e. keep rss_hf as
> >>>>>>>>> is. That could be the reason to get first.
> >>>
> >>> +1
> >>> The intent is only updating rss key. That's why to get_rss_conf first.
> >>> At least for all intel devices. RSS key is a global value for all rss_hf.
> >>>
> >>> And since the intent is to only update the key value. I don't think
> >>> this patch
> >> makes sense since you're changing rss_hf which will break current test cases.
> >>> And before 8205e241b2b01c, the command is what we want.
> >> port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only
> >> updates key.
> >>>
> >>> But if mlx has the configuration of config key for each rss_type
> >>> then the code
> >> should remain to the current code in which keylen and rss_hf will be
> >> overridden to the correct value intel wants and mlx has their own
> >> configuration for specific rss type.
> >>> But to be honest, I checked mlx5. I don't see they have this kind of
> >> configuration. Need to confirm with their driver maintainer though.
> >>>
> >>> But anyway, from my point of view, I prefer to revert what
> >>> 8205e241b2b01c0
> >> does and remove rss_hf, rss_key_len setting in this command if mlx
> >> doesn't have key update for specific rss type. Otherwise, remain what it's like
> wight now.
> >>
> >> For the main branch, we could revert 8205e241b2b01c0, but in my
> >> opinion, we need to keep the hash_key_length otherwise it could lead
> >> to out of bounds accesses if the key passed by the user is shorter
> >> than the key length in use by the driver.
> >>
> >> Note that it would also imply changes in the DTS and CIs tests that
> >> make use of this command. We would also need to introduce a new
> >> command to be able to set the rss hash types, and rename the existing one to
> be key- specific.
> >> Otherwise we have no way with testpmd to configure RSS properly.
> >> Given there does not seem to have any driver that leverages RSS
> >> HF/Key pairs, maybe the simple thing is to just do what I did in this patch.
> >>
> >> For stable, I don't think this is a good idea to change testpmd
> >> commands, as it could break users setups.
> >
> > I don’t think so. This command is used for key setting only like the name says.
> And users use this command only setting key. What you did in this patch actually
> will break test cases results.
> >
> > And change hash type you can use command "port config all rss
> eth|vlan|tcp|...".
> 
> Ok, I did not find this command as I was trying completion with 'port config 0
> rss'. It might be good to to change it so that we can configure it per port, but
> not a big deal.
> 
> Remains the key length that should be passed, otherwise can have out of bounds
> accesses, no? And we still need to update DTS if we revert 8205e241b2b01c0.

The key length will be overridden after calling hash_conf_get so I think it doesn’t need to be passed.
It's useful before calling port_rss_hash_key_update() for "key" check.
And I agree with the breaking test cases for cmdlines, what about just remove the useless setting key_len and rss_hf assignment since they will be overridden anyway?
But rss_hf that users set is not taking effect anyway. Want to clean it but we can't. Tricky.

But I don't have a strong opinion to revert that patch.

> 
> Thanks,
> Maxime
> 
> >
> >>
> >> Thanks,
> >> Maxime
> >>
> >>>
> >>> BRs
> >>> Xiaoyun
> >>>
> >>>>>>
> >>>>>> True,
> >>>>>>
> >>>>>>>> I think that was the intial purpose of the command, but patch
> >>>>>>>> 8205e241b2b0 added setting the hash type as mandatory. There
> >>>>>>>> are no other command to configure the hash type from testpmd
> AFAICT.
> >>>>>>
> >>>>>> Also for the same initial purpose, some NIC have an hash key per
> >>>>>> protocol, by default it uses the same key for all of them but it
> >>>>>> can be configured individually making for example key0 for all
> >>>>>> protocols expect
> >>>>>> IPv4 which uses key1.
> >>>>>
> >>>>> Thanks for the info, I have looked at most drivers but didn't
> >>>>> found one that support this feature, could you give some pointer?
> >>>>
> >>>> Well, I have done the modification at that time for MLX5 PMD, since
> >>>> I left DPDK in 2018 I don't know if they still support such
> >>>> configuration from this API or if they fully moved to rte_flow.
> >>>>
> >>>>> Given how the drivers implément the callback, do you agree with
> >>>>> the fix, or do you have something else in mind?
> >>>>
> >>>> I cannot answer if this get() is mandatory, this predates my
> >>>> arrival on DPDK (original commit written in 2014), looking at DPDK
> >>>> state on commit
> >>>> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
> >>>> Maybe someone from Intel can help, eventually you can contact PMD
> >>>> maintainers to review this patch.
> >>>>
> >>>> Regards,
> >>>> Nélio
> >>>>
> >>>>> Thanks,
> >>>>> Maxime
> >>>>>
> >>>>>>>> Also, even without 8205e241b2b0, the function was broken
> >>>>>>>> because the key length was overiden.
> >>>>>>>
> >>>>>>> I see, many thanks for explanations.
> >>>>>>
> >>>>>
> >>>>
> >>>> --
> >>>> Nélio Laranjeiro
> >>>> 6WIND
> >>>
> >

[dpdk-dev] [PATCH 3/3] net/mlx5: Fix RSS RETA update

[Maxime Coquelin]

This patch fixes RETA updating for entries above 64.
Without ithat, these entries are never updated as
calculated mask value will always be 0.

Fixes: 634efbc2c8c0 ("mlx5: support RETA query and update")
Cc: stable@dpdk.org
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/mlx5/mlx5_rss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c
index c32129cdc2..6dc52acee0 100644
--- a/drivers/net/mlx5/mlx5_rss.c
+++ b/drivers/net/mlx5/mlx5_rss.c
@@ -211,7 +211,7 @@ mlx5_dev_rss_reta_update(struct rte_eth_dev *dev,
 	for (idx = 0, i = 0; (i != reta_size); ++i) {
 		idx = i / RTE_RETA_GROUP_SIZE;
 		pos = i % RTE_RETA_GROUP_SIZE;
-		if (((reta_conf[idx].mask >> i) & 0x1) == 0)
+		if (((reta_conf[idx].mask >> pos) & 0x1) == 0)
 			continue;
 		MLX5_ASSERT(reta_conf[idx].reta[pos] < priv->rxqs_n);
 		(*priv->reta_idx)[i] = reta_conf[idx].reta[pos];
-- 
2.31.1

Re: [dpdk-dev] [PATCH 3/3] net/mlx5: Fix RSS RETA update

[Slava Ovsiienko]

Hi, Maxime

Very nice catch, thank you for the patch.
Minor typo in the commit message "ithat" -> "that".
Besides this:

Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>


> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, September 10, 2021 12:18
> To: dev@dpdk.org; chenbo.xia@intel.com; amorenoz@redhat.com;
> david.marchand@redhat.com; andrew.rybchenko@oktetlabs.ru;
> ferruh.yigit@intel.com; Michael Baum <michaelba@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>
> Cc: stable@dpdk.org; NBU-Contact-N?lio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: [PATCH 3/3] net/mlx5: Fix RSS RETA update
> 
> This patch fixes RETA updating for entries above 64.
> Without ithat, these entries are never updated as calculated mask value will
> always be 0.
> 
> Fixes: 634efbc2c8c0 ("mlx5: support RETA query and update")
> Cc: stable@dpdk.org
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/mlx5/mlx5_rss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c index
> c32129cdc2..6dc52acee0 100644
> --- a/drivers/net/mlx5/mlx5_rss.c
> +++ b/drivers/net/mlx5/mlx5_rss.c
> @@ -211,7 +211,7 @@ mlx5_dev_rss_reta_update(struct rte_eth_dev *dev,
>  	for (idx = 0, i = 0; (i != reta_size); ++i) {
>  		idx = i / RTE_RETA_GROUP_SIZE;
>  		pos = i % RTE_RETA_GROUP_SIZE;
> -		if (((reta_conf[idx].mask >> i) & 0x1) == 0)
> +		if (((reta_conf[idx].mask >> pos) & 0x1) == 0)
>  			continue;
>  		MLX5_ASSERT(reta_conf[idx].reta[pos] < priv->rxqs_n);
>  		(*priv->reta_idx)[i] = reta_conf[idx].reta[pos];
> --
> 2.31.1