| Message ID | 20240604073755.1859-9-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/20] tap: Remove tap_probe_vnet_hdr_len() | expand |
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
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);