[PATCH] virtio-net: Add queues for RSS during migration

[PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki]

virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
VIRTIO_NET_F_MQ uses bit 22.

Instead of inferring the required number of queues from
vdev->guest_features, use the number loaded from the vm state.

Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/virtio/virtio.h |  2 +-
 hw/net/virtio-net.c        | 11 ++++-------
 hw/virtio/virtio.c         | 14 +++++++-------
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 638691028050..af52580c1e63 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -211,7 +211,7 @@ struct VirtioDeviceClass {
     int (*start_ioeventfd)(VirtIODevice *vdev);
     void (*stop_ioeventfd)(VirtIODevice *vdev);
     /* Called before loading queues. Useful to add queues before loading. */
-    int (*pre_load_queues)(VirtIODevice *vdev);
+    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
     /* Saving and loading of a device; trying to deprecate save/load
      * use vmsd for new devices.
      */
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de87cfadffe1..c25c6cf54183 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3026,11 +3026,10 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
     virtio_del_queue(vdev, index * 2 + 1);
 }
 
-static void virtio_net_change_num_queue_pairs(VirtIONet *n, int new_max_queue_pairs)
+static void virtio_net_change_num_queues(VirtIONet *n, int new_num_queues)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
     int old_num_queues = virtio_get_num_queues(vdev);
-    int new_num_queues = new_max_queue_pairs * 2 + 1;
     int i;
 
     assert(old_num_queues >= 3);
@@ -3066,16 +3065,14 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
     int max = multiqueue ? n->max_queue_pairs : 1;
 
     n->multiqueue = multiqueue;
-    virtio_net_change_num_queue_pairs(n, max);
+    virtio_net_change_num_queues(n, max * 2 + 1);
 
     virtio_net_set_queue_pairs(n);
 }
 
-static int virtio_net_pre_load_queues(VirtIODevice *vdev)
+static int virtio_net_pre_load_queues(VirtIODevice *vdev, uint32_t n)
 {
-    virtio_net_set_multiqueue(VIRTIO_NET(vdev),
-                              virtio_has_feature(vdev->guest_features, VIRTIO_NET_F_RSS) ||
-                              virtio_has_feature(vdev->guest_features, VIRTIO_NET_F_MQ));
+    virtio_net_change_num_queues(VIRTIO_NET(vdev), n);
 
     return 0;
 }
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 85110bce3744..286648fe9b60 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3255,13 +3255,6 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         config_len--;
     }
 
-    if (vdc->pre_load_queues) {
-        ret = vdc->pre_load_queues(vdev);
-        if (ret) {
-            return ret;
-        }
-    }
-
     num = qemu_get_be32(f);
 
     if (num > VIRTIO_QUEUE_MAX) {
@@ -3269,6 +3262,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         return -1;
     }
 
+    if (vdc->pre_load_queues) {
+        ret = vdc->pre_load_queues(vdev, num);
+        if (ret) {
+            return ret;
+        }
+    }
+
     for (i = 0; i < num; i++) {
         vdev->vq[i].vring.num = qemu_get_be32(f);
         if (k->has_variable_vring_alignment) {

---
base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
change-id: 20250406-n-ae7be0389382

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Lei Yang]

I tested this patch with virtio-net regression tests, everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>


On Sat, May 10, 2025 at 3:25 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
> VIRTIO_NET_F_MQ uses bit 22.
>
> Instead of inferring the required number of queues from
> vdev->guest_features, use the number loaded from the vm state.
>
> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  include/hw/virtio/virtio.h |  2 +-
>  hw/net/virtio-net.c        | 11 ++++-------
>  hw/virtio/virtio.c         | 14 +++++++-------
>  3 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 638691028050..af52580c1e63 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>      int (*start_ioeventfd)(VirtIODevice *vdev);
>      void (*stop_ioeventfd)(VirtIODevice *vdev);
>      /* Called before loading queues. Useful to add queues before loading. */
> -    int (*pre_load_queues)(VirtIODevice *vdev);
> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
>      /* Saving and loading of a device; trying to deprecate save/load
>       * use vmsd for new devices.
>       */
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index de87cfadffe1..c25c6cf54183 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3026,11 +3026,10 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
>      virtio_del_queue(vdev, index * 2 + 1);
>  }
>
> -static void virtio_net_change_num_queue_pairs(VirtIONet *n, int new_max_queue_pairs)
> +static void virtio_net_change_num_queues(VirtIONet *n, int new_num_queues)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
>      int old_num_queues = virtio_get_num_queues(vdev);
> -    int new_num_queues = new_max_queue_pairs * 2 + 1;
>      int i;
>
>      assert(old_num_queues >= 3);
> @@ -3066,16 +3065,14 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
>      int max = multiqueue ? n->max_queue_pairs : 1;
>
>      n->multiqueue = multiqueue;
> -    virtio_net_change_num_queue_pairs(n, max);
> +    virtio_net_change_num_queues(n, max * 2 + 1);
>
>      virtio_net_set_queue_pairs(n);
>  }
>
> -static int virtio_net_pre_load_queues(VirtIODevice *vdev)
> +static int virtio_net_pre_load_queues(VirtIODevice *vdev, uint32_t n)
>  {
> -    virtio_net_set_multiqueue(VIRTIO_NET(vdev),
> -                              virtio_has_feature(vdev->guest_features, VIRTIO_NET_F_RSS) ||
> -                              virtio_has_feature(vdev->guest_features, VIRTIO_NET_F_MQ));
> +    virtio_net_change_num_queues(VIRTIO_NET(vdev), n);
>
>      return 0;
>  }
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 85110bce3744..286648fe9b60 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3255,13 +3255,6 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          config_len--;
>      }
>
> -    if (vdc->pre_load_queues) {
> -        ret = vdc->pre_load_queues(vdev);
> -        if (ret) {
> -            return ret;
> -        }
> -    }
> -
>      num = qemu_get_be32(f);
>
>      if (num > VIRTIO_QUEUE_MAX) {
> @@ -3269,6 +3262,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          return -1;
>      }
>
> +    if (vdc->pre_load_queues) {
> +        ret = vdc->pre_load_queues(vdev, num);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
>      for (i = 0; i < num; i++) {
>          vdev->vq[i].vring.num = qemu_get_be32(f);
>          if (k->has_variable_vring_alignment) {
>
> ---
> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
> change-id: 20250406-n-ae7be0389382
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
>
>

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Jason Wang]

On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
> VIRTIO_NET_F_MQ uses bit 22.
>
> Instead of inferring the required number of queues from
> vdev->guest_features, use the number loaded from the vm state.
>
> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  include/hw/virtio/virtio.h |  2 +-
>  hw/net/virtio-net.c        | 11 ++++-------
>  hw/virtio/virtio.c         | 14 +++++++-------
>  3 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 638691028050..af52580c1e63 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>      int (*start_ioeventfd)(VirtIODevice *vdev);
>      void (*stop_ioeventfd)(VirtIODevice *vdev);
>      /* Called before loading queues. Useful to add queues before loading. */
> -    int (*pre_load_queues)(VirtIODevice *vdev);
> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);

This turns out to be tricky as it has a lot of assumptions as
described in the changelog (e.g only lower 32bit of guest_features is
correct etc when calling this function).

Looking at the commit that introduces this which is 9379ea9db3c that says:

"""
Otherwise the loaded queues will not have handlers and elements
in them will not be processed.
"""

I fail to remember what it means or what happens if we don't do 9379ea9db3c.

Thanks

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki]

