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

[Jason Wang <jasowang@redhat.com>]

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
>