On 17/10/2024 11:07, Akihiko Odaki wrote:
On 2024/10/17 16:32, Laurent Vivier wrote:
On 17/10/2024 08:59, Jason Wang wrote:
On Mon, Oct 14, 2024 at 11:16 PM Laurent Vivier <lviv...@redhat.com> wrote:
On 14/10/2024 10:30, Laurent Vivier wrote:
Hi Akihiko,
On 04/06/2024 09:37, Jason Wang wrote:
From: Akihiko Odaki <akihiko.od...@daynix.com>
Multiqueue usage is not negotiated yet when realizing. If more than
one queue is added and the guest never requests to enable multiqueue,
the extra queues will not be deleted when unrealizing and leak.
Fixes: f9d6dbf0bf6e ("virtio-net: remove virtio queues if the guest doesn't
support
multiqueue")
Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
Signed-off-by: Jason Wang <jasow...@redhat.com>
---
hw/net/virtio-net.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3cee2ef3ac..a8db8bfd9c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3743,9 +3743,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error
**errp)
n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n),
n->net_conf.tx_queue_size);
- for (i = 0; i < n->max_queue_pairs; i++) {
- virtio_net_add_queue(n, i);
- }
+ virtio_net_add_queue(n, 0);
n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
This change breaks virtio net migration when multiqueue is enabled.
I think this is because virtqueues are half initialized after migration : they
are
initialized on guest side (kernel is using them) but not on QEMU side (realized
has only
initialized one). After migration, they are not initialized by the call to
virtio_net_set_multiqueue() from virtio_net_set_features() because
virtio_get_num_queues()
reports already n->max_queue_pairs as this value is coming from the source
guest memory.
I don't think we have a way to half-initialize a virtqueue (to initialize them
only on
QEMU side as they are already initialized on kernel side).
I think this change should be reverted to fix the migration issue.
Moreover, if I look in the code of virtio_load() and virtio_add_queue() we can
guess it's
not correct to migrate a virtqueue that is not initialized on the destination side
because
fields like 'vdev->vq[i].handle_output' or 'vdev->vq[i].used_elems' cannot be
initialized
by virtio_load() and neither by virtio_add_queue() after virtio_load() as
fields like
'vring.num' are already initialized by virtio_load().
For instance, in virtio_load() we set:
for (i = 0; i < num; i++) {
vdev->vq[i].vring.num = qemu_get_be32(f);
and in virtio_add_queue() we search for the firt available queue to add with:
for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
if (vdev->vq[i].vring.num == 0)
break;
}
So virtio_add_queue() cannot be used to set:
vdev->vq[i].handle_output = handle_output;
vdev->vq[i].used_elems = g_new0(VirtQueueElement, queue_size);
Moreover it would overwrite fields already set by virtio_load():
vdev->vq[i].vring.num = queue_size;
vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
It also explains why virtio_net_change_num_queue_pairs() (indirectly called by
virtio_net_set_features()) doesn't update the queue pair numbers: vring.num is already
set
so it thinks there is no more queues to add.
Thanks,
LAurent
I agree.
Laurent, would you like to send a patch to revert this?
Yes. I will also try to fix the leak in unrealize that the patch wanted to fix
initially.
I wrote a fix so I will submit it once internal testing is done. You can see
the change at:
https://gitlab.com/akihiko.odaki/qemu-kvm/-/commit/22161221aa2d2031d7ad1be7701852083aa9109a
It works fine for me but I don't know if it's a good idea to add queues while the state is
loading.
Jason, let me know which solution you prefer (revert or pre_load_queues helper).
CC'ing MST
Thanks,
Laurent
