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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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
>