Re: [PULL 08/20] virtio-net: Add only one queue pair when realizing

[Laurent Vivier <lvivier@redhat.com>]

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