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
?
Thanks
>
> >
> > Jason, let me know which solution you prefer (revert or pre_load_queues
> > helper).
> >
> > CC'ing MST
> >
> > Thanks,
> > Laurent
> >
>
