Hi,
On 10/19/21 11:37, Andrew Rybchenko wrote:
Hi Maxime,
On 10/19/21 12:22 PM, Maxime Coquelin wrote:
Hi Andrew,
On 10/19/21 09:30, Andrew Rybchenko wrote:
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.coque...@redhat.com>
[snip]
+ 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?
Virtio networking devices works with queue pairs.
But it seems to me that I still can configure just 1 Rx queue
and many Tx queues. Basically just non equal.
The line is suspicious since I'd expect nb_queues to be
a number of Rx queues in the function, but we set max_tx_vq
count here.
The Virtio spec says:
"
A driver sets max_tx_vq to inform a device how many transmit
virtqueues it may use (transmitq1. . .transmitq max_tx_vq).
"
But looking at Qemu side, its value is interpreted as the number of
queue pairs setup by the driver, same as is handled virtqueue_pairs of
struct virtio_net_ctrl_mq when RSS is not supported.
In this patch we are compatible with what is done in Qemu, and what is
done for multiqueue when RSS is not enabled.
I don't get why the spec talks about transmit queues, Yan & Yuri, any
idea?
+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 = ð_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.
I'm not sure, because rte_eth_dev_rss_hash_update() API does not update
eth_dev->data->dev_conf.rx_adv_conf.rss_conf, so we may lose the RSS key
the user would have updated using this API. What do you think?
I think, but I think it should be used correctly by the
application. I.e. if application does reconfigure and
provides same or new key, does not matter, it should be
applied. Key could be a part of configure request and we
should not ignore it in the case of reconfigure.
Yes, I agree that it is a bit tricky, but still right
logic.
Ok. I'm applying your suggestion, which is to apply rss_conf whatever
rss_key was initialized or not before.
+ }
+
+ 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?
Hmm, good catch! I did not think about this, and so did not test it.
Not to lose user-provisionned reta in case of unrelated change, maybe I
should save the number of Rx queues when setting the reta (here and in
virtio_dev_rss_reta_update), and perform a re-initialization if it is
different?
Yes, it is much harder than RSS key case since the data are not
provided in dev_conf explicitly. So, we should guess here what
to do. The simple solution is to reinitialize if number of RxQs
change. I'd go this way.
OK, this will be done in next revision.
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.
Yes, we only add VIRTIO_NET_F_RSS to the supported features in
virtio_dev_configure(), if Rx MQ mode is RSS. So virtio_dev_rss_init()
will never be called from eth_virtio_dev_init().
If I'm not mistaken, rte_eth_dev_configure() must be called it is stated
in the API documentation, so it will be negotiated if conditions are
met.
As far as I know we can configure ethdev with zero number of
RxQs, but non-zero number of Tx queues. E.g. traffic generator
usecase.
Division by zero is a guaranteed crash, so, I'd care
about it explicitly in any case. Just do not try to rely
on other checks faraway.
Sure, adding a check on nb_rx_queues before doing any RSS init.
Thanks,
Maxime
Andrew.