On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
>> VIRTIO_NET_F_MQ uses bit 22.
>>
>> Instead of inferring the required number of queues from
>> vdev->guest_features, use the number loaded from the vm state.
>>
>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   include/hw/virtio/virtio.h |  2 +-
>>   hw/net/virtio-net.c        | 11 ++++-------
>>   hw/virtio/virtio.c         | 14 +++++++-------
>>   3 files changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 638691028050..af52580c1e63 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>>       int (*start_ioeventfd)(VirtIODevice *vdev);
>>       void (*stop_ioeventfd)(VirtIODevice *vdev);
>>       /* Called before loading queues. Useful to add queues before loading. */
>> -    int (*pre_load_queues)(VirtIODevice *vdev);
>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
> 
> This turns out to be tricky as it has a lot of assumptions as
> described in the changelog (e.g only lower 32bit of guest_features is
> correct etc when calling this function).

The problem is that I missed the following comment in
hw/virtio/virtio.c:
     /*
      * Temporarily set guest_features low bits - needed by
      * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
      * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
      *
      * Note: devices should always test host features in future - don't 
create
      * new dependencies like this.
      */
     vdev->guest_features = features;

This problem is specific to guest_features so avoiding it should give us 
a reliable solution.

> 
> Looking at the commit that introduces this which is 9379ea9db3c that says:
> 
> """
> Otherwise the loaded queues will not have handlers and elements
> in them will not be processed.
> """
> 
> I fail to remember what it means or what happens if we don't do 9379ea9db3c.

The packets and control commands in the queues except the first queue 
will not be processed because they do not have handle_output set.

Regards,
Akihiko Odaki

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Jason Wang]

On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
> > On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
> >> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
> >> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
> >> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
> >> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
> >> VIRTIO_NET_F_MQ uses bit 22.
> >>
> >> Instead of inferring the required number of queues from
> >> vdev->guest_features, use the number loaded from the vm state.
> >>
> >> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   include/hw/virtio/virtio.h |  2 +-
> >>   hw/net/virtio-net.c        | 11 ++++-------
> >>   hw/virtio/virtio.c         | 14 +++++++-------
> >>   3 files changed, 12 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index 638691028050..af52580c1e63 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
> >>       int (*start_ioeventfd)(VirtIODevice *vdev);
> >>       void (*stop_ioeventfd)(VirtIODevice *vdev);
> >>       /* Called before loading queues. Useful to add queues before loading. */
> >> -    int (*pre_load_queues)(VirtIODevice *vdev);
> >> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
> >
> > This turns out to be tricky as it has a lot of assumptions as
> > described in the changelog (e.g only lower 32bit of guest_features is
> > correct etc when calling this function).
>
> The problem is that I missed the following comment in
> hw/virtio/virtio.c:
>      /*
>       * Temporarily set guest_features low bits - needed by
>       * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>       * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
>       *
>       * Note: devices should always test host features in future - don't
> create
>       * new dependencies like this.
>       */
>      vdev->guest_features = features;
>
> This problem is specific to guest_features so avoiding it should give us
> a reliable solution.

