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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/18/21 1:20 PM, Maxime Coquelin wrote:
> 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>

See review notes below

> diff --git a/drivers/net/virtio/virtio.h b/drivers/net/virtio/virtio.h
> index e78b2e429e..7118e5d24c 100644
> --- a/drivers/net/virtio/virtio.h
> +++ b/drivers/net/virtio/virtio.h

[snip]

> @@ -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)

I think it is a bit better to use RTE_BIT32() above.

[snip]

> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index aff791fbd0..a8e2bf3e1a 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c

[snip]

> @@ -347,20 +357,51 @@ 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;
> -	int dlen[1];
> +	struct virtio_net_ctrl_rss rss;
> +	int dlen, ret;
> +
> +	rss.hash_types = hw->rss_hash_types & VIRTIO_NET_HASH_TYPE_MASK;

RTE_BUILD_BUG_ON(!RTE_IS_POWER_OF_2(VIRTIO_NET_RSS_RETA_SIZE));

> +	rss.indirection_table_mask = VIRTIO_NET_RSS_RETA_SIZE - 1;

It relies on the fact that device is power of 2.
So, suggest to add above check.

> +	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;

Is it guaranteed that driver is configured with equal number
of Rx and Tx queues? Or is it not a problem otherwise?

[snip]

> +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) {

Shouldn't it be
config->rss_max_key_size != VIRTIO_NET_RSS_KEY_SIZE ?
Or do we just ensure that HW supports at least required key
size and rely on the fact that it will reject set request later
if our size is not supported in fact?

> +		PMD_INIT_LOG(ERR, "Invalid device RSS max key size (%u)",
> +				config->rss_max_key_size);
> +		return -EINVAL;
> +	}
> +
> +	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) {

Same question here.

> +		PMD_INIT_LOG(ERR, "Invalid device RSS max reta size (%u)",
> +				config->rss_max_indirection_table_length);
> +		return -EINVAL;
> +	}
> +
> +	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 (0x%x)",
> +				config->supported_hash_types);
> +		return -EINVAL;
> +	}
> +
> +	*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 = ethdev_to_virtio_rss_offloads(rss_conf->rss_hf);
> +
> +	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);

Don't we need to rollback data in hw in the case of failure?

[snip]

> +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_size != VIRTIO_NET_RSS_RETA_SIZE)
> +		return -EINVAL;
> +
> +	for (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);

Question about rollback in the case of failure stands here as
well.

> +}

[snip]


> +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;
> +	struct rte_eth_rss_conf *rss_conf;
> +	int ret, i;
> +
> +	rss_conf = &eth_dev->data->dev_conf.rx_adv_conf.rss_conf;
> +
> +	ret = virtio_dev_get_rss_config(hw, &hw->rss_hash_types);
> +	if (ret)
> +		return ret;
> +
> +	if (rss_conf->rss_hf) {
> +		/*  Ensure requested hash types are supported by the device */
> +		if (rss_conf->rss_hf & ~virtio_to_ethdev_rss_offloads(hw->rss_hash_types))
> +			return -EINVAL;
> +
> +		hw->rss_hash_types = ethdev_to_virtio_rss_offloads(rss_conf->rss_hf);
> +	}
> +
> +	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;
> +		}
> +
> +		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);
> +		} else {
> +			memcpy(hw->rss_key, rss_intel_key, VIRTIO_NET_RSS_KEY_SIZE);
> +		}

Above if should work in the case of reconfigure as well when
array is already allocated.

> +	}
> +
> +	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;

How should it work in the case of reconfigure if a nubmer of Rx
queue changes?

Also I'm wondering how it works...
virtio_dev_rss_init() is called from eth_virtio_dev_init() as
well when a number of Rx queues is zero. I guess the reason is
VIRTIO_PMD_DEFAULT_GUEST_FEATURES, but it looks very fragile.

> +	}
> +
> +	return 0;
> +}
> +

[snip]

> @@ -2107,6 +2465,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);

RTE_BIT64

> +
>  	if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) &&
>  	    (rxmode->max_rx_pkt_len > hw->max_mtu + ether_hdr_len))
>  		req_features &= ~(1ULL << VIRTIO_NET_F_MTU);

[snip]

> @@ -2578,6 +2946,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)) {

RTE_BIT64

> +		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;
> +	}
> +
>  
>  	if (host_features & (1ULL << VIRTIO_F_RING_PACKED)) {
>  		/*
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 40be484218..c08f382791 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)

IMHO it should be converted to use RTE_BIT64().
Yes, separate story, but right now it looks
confusing to see 1u and 1ULL above.

>  
>  extern const struct eth_dev_ops virtio_user_secondary_eth_dev_ops;
>  
[

[snip]