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