On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2025/05/21 9:51, Jason Wang wrote:
> > 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.
> > */
>
> 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
