On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2025/05/16 10:44, Jason Wang wrote:
> > On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.od...@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.od...@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.od...@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
>
