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.
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
