On 2024/10/21 16:23, Jason Wang wrote:
On Sat, Oct 19, 2024 at 8:38 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
On 2024/10/18 13:50, Jason Wang wrote:
On Thu, Oct 17, 2024 at 5:42 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
On 2024/10/17 18:17, Laurent Vivier wrote:
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.
I couldn't come up with other options. The problem is that the number of
queues added during realization does not match with the loaded state. We
need to add queues after knowing the negotiated feature set and before
loading the queue states to fix this problem.
Reverting will add queues that are used when the multiqueue feature is
negotiated so it will fix migration for such cases, but will also break
the other cases (i.e., the multiqueue feature is not negotiated) as it
adds too many queues.
Regards,
Akihiko Odaki
I wonder if the following is much more simpler:
1) introducing booleans whether the queue has been deleted
2) in unrelize, deleted only the queue that has not been deleted
The memory leak problem is trivial to solve, but the problem with queue
state loading is not. We need to ensure the number of queues are
consistent with the number of loaded queues.
We currently have too few queues if the multiqueue feature is
negotiated, which results in queues partially initialized with the
loaded state. Reverting will leave too many queues for the cases where
the multiqueue feature is not negotiated, which is also problematic
because virtio-net will reconfigure queues to reduce the number of
queues and dispose loaded states.
I'm not sure I would get here, if those queues were not visible to
drivers. Why do we care?
We also need to reconfigure controlq, which is visible to drivers, and
invalidate its loaded state. virtio_net_change_num_queue_pairs() has the
following comment:
> We always need to remove and add ctrl vq if
> old_num_queues != new_num_queues. Remove ctrl_vq first,
> and then we only enter one of the following two loops.
Regards,
Akihiko Odaki