I meant not all the states were fully loaded for pre_load_queues(),
this seems another trick besides the above one. We should seek a way
to do it in post_load() or at least document the assumptions.

>
> >
> > Looking at the commit that introduces this which is 9379ea9db3c that says:
> >
> > """
> > Otherwise the loaded queues will not have handlers and elements
> > in them will not be processed.
> > """
> >
> > I fail to remember what it means or what happens if we don't do 9379ea9db3c.
>
> The packets and control commands in the queues except the first queue
> will not be processed because they do not have handle_output set.

I don't understand here, the VM is not resumed in this case. Or what
issue did you see here?

Thanks

>
> Regards,
> Akihiko Odaki
>

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki]

On 2025/05/16 10:44, Jason Wang wrote:
> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
>>>> VIRTIO_NET_F_MQ uses bit 22.
>>>>
>>>> Instead of inferring the required number of queues from
>>>> vdev->guest_features, use the number loaded from the vm state.
>>>>
>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    include/hw/virtio/virtio.h |  2 +-
>>>>    hw/net/virtio-net.c        | 11 ++++-------
>>>>    hw/virtio/virtio.c         | 14 +++++++-------
>>>>    3 files changed, 12 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>> index 638691028050..af52580c1e63 100644
>>>> --- a/include/hw/virtio/virtio.h
>>>> +++ b/include/hw/virtio/virtio.h
>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>>>>        int (*start_ioeventfd)(VirtIODevice *vdev);
>>>>        void (*stop_ioeventfd)(VirtIODevice *vdev);
>>>>        /* Called before loading queues. Useful to add queues before loading. */
>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
>>>
>>> This turns out to be tricky as it has a lot of assumptions as
>>> described in the changelog (e.g only lower 32bit of guest_features is
>>> correct etc when calling this function).
>>
>> The problem is that I missed the following comment in
>> hw/virtio/virtio.c:
>>       /*
>>        * Temporarily set guest_features low bits - needed by
>>        * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>        * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
>>        *
>>        * Note: devices should always test host features in future - don't
>> create
>>        * new dependencies like this.
>>        */
>>       vdev->guest_features = features;
>>
>> This problem is specific to guest_features so avoiding it should give us
>> a reliable solution.
> 
> I meant not all the states were fully loaded for pre_load_queues(),
> this seems another trick besides the above one. We should seek a way
> to do it in post_load() or at least document the assumptions.

The name of the function already clarifies the state is not fully 
loaded. An implementation of the function can make no assumption on the 
state except that you can add queues here, which is already documented.

> 
>>
>>>
>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
>>>
>>> """
>>> Otherwise the loaded queues will not have handlers and elements
>>> in them will not be processed.
>>> """
>>>
>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
>>
>> The packets and control commands in the queues except the first queue
>> will not be processed because they do not have handle_output set.
> 
> I don't understand here, the VM is not resumed in this case. Or what
> issue did you see here?

Below is the order of relevant events that happen when loading and 
resuming a VM for migration:

1) vdc->realize() gets called. virtio-net adds one RX queue, one TX 
queue, and one control queue.
2) vdc->pre_load_queues() gets called. virtio-net adds more queues if 
the "n" parameter requires that.
3) The state of queues are loaded.
4) The VM gets resumed.

If we skip 2), 3) will load states of queues that are not added yet, 
which breaks these queues and packets and leave control packets on them 
unprocessed.

Regards,
Akihiko Odaki

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Jason Wang]

On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/05/16 10:44, Jason Wang wrote:
> > On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
> >>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
> >>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
> >>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
> >>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
> >>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
> >>>> VIRTIO_NET_F_MQ uses bit 22.
> >>>>
> >>>> Instead of inferring the required number of queues from
> >>>> vdev->guest_features, use the number loaded from the vm state.
> >>>>
> >>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>> ---
> >>>>    include/hw/virtio/virtio.h |  2 +-
> >>>>    hw/net/virtio-net.c        | 11 ++++-------
> >>>>    hw/virtio/virtio.c         | 14 +++++++-------
> >>>>    3 files changed, 12 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>>> index 638691028050..af52580c1e63 100644
> >>>> --- a/include/hw/virtio/virtio.h
> >>>> +++ b/include/hw/virtio/virtio.h
> >>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
> >>>>        int (*start_ioeventfd)(VirtIODevice *vdev);
> >>>>        void (*stop_ioeventfd)(VirtIODevice *vdev);
> >>>>        /* Called before loading queues. Useful to add queues before loading. */
> >>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
> >>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
> >>>
> >>> This turns out to be tricky as it has a lot of assumptions as
> >>> described in the changelog (e.g only lower 32bit of guest_features is
> >>> correct etc when calling this function).
> >>
> >> The problem is that I missed the following comment in
> >> hw/virtio/virtio.c:
> >>       /*
> >>        * Temporarily set guest_features low bits - needed by
> >>        * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>        * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
> >>        *
> >>        * Note: devices should always test host features in future - don't
> >> create
> >>        * new dependencies like this.
> >>        */
> >>       vdev->guest_features = features;
> >>
> >> This problem is specific to guest_features so avoiding it should give us
> >> a reliable solution.
> >
> > I meant not all the states were fully loaded for pre_load_queues(),
> > this seems another trick besides the above one. We should seek a way
> > to do it in post_load() or at least document the assumptions.
>
> The name of the function already clarifies the state is not fully
> loaded. An implementation of the function can make no assumption on the
> state except that you can add queues here, which is already documented.

