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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/23/21 09:35, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, September 22, 2021 5:58 PM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>> david.marchand@redhat.com; andrew.rybchenko@oktetlabs.ru; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; michaelba@nvidia.com; viacheslavo@nvidia.com; Li,
>> Xiaoyun <xiaoyun.li@intel.com>
>> Cc: stable@dpdk.org; nelio.laranjeiro@6wind.com; yvugenfi@redhat.com;
>> ybendito@redhat.com; Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH v2 1/5] net/virtio: add initial RSS support
>>
>> Provide 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    |   3 +
>>   doc/guides/nics/virtio.rst             |   3 +
>>   doc/guides/rel_notes/release_21_11.rst |   6 +
>>   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, 428 insertions(+), 6 deletions(-)
>>
>> diff --git a/doc/guides/nics/features/virtio.ini
>> b/doc/guides/nics/features/virtio.ini
>> index 48f6f393b1..a5eab4932f 100644
>> --- a/doc/guides/nics/features/virtio.ini
>> +++ b/doc/guides/nics/features/virtio.ini
>> @@ -14,6 +14,9 @@ Promiscuous mode     = Y
>>   Allmulticast mode    = Y
>>   Unicast MAC filter   = Y
>>   Multicast MAC filter = Y
>> +RSS hash             = P
>> +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..98e0d012b7 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 length, 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 f5d16993db..2f9d81926b 100644
>> --- a/doc/guides/rel_notes/release_21_11.rst
>> +++ b/doc/guides/rel_notes/release_21_11.rst
>> @@ -96,6 +96,12 @@ New Features
>>     Added command-line options to specify total number of processes and
>>     current process ID. Each process owns subset of Rx and Tx queues.
>>
>> +* **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)
> 
> Spec uses 'v' instead of 'V' for macro definition.
> Better to align it?

I actually put it in uper case on purpose. I don't have a strong opinion
on this, but prefer to keep defines in upper case.

The spec lacks consistency on this, as VIRTIO_NET_HDR_GSO_TCPV6 is all
upper case for example.

>> +#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 da1633d77e..10a9f708eb 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -51,6 +51,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);
>> @@ -347,7 +357,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;
> 
> Do we really need 'int dlen[1]' rather than 'int dlen'?
> 

No we don't, did that for consistency with
virtio_set_multiple_queues_auto, but I'm fixing both occurences in new
revision.

>> +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;
> 
> No need to check, it's done in ethdev.

OK, removing it here and elsewhere.

...
>> +/**
>> + * RSS control
>> + *
>> + * The RSS feature <todo>
>> + */
>> +#define VIRTIO_NET_RSS_RETA_SIZE 128
>> +#define VIRTIO_NET_RSS_KEY_SIZE       40
> 
> Better align the numbers?

Will do.

Thanks!
Maxime

> Thanks,
> Chenbo