On 2025/03/21 19:44, Yuri Benditovich wrote:
On Fri, Mar 21, 2025 at 11:56 AM Akihiko Odaki <[email protected]> wrote:
virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to
determine the number of queue pairs and emits an error message saying
"Can't get queue_pairs". However, the field tells only about tx.
Examine the indirection table to determine the number of queues required
for rx, and correct the name of field in the error message, clarifying
its correct semantics.
Signed-off-by: Akihiko Odaki <[email protected]>
---
hw/net/virtio-net.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de87cfadffe1..d9ab9e1eb74d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1450,23 +1450,28 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
err_value = (uint32_t)s;
goto error;
}
- for (i = 0; i < n->rss_data.indirections_len; ++i) {
- uint16_t val = n->rss_data.indirections_table[i];
- n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
- }
offset += size_get;
size_get = sizeof(temp);
s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get);
if (s != size_get) {
- err_msg = "Can't get queue_pairs";
+ err_msg = "Can't get max_tx_vq";
err_value = (uint32_t)s;
goto error;
}
- queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs;
- if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
- err_msg = "Invalid number of queue_pairs";
- err_value = queue_pairs;
- goto error;
+ if (do_rss) {
+ queue_pairs = virtio_lduw_p(vdev, &temp.us);
+ for (i = 0; i < n->rss_data.indirections_len; ++i) {
+ uint16_t val = n->rss_data.indirections_table[i];
+ n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
+ queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]);
+ }
I think this change will create a problem rather than improve something.
curr_queue_pairs is used in virtio_net_can_receive.
Let's say the device has 4 queues and 4 CPUs.
If the OS currently configures the RSS for first 2 queues only the
curr_queue_pairs becomes 2.
The packets that come to queues 2 and 3 will be dropped, see
virtio_net_receive_rcu and virtio_net_can_receive
IMO the curr_queue_pairs should not be changed after it was set by
VIRTIO_NET_CTRL_MQ / VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET
VIRTIO_NET_CTRL_MQ_RSS_CONFIG is part of class VIRTIO_NET_CTRL_MQ and
should be used for configurable receive steering while
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET should be used for automatic receive
steering.
5.1.6.5.5 "Device operation in multiqueue mode" says:
> The driver enables multiqueue by sending a command using class
> VIRTIO_NET_CTRL_MQ. The command selects the mode of multiqueue
> operation, as follows:
>
> #define VIRTIO_NET_CTRL_MQ 4
> #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET 0 (for automatic
> receive steering)
> #define VIRTIO_NET_CTRL_MQ_RSS_CONFIG 1 (for configurable
> receive steering)
> #define VIRTIO_NET_CTRL_MQ_HASH_CONFIG 2 (for configurable
> hash calculation)
>
> If more than one multiqueue mode is negotiated, the resulting device
> configuration is defined by the last command sent by the driver.
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET may not be even available if
VIRTIO_NET_F_RSS is negotiated as VIRTIO_NET_F_RSS does not depend on
VIRTIO_NET_F_MQ.
Jason, as you are participating in a relevant discussion on LKML, I'd
also like to ask you if you have anything to add.
Regards,
Akihiko Odaki
+ if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
+ err_msg = "Invalid number of queue_pairs";
+ err_value = queue_pairs;
+ goto error;
+ }
+ } else {
+ queue_pairs = n->curr_queue_pairs;
}
if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
err_msg = "Invalid key size";
---
base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
change-id: 20250321-vq-87aff4f531bf
Best regards,
--
Akihiko Odaki <[email protected]>