Where? I can only see this:

    /* Called before loading queues. Useful to add queues before loading. */

>
> >
> >>
> >>>
> >>> Looking at the commit that introduces this which is 9379ea9db3c that says:
> >>>
> >>> """
> >>> Otherwise the loaded queues will not have handlers and elements
> >>> in them will not be processed.
> >>> """
> >>>
> >>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
> >>
> >> The packets and control commands in the queues except the first queue
> >> will not be processed because they do not have handle_output set.
> >
> > I don't understand here, the VM is not resumed in this case. Or what
> > issue did you see here?
>
> Below is the order of relevant events that happen when loading and
> resuming a VM for migration:
>
> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
> queue, and one control queue.
> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
> the "n" parameter requires that.
> 3) The state of queues are loaded.
> 4) The VM gets resumed.
>
> If we skip 2), 3) will load states of queues that are not added yet,
> which breaks these queues and packets and leave control packets on them
> unprocessed.

Ok, let's document this and

1) explain why only virtio-net requires the pre_load_queues (due to
the fact that the index of ctrl vq could be changed according to
#queue_paris)
2) the commit also fixes the issue that changing the TAP queue pairs twice

Thanks

>
> Regards,
> Akihiko Odaki
>

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki]

On 2025/05/21 9:51, Jason Wang wrote:
> On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/05/16 10:44, Jason Wang wrote:
>>> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
>>>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
>>>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
>>>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
>>>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
>>>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
>>>>>> VIRTIO_NET_F_MQ uses bit 22.
>>>>>>
>>>>>> Instead of inferring the required number of queues from
>>>>>> vdev->guest_features, use the number loaded from the vm state.
>>>>>>
>>>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>> ---
>>>>>>     include/hw/virtio/virtio.h |  2 +-
>>>>>>     hw/net/virtio-net.c        | 11 ++++-------
>>>>>>     hw/virtio/virtio.c         | 14 +++++++-------
>>>>>>     3 files changed, 12 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>> index 638691028050..af52580c1e63 100644
>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>>>>>>         int (*start_ioeventfd)(VirtIODevice *vdev);
>>>>>>         void (*stop_ioeventfd)(VirtIODevice *vdev);
>>>>>>         /* Called before loading queues. Useful to add queues before loading. */
>>>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
>>>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
>>>>>
>>>>> This turns out to be tricky as it has a lot of assumptions as
>>>>> described in the changelog (e.g only lower 32bit of guest_features is
>>>>> correct etc when calling this function).
>>>>
>>>> The problem is that I missed the following comment in
>>>> hw/virtio/virtio.c:
>>>>        /*
>>>>         * Temporarily set guest_features low bits - needed by
>>>>         * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>>         * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
>>>>         *
>>>>         * Note: devices should always test host features in future - don't
>>>> create
>>>>         * new dependencies like this.
>>>>         */
>>>>        vdev->guest_features = features;
>>>>
>>>> This problem is specific to guest_features so avoiding it should give us
>>>> a reliable solution.
>>>
>>> I meant not all the states were fully loaded for pre_load_queues(),
>>> this seems another trick besides the above one. We should seek a way
>>> to do it in post_load() or at least document the assumptions.
>>
>> The name of the function already clarifies the state is not fully
>> loaded. An implementation of the function can make no assumption on the
>> state except that you can add queues here, which is already documented.
> 
> Where? I can only see this:
> 
>      /* Called before loading queues. Useful to add queues before loading. */

I meant it is documented that it can add queues. There is nothing else 
to document as an implementation of the function can make no assumption 
else.

> 
>>
>>>
>>>>
>>>>>
>>>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
>>>>>
>>>>> """
>>>>> Otherwise the loaded queues will not have handlers and elements
>>>>> in them will not be processed.
>>>>> """
>>>>>
>>>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
>>>>
>>>> The packets and control commands in the queues except the first queue
>>>> will not be processed because they do not have handle_output set.
>>>
>>> I don't understand here, the VM is not resumed in this case. Or what
>>> issue did you see here?
>>
>> Below is the order of relevant events that happen when loading and
>> resuming a VM for migration:
>>
>> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
>> queue, and one control queue.
>> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
>> the "n" parameter requires that.
>> 3) The state of queues are loaded.
>> 4) The VM gets resumed.
>>
>> If we skip 2), 3) will load states of queues that are not added yet,
>> which breaks these queues and packets and leave control packets on them
>> unprocessed.
> 
> Ok, let's document this and
> 
> 1) explain why only virtio-net requires the pre_load_queues (due to
> the fact that the index of ctrl vq could be changed according to
> #queue_paris)

We would need this logic even if the index of ctrl vq didn't change. We 
need it because virtio-net have varying number of queues, which needs to 
be added before loading.

> 2) the commit also fixes the issue that changing the TAP queue pairs twice

