On Mon, 3 Mar 2025 13:02:17 -0500
yuanminghao <yuanm...@chinatelecom.cn> wrote:
> > > Global used_memslots or used_shared_memslots is updated to 0 unexpectly
> >
> > it shouldn't be 0 in practice, as it comes from number of RAM regions VM
> > has.
> > It's likely a bug somewhere else.
I haven't touched this code for a long time, but I'd say if we consider multiple
devices, we shouldn't do following:
static void vhost_commit(MemoryListener *listener)
...
if (dev->vhost_ops->vhost_backend_no_private_memslots &&
dev->vhost_ops->vhost_backend_no_private_memslots(dev)) {
used_shared_memslots = dev->mem->nregions;
} else {
used_memslots = dev->mem->nregions;
}
where value dev->mem->nregions gets is well hidden/obscured
and hard to trace where tail ends => fragile.
CCing David (accidental victim) who rewrote this part the last time,
perhaps he can suggest a better way to fix the issue.
> > Please describe a way to reproduce the issue.
> >
> Hi, Igor Mammedov,
> Sorry for the late response, here are the steps to reproduce the issue:
>
> 1.start a domain with 1Core 1GiB memory, no network interface.
> 2.print used_memslots with gdb
> gdb -p ${qemupid} <<< "p used_memslots"
> $1 = 0
> 3.attach a network interface net1
> cat>/tmp/net1.xml <<EOF
> <interface type='network'>
> <mac address='52:54:00:12:34:56'/>
> <source network='default'/>
> <model type='virtio'/>
> </interface>
> EOF
> virsh attach-device dom /tmp/net1.xml --live
> 4.print current used_memslots with gdb
> gdb -p ${qemupid} <<< "p used_memslots"
> $1 = 2
> 5.attach another network interface net2
> cat>/tmp/net2.xml <<EOF
> <interface type='network'>
> <mac address='52:54:00:12:34:78'/>
> <source network='default'/>
> <model type='virtio'/>
> </interface>
> EOF
> virsh attach-device dom /tmp/net2.xml --live
> 6.print current used_memslots with gdb
> gdb -p ${qemupid} <<< "p used_memslots"
> $1 = 2
> 7.detach network interface net2
> virsh detach-device dom /tmp/net2.xml --live
> 8.print current used_memslots with gdb
> gdb -p ${qemupid} <<< "p used_memslots"
> $1 = 0
> After detaching net2, the used_memslots was reseted to 0, which was expected
> to be 2.
>
> > > when a vhost device destroyed. This can occur during scenarios such as
> > > live
> > > detaching a vhost device or restarting a vhost-user net backend (e.g.,
> > > OVS-DPDK):
> > > #0 vhost_commit(listener) at hw/virtio/vhost.c:439
> > > #1 listener_del_address_space(as, listener) at memory.c:2777
> > > #2 memory_listener_unregister(listener) at memory.c:2823
> > > #3 vhost_dev_cleanup(hdev) at hw/virtio/vhost.c:1406
> > > #4 vhost_net_cleanup(net) at hw/net/vhost_net.c:402
> > > #5 vhost_user_start(be, ncs, queues) at net/vhost-user.c:113
> > > #6 net_vhost_user_event(opaque, event) at net/vhost-user.c:281
> > > #7 tcp_chr_new_client(chr, sioc) at chardev/char-socket.c:924
> > > #8 tcp_chr_accept(listener, cioc, opaque) at chardev/char-socket.c:961
> > >
> > > So we skip the update of used_memslots and used_shared_memslots when
> > > destroying
> > > vhost devices, and it should work event if all vhost devices are removed.
> > >
> > > Signed-off-by: yuanminghao <yuanm...@chinatelecom.cn>
> > > ---
> > > hw/virtio/vhost.c | 14 +++++++++-----
> > > include/hw/virtio/vhost.h | 1 +
> > > 2 files changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 6aa72fd434..2258a12066 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -666,11 +666,13 @@ static void vhost_commit(MemoryListener *listener)
> > > dev->mem = g_realloc(dev->mem, regions_size);
> > > dev->mem->nregions = dev->n_mem_sections;
> > >
> > > - if (dev->vhost_ops->vhost_backend_no_private_memslots &&
> > > - dev->vhost_ops->vhost_backend_no_private_memslots(dev)) {
> > > - used_shared_memslots = dev->mem->nregions;
> > > - } else {
> > > - used_memslots = dev->mem->nregions;
> > > + if (!dev->listener_removing) {
> > > + if (dev->vhost_ops->vhost_backend_no_private_memslots &&
> > > + dev->vhost_ops->vhost_backend_no_private_memslots(dev)) {
> > > + used_shared_memslots = dev->mem->nregions;
> > > + } else {
> > > + used_memslots = dev->mem->nregions;
> > > + }
> > > }
> > >
> > > for (i = 0; i < dev->n_mem_sections; i++) {
> > > @@ -1668,7 +1670,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> > > }
> > > if (hdev->mem) {
> > > /* those are only safe after successful init */
> > > + hdev->listener_removing = true;
> > > memory_listener_unregister(&hdev->memory_listener);
> > > + hdev->listener_removing = false;
> > > QLIST_REMOVE(hdev, entry);
> > > }
> > > migrate_del_blocker(&hdev->migration_blocker);
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index a9469d50bc..037f85b642 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -133,6 +133,7 @@ struct vhost_dev {
> > > QLIST_HEAD(, vhost_iommu) iommu_list;
> > > IOMMUNotifier n;
> > > const VhostDevConfigOps *config_ops;
> > > + bool listener_removing;
> > > };
> > >
> > > extern const VhostOps kernel_ops;
>
