[PATCH] virtio-net: Fix the interpretation of max_tx_vq

[PATCH] virtio-net: Fix the interpretation of max_tx_vq

[Akihiko Odaki]

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 <akihiko.odaki@daynix.com>
---
 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]);
+        }
+        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 <akihiko.odaki@daynix.com>

Re: [PATCH] virtio-net: Fix the interpretation of max_tx_vq

[Yuri Benditovich]

On Fri, Mar 21, 2025 at 11:56 AM Akihiko Odaki <akihiko.odaki@daynix.com> 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 <akihiko.odaki@daynix.com>
> ---
>  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

> +        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 <akihiko.odaki@daynix.com>
>

Re: [PATCH] virtio-net: Fix the interpretation of max_tx_vq

[Akihiko Odaki]

On 2025/03/21 19:44, Yuri Benditovich wrote:
> On Fri, Mar 21, 2025 at 11:56 AM Akihiko Odaki <akihiko.odaki@daynix.com> 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 <akihiko.odaki@daynix.com>
>> ---
>>   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 <akihiko.odaki@daynix.com>
>>