Commit 9379ea9db3c makes it change the TAP queue pairs once more. This 
patch reverts it, but that doesn't matter because the operation is 
idempotent.

More concretely, before commit 9379ea9db3c, we had two points that 
updates the TAP queue pairs when loading a VM for migration:
1) virtio_net_set_features()
2) virtio_net_post_load_device()

Commit 9379ea9db3c added a third point: virtio_net_pre_load_queues().
This patch removes the third point by avoid calling 
virtio_net_set_queue_pairs(), which is a nice side effect.

Regards,
Akihiko Odaki

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Jason Wang]

On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/05/21 9:51, Jason Wang wrote:
> > On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/05/16 10:44, Jason Wang wrote:
> >>> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
> >>>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>>>
> >>>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
> >>>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
> >>>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
> >>>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
> >>>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
> >>>>>> VIRTIO_NET_F_MQ uses bit 22.
> >>>>>>
> >>>>>> Instead of inferring the required number of queues from
> >>>>>> vdev->guest_features, use the number loaded from the vm state.
> >>>>>>
> >>>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
> >>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>>>> ---
> >>>>>>     include/hw/virtio/virtio.h |  2 +-
> >>>>>>     hw/net/virtio-net.c        | 11 ++++-------
> >>>>>>     hw/virtio/virtio.c         | 14 +++++++-------
> >>>>>>     3 files changed, 12 insertions(+), 15 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>>>>> index 638691028050..af52580c1e63 100644
> >>>>>> --- a/include/hw/virtio/virtio.h
> >>>>>> +++ b/include/hw/virtio/virtio.h
> >>>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
> >>>>>>         int (*start_ioeventfd)(VirtIODevice *vdev);
> >>>>>>         void (*stop_ioeventfd)(VirtIODevice *vdev);
> >>>>>>         /* Called before loading queues. Useful to add queues before loading. */
> >>>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
> >>>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
> >>>>>
> >>>>> This turns out to be tricky as it has a lot of assumptions as
> >>>>> described in the changelog (e.g only lower 32bit of guest_features is
> >>>>> correct etc when calling this function).
> >>>>
> >>>> The problem is that I missed the following comment in
> >>>> hw/virtio/virtio.c:
> >>>>        /*
> >>>>         * Temporarily set guest_features low bits - needed by
> >>>>         * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>>>         * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
> >>>>         *
> >>>>         * Note: devices should always test host features in future - don't
> >>>> create
> >>>>         * new dependencies like this.
> >>>>         */
> >>>>        vdev->guest_features = features;
> >>>>
> >>>> This problem is specific to guest_features so avoiding it should give us
> >>>> a reliable solution.
> >>>
> >>> I meant not all the states were fully loaded for pre_load_queues(),
> >>> this seems another trick besides the above one. We should seek a way
> >>> to do it in post_load() or at least document the assumptions.
> >>
> >> The name of the function already clarifies the state is not fully
> >> loaded. An implementation of the function can make no assumption on the
> >> state except that you can add queues here, which is already documented.
> >
> > Where? I can only see this:
> >
> >      /* Called before loading queues. Useful to add queues before loading. */
>
> I meant it is documented that it can add queues. There is nothing else
> to document as an implementation of the function can make no assumption
> else.
>
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
> >>>>>
> >>>>> """
> >>>>> Otherwise the loaded queues will not have handlers and elements
> >>>>> in them will not be processed.
> >>>>> """
> >>>>>
> >>>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
> >>>>
> >>>> The packets and control commands in the queues except the first queue
> >>>> will not be processed because they do not have handle_output set.
> >>>
> >>> I don't understand here, the VM is not resumed in this case. Or what
> >>> issue did you see here?
> >>
> >> Below is the order of relevant events that happen when loading and
> >> resuming a VM for migration:
> >>
> >> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
> >> queue, and one control queue.
> >> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
> >> the "n" parameter requires that.
> >> 3) The state of queues are loaded.
> >> 4) The VM gets resumed.
> >>
> >> If we skip 2), 3) will load states of queues that are not added yet,
> >> which breaks these queues and packets and leave control packets on them
> >> unprocessed.
> >
> > Ok, let's document this and
> >
> > 1) explain why only virtio-net requires the pre_load_queues (due to
> > the fact that the index of ctrl vq could be changed according to
> > #queue_paris)
>
> We would need this logic even if the index of ctrl vq didn't change. We
> need it because virtio-net have varying number of queues, which needs to
> be added before loading.

Well, if the ctrl vq index doesn't change we don't need a dynamic
virtio_add_queue() we can do them all in realize just like other
multiqueue devices.

>
> > 2) the commit also fixes the issue that changing the TAP queue pairs twice
>
> Commit 9379ea9db3c makes it change the TAP queue pairs once more. This
> patch reverts it, but that doesn't matter because the operation is
> idempotent.

It avoids unnecessary stuff like uevent etc...

