From: Akihiko Odaki <akihiko.odaki@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.odaki@daynix.com>
Signed-off-by: Jason Wang <jasowang@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);
-- 
2.42.0

Hi Akihiko,

On 04/06/2024 09:37, Jason Wang wrote:
> From: Akihiko Odaki <akihiko.odaki@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.odaki@daynix.com>
> Signed-off-by: Jason Wang <jasowang@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.

How to reproduce the problem:

Source:

qemu-system-x86_64 -serial mon:stdio -accel kvm -cpu host -m 2G -display none -hda 
vm3.qcow2 -netdev tap,vhost=false,queues=2,id=hostnet0,script=/etc/qemu-ifup -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:49:47:db,mq=true

Destination:

qemu-system-x86_64 -serial mon:stdio -accel kvm -cpu host -m 2G -display none -hda 
vm3.qcow2 -netdev tap,vhost=false,queues=2,id=hostnet0,script=/etc/qemu-ifup -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:49:47:db,mq=true -incoming 
tcp:localhost:4444

In monitor:

migrate tcp:localhost:4444

Result on destination side:

(hangs and then: )
[   44.175916] watchdog: BUG: soft lockup - CPU#0 stuck for 26s! [kworker/0:0:8]
...
I think we have this error because the control virqueue is #3 for QEMU, whereas the kernel 
is using a control virqueue set by the multiqueue (max_queue_pairs * 2 + 1). There is a 
mismatch between queues...

Thanks,
Laurent

On 14/10/2024 10:30, Laurent Vivier wrote:
> Hi Akihiko,
> 
> On 04/06/2024 09:37, Jason Wang wrote:
>> From: Akihiko Odaki <akihiko.odaki@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.odaki@daynix.com>
>> Signed-off-by: Jason Wang <jasowang@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

On Mon, Oct 14, 2024 at 11:16 PM Laurent Vivier <lvivier@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.odaki@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.odaki@daynix.com>
> >> Signed-off-by: Jason Wang <jasowang@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?

Thanks

>

On 17/10/2024 08:59, Jason Wang wrote:
> On Mon, Oct 14, 2024 at 11:16 PM Laurent Vivier <lvivier@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.odaki@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.odaki@daynix.com>
>>>> Signed-off-by: Jason Wang <jasowang@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.

Thanks,
Laurent

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 <lvivier@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.odaki@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.odaki@daynix.com>
>>>>> Signed-off-by: Jason Wang <jasowang@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

Regards,
Akihiko Odaki

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 <lvivier@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.odaki@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.odaki@daynix.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@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

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 <lvivier@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.odaki@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.odaki@daynix.com>
>>>>>>> Signed-off-by: Jason Wang <jasowang@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

> 
> Jason, let me know which solution you prefer (revert or pre_load_queues 
> helper).
> 
> CC'ing MST
> 
> Thanks,
> Laurent
> 

On Thu, Oct 17, 2024 at 5:42 PM Akihiko Odaki <akihiko.odaki@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 <lvivier@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.odaki@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.odaki@daynix.com>
> >>>>>>> Signed-off-by: Jason Wang <jasowang@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

?

Thanks

>
> >
> > Jason, let me know which solution you prefer (revert or pre_load_queues
> > helper).
> >
> > CC'ing MST
> >
> > Thanks,
> > Laurent
> >
>

On 2024/10/18 13:50, Jason Wang wrote:
> On Thu, Oct 17, 2024 at 5:42 PM Akihiko Odaki <akihiko.odaki@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 <lvivier@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.odaki@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.odaki@daynix.com>
>>>>>>>>> Signed-off-by: Jason Wang <jasowang@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.

Regards,
Akihiko Odaki

> 
> ?
> 
> Thanks
> 
>>
>>>
>>> Jason, let me know which solution you prefer (revert or pre_load_queues
>>> helper).
>>>
>>> CC'ing MST
>>>
>>> Thanks,
>>> Laurent
>>>
>>
> 

On Sat, Oct 19, 2024 at 8:38 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/18 13:50, Jason Wang wrote:
> > On Thu, Oct 17, 2024 at 5:42 PM Akihiko Odaki <akihiko.odaki@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 <lvivier@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.odaki@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.odaki@daynix.com>
> >>>>>>>>> Signed-off-by: Jason Wang <jasowang@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?

Thanks

>
> Regards,
> Akihiko Odaki
>
> >
> > ?
> >
> > Thanks
> >
> >>
> >>>
> >>> Jason, let me know which solution you prefer (revert or pre_load_queues
> >>> helper).
> >>>
> >>> CC'ing MST
> >>>
> >>> Thanks,
> >>> Laurent
> >>>
> >>
> >
>

On 2024/10/21 16:23, Jason Wang wrote:
> On Sat, Oct 19, 2024 at 8:38 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/10/18 13:50, Jason Wang wrote:
>>> On Thu, Oct 17, 2024 at 5:42 PM Akihiko Odaki <akihiko.odaki@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 <lvivier@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.odaki@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.odaki@daynix.com>
>>>>>>>>>>> Signed-off-by: Jason Wang <jasowang@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