>
> More concretely, before commit 9379ea9db3c, we had two points that
> updates the TAP queue pairs when loading a VM for migration:
> 1) virtio_net_set_features()
> 2) virtio_net_post_load_device()
>
> Commit 9379ea9db3c added a third point: virtio_net_pre_load_queues().
> This patch removes the third point by avoid calling
> virtio_net_set_queue_pairs(), which is a nice side effect.
>
> Regards,
> Akihiko Odaki
>

Thanks

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki]

On 2025/05/22 10:50, 'Jason Wang' via devel wrote:
> On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/05/21 9:51, Jason Wang wrote:
>>> On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/05/16 10:44, Jason Wang wrote:
>>>>> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
>>>>>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>
>>>>>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
>>>>>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
>>>>>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
>>>>>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
>>>>>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
>>>>>>>> VIRTIO_NET_F_MQ uses bit 22.
>>>>>>>>
>>>>>>>> Instead of inferring the required number of queues from
>>>>>>>> vdev->guest_features, use the number loaded from the vm state.
>>>>>>>>
>>>>>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
>>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>>> ---
>>>>>>>>      include/hw/virtio/virtio.h |  2 +-
>>>>>>>>      hw/net/virtio-net.c        | 11 ++++-------
>>>>>>>>      hw/virtio/virtio.c         | 14 +++++++-------
>>>>>>>>      3 files changed, 12 insertions(+), 15 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>>>> index 638691028050..af52580c1e63 100644
>>>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>>>>>>>>          int (*start_ioeventfd)(VirtIODevice *vdev);
>>>>>>>>          void (*stop_ioeventfd)(VirtIODevice *vdev);
>>>>>>>>          /* Called before loading queues. Useful to add queues before loading. */
>>>>>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
>>>>>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
>>>>>>>
>>>>>>> This turns out to be tricky as it has a lot of assumptions as
>>>>>>> described in the changelog (e.g only lower 32bit of guest_features is
>>>>>>> correct etc when calling this function).
>>>>>>
>>>>>> The problem is that I missed the following comment in
>>>>>> hw/virtio/virtio.c:
>>>>>>         /*
>>>>>>          * Temporarily set guest_features low bits - needed by
>>>>>>          * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>>>>          * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
>>>>>>          *
>>>>>>          * Note: devices should always test host features in future - don't
>>>>>> create
>>>>>>          * new dependencies like this.
>>>>>>          */
>>>>>>         vdev->guest_features = features;
>>>>>>
>>>>>> This problem is specific to guest_features so avoiding it should give us
>>>>>> a reliable solution.
>>>>>
>>>>> I meant not all the states were fully loaded for pre_load_queues(),
>>>>> this seems another trick besides the above one. We should seek a way
>>>>> to do it in post_load() or at least document the assumptions.
>>>>
>>>> The name of the function already clarifies the state is not fully
>>>> loaded. An implementation of the function can make no assumption on the
>>>> state except that you can add queues here, which is already documented.
>>>
>>> Where? I can only see this:
>>>
>>>       /* Called before loading queues. Useful to add queues before loading. */
>>
>> I meant it is documented that it can add queues. There is nothing else
>> to document as an implementation of the function can make no assumption
>> else.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
>>>>>>>
>>>>>>> """
>>>>>>> Otherwise the loaded queues will not have handlers and elements
>>>>>>> in them will not be processed.
>>>>>>> """
>>>>>>>
>>>>>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
>>>>>>
>>>>>> The packets and control commands in the queues except the first queue
>>>>>> will not be processed because they do not have handle_output set.
>>>>>
>>>>> I don't understand here, the VM is not resumed in this case. Or what
>>>>> issue did you see here?
>>>>
>>>> Below is the order of relevant events that happen when loading and
>>>> resuming a VM for migration:
>>>>
>>>> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
>>>> queue, and one control queue.
>>>> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
>>>> the "n" parameter requires that.
>>>> 3) The state of queues are loaded.
>>>> 4) The VM gets resumed.
>>>>
>>>> If we skip 2), 3) will load states of queues that are not added yet,
>>>> which breaks these queues and packets and leave control packets on them
>>>> unprocessed.
>>>
>>> Ok, let's document this and
>>>
>>> 1) explain why only virtio-net requires the pre_load_queues (due to
>>> the fact that the index of ctrl vq could be changed according to
>>> #queue_paris)
>>
>> We would need this logic even if the index of ctrl vq didn't change. We
>> need it because virtio-net have varying number of queues, which needs to
>> be added before loading.
> 
> Well, if the ctrl vq index doesn't change we don't need a dynamic
> virtio_add_queue() we can do them all in realize just like other
> multiqueue devices.

The number of virtqueues also affects the behavior visible to the guest 
so we shouldn't add them all in realize anyway. In particular, struct 
virtio_pci_common_cfg contains fields related virtqueus, and most (if 
not all) of them are affected by the number of virtqueues.

Regards,
Akihiko Odaki

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Jason Wang]

On Thu, May 22, 2025 at 12:39 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/05/22 10:50, 'Jason Wang' via devel wrote:
> > On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/05/21 9:51, Jason Wang wrote:
> >>> On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> On 2025/05/16 10:44, Jason Wang wrote:
> >>>>> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>>>
> >>>>>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
> >>>>>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>>>>>
> >>>>>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
> >>>>>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
> >>>>>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
> >>>>>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
> >>>>>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
> >>>>>>>> VIRTIO_NET_F_MQ uses bit 22.
> >>>>>>>>
> >>>>>>>> Instead of inferring the required number of queues from
> >>>>>>>> vdev->guest_features, use the number loaded from the vm state.
> >>>>>>>>
> >>>>>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
> >>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>>>>>> ---
> >>>>>>>>      include/hw/virtio/virtio.h |  2 +-
> >>>>>>>>      hw/net/virtio-net.c        | 11 ++++-------
> >>>>>>>>      hw/virtio/virtio.c         | 14 +++++++-------
> >>>>>>>>      3 files changed, 12 insertions(+), 15 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>>>>>>> index 638691028050..af52580c1e63 100644
> >>>>>>>> --- a/include/hw/virtio/virtio.h
> >>>>>>>> +++ b/include/hw/virtio/virtio.h
> >>>>>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
> >>>>>>>>          int (*start_ioeventfd)(VirtIODevice *vdev);
> >>>>>>>>          void (*stop_ioeventfd)(VirtIODevice *vdev);
> >>>>>>>>          /* Called before loading queues. Useful to add queues before loading. */
> >>>>>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
> >>>>>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
> >>>>>>>
> >>>>>>> This turns out to be tricky as it has a lot of assumptions as
> >>>>>>> described in the changelog (e.g only lower 32bit of guest_features is
> >>>>>>> correct etc when calling this function).
> >>>>>>
> >>>>>> The problem is that I missed the following comment in
> >>>>>> hw/virtio/virtio.c:
> >>>>>>         /*
> >>>>>>          * Temporarily set guest_features low bits - needed by
> >>>>>>          * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>>>>>          * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
> >>>>>>          *
> >>>>>>          * Note: devices should always test host features in future - don't
> >>>>>> create
> >>>>>>          * new dependencies like this.
> >>>>>>          */
> >>>>>>         vdev->guest_features = features;
> >>>>>>
> >>>>>> This problem is specific to guest_features so avoiding it should give us
> >>>>>> a reliable solution.
> >>>>>
> >>>>> I meant not all the states were fully loaded for pre_load_queues(),
> >>>>> this seems another trick besides the above one. We should seek a way
> >>>>> to do it in post_load() or at least document the assumptions.
> >>>>
> >>>> The name of the function already clarifies the state is not fully
> >>>> loaded. An implementation of the function can make no assumption on the
> >>>> state except that you can add queues here, which is already documented.
> >>>
> >>> Where? I can only see this:
> >>>
> >>>       /* Called before loading queues. Useful to add queues before loading. */
> >>
> >> I meant it is documented that it can add queues. There is nothing else
> >> to document as an implementation of the function can make no assumption
> >> else.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
> >>>>>>>
> >>>>>>> """
> >>>>>>> Otherwise the loaded queues will not have handlers and elements
> >>>>>>> in them will not be processed.
> >>>>>>> """
> >>>>>>>
> >>>>>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
> >>>>>>
> >>>>>> The packets and control commands in the queues except the first queue
> >>>>>> will not be processed because they do not have handle_output set.
> >>>>>
> >>>>> I don't understand here, the VM is not resumed in this case. Or what
> >>>>> issue did you see here?
> >>>>
> >>>> Below is the order of relevant events that happen when loading and
> >>>> resuming a VM for migration:
> >>>>
> >>>> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
> >>>> queue, and one control queue.
> >>>> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
> >>>> the "n" parameter requires that.
> >>>> 3) The state of queues are loaded.
> >>>> 4) The VM gets resumed.
> >>>>
> >>>> If we skip 2), 3) will load states of queues that are not added yet,
> >>>> which breaks these queues and packets and leave control packets on them
> >>>> unprocessed.
> >>>
> >>> Ok, let's document this and
> >>>
> >>> 1) explain why only virtio-net requires the pre_load_queues (due to
> >>> the fact that the index of ctrl vq could be changed according to
> >>> #queue_paris)
> >>
> >> We would need this logic even if the index of ctrl vq didn't change. We
> >> need it because virtio-net have varying number of queues, which needs to
> >> be added before loading.
> >
> > Well, if the ctrl vq index doesn't change we don't need a dynamic
> > virtio_add_queue() we can do them all in realize just like other
> > multiqueue devices.
>
> The number of virtqueues also affects the behavior visible to the guest
> so we shouldn't add them all in realize anyway. In particular, struct
> virtio_pci_common_cfg contains fields related virtqueus, and most (if
> not all) of them are affected by the number of virtqueues.

I don't understand here, is this requested by the spec for example?

Thanks

>
> Regards,
> Akihiko Odaki
>

Re: [PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki]

On 2025/05/26 9:41, Jason Wang wrote:
> On Thu, May 22, 2025 at 12:39 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/05/22 10:50, 'Jason Wang' via devel wrote:
>>> On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/05/21 9:51, Jason Wang wrote:
>>>>> On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> On 2025/05/16 10:44, Jason Wang wrote:
>>>>>>> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>
>>>>>>>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
>>>>>>>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>>>
>>>>>>>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
>>>>>>>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
>>>>>>>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
>>>>>>>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
>>>>>>>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
>>>>>>>>>> VIRTIO_NET_F_MQ uses bit 22.
>>>>>>>>>>
>>>>>>>>>> Instead of inferring the required number of queues from
>>>>>>>>>> vdev->guest_features, use the number loaded from the vm state.
>>>>>>>>>>
>>>>>>>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
>>>>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>>>>> ---
>>>>>>>>>>       include/hw/virtio/virtio.h |  2 +-
>>>>>>>>>>       hw/net/virtio-net.c        | 11 ++++-------
>>>>>>>>>>       hw/virtio/virtio.c         | 14 +++++++-------
>>>>>>>>>>       3 files changed, 12 insertions(+), 15 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>>>>>> index 638691028050..af52580c1e63 100644
>>>>>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>>>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>>>>>>>>>>           int (*start_ioeventfd)(VirtIODevice *vdev);
>>>>>>>>>>           void (*stop_ioeventfd)(VirtIODevice *vdev);
>>>>>>>>>>           /* Called before loading queues. Useful to add queues before loading. */
>>>>>>>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
>>>>>>>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
>>>>>>>>>
>>>>>>>>> This turns out to be tricky as it has a lot of assumptions as
>>>>>>>>> described in the changelog (e.g only lower 32bit of guest_features is
>>>>>>>>> correct etc when calling this function).
>>>>>>>>
>>>>>>>> The problem is that I missed the following comment in
>>>>>>>> hw/virtio/virtio.c:
>>>>>>>>          /*
>>>>>>>>           * Temporarily set guest_features low bits - needed by
>>>>>>>>           * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>>>>>>           * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
>>>>>>>>           *
>>>>>>>>           * Note: devices should always test host features in future - don't
>>>>>>>> create
>>>>>>>>           * new dependencies like this.
>>>>>>>>           */
>>>>>>>>          vdev->guest_features = features;
>>>>>>>>
>>>>>>>> This problem is specific to guest_features so avoiding it should give us
>>>>>>>> a reliable solution.
>>>>>>>
>>>>>>> I meant not all the states were fully loaded for pre_load_queues(),
>>>>>>> this seems another trick besides the above one. We should seek a way
>>>>>>> to do it in post_load() or at least document the assumptions.
>>>>>>
>>>>>> The name of the function already clarifies the state is not fully
>>>>>> loaded. An implementation of the function can make no assumption on the
>>>>>> state except that you can add queues here, which is already documented.
>>>>>
>>>>> Where? I can only see this:
>>>>>
>>>>>        /* Called before loading queues. Useful to add queues before loading. */
>>>>
>>>> I meant it is documented that it can add queues. There is nothing else
>>>> to document as an implementation of the function can make no assumption
>>>> else.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
>>>>>>>>>
>>>>>>>>> """
>>>>>>>>> Otherwise the loaded queues will not have handlers and elements
>>>>>>>>> in them will not be processed.
>>>>>>>>> """
>>>>>>>>>
>>>>>>>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
>>>>>>>>
>>>>>>>> The packets and control commands in the queues except the first queue
>>>>>>>> will not be processed because they do not have handle_output set.
>>>>>>>
>>>>>>> I don't understand here, the VM is not resumed in this case. Or what
>>>>>>> issue did you see here?
>>>>>>
>>>>>> Below is the order of relevant events that happen when loading and
>>>>>> resuming a VM for migration:
>>>>>>
>>>>>> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
>>>>>> queue, and one control queue.
>>>>>> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
>>>>>> the "n" parameter requires that.
>>>>>> 3) The state of queues are loaded.
>>>>>> 4) The VM gets resumed.
>>>>>>
>>>>>> If we skip 2), 3) will load states of queues that are not added yet,
>>>>>> which breaks these queues and packets and leave control packets on them
>>>>>> unprocessed.
>>>>>
>>>>> Ok, let's document this and
>>>>>
>>>>> 1) explain why only virtio-net requires the pre_load_queues (due to
>>>>> the fact that the index of ctrl vq could be changed according to
>>>>> #queue_paris)
>>>>
>>>> We would need this logic even if the index of ctrl vq didn't change. We
>>>> need it because virtio-net have varying number of queues, which needs to
>>>> be added before loading.
>>>
>>> Well, if the ctrl vq index doesn't change we don't need a dynamic
>>> virtio_add_queue() we can do them all in realize just like other
>>> multiqueue devices.
>>
>> The number of virtqueues also affects the behavior visible to the guest
>> so we shouldn't add them all in realize anyway. In particular, struct
>> virtio_pci_common_cfg contains fields related virtqueus, and most (if
>> not all) of them are affected by the number of virtqueues.
> 
> I don't understand here, is this requested by the spec for example?

The spec is not relevant here as commit 9379ea9db3c0 ("virtio-net: Add 
queues before loading them") and this patch deal with migration.

QEMU promises that migration will not change behaviors visible to the 
guest and the number of virtqueues are visible to the guest. So these 
changes aim to prevent the number of virtqueues changes after migration.

Regards,
Akihiko Odaki