Re: [PATCH 1/1] vhost: do not reset used_memslots when destroying vhost dev

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 14, 2025 at 11:26:05AM +0200, David Hildenbrand wrote:
> On 14.05.25 11:12, Igor Mammedov wrote:
> > On Tue, 13 May 2025 15:12:11 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> > 
> > > On 13.05.25 14:13, Igor Mammedov wrote:
> > > > On Mon,  3 Mar 2025 13:02:17 -0500
> > > > yuanminghao <yuanmh12@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.
> > > 
> > > I think the original idea is that all devices (of on type: private vs.
> > > non-private memslots) have the same number of memslots.
> > > 
> > > This avoids having to loop over all devices to figure out the number of
> > > memslots.
> > > 
> > > ... but in vhost_get_free_memslots() we already loop over all devices.
> > > 
> > > The check in vhost_dev_init() needs to be taken care of.
> > > 
> > > So maybe we can get rid of both variables completely?
> > 
> > looks reasonable to me,  (instead of current state which is
> > juggling with  dev->mem->nregions that can become 0 on unplug
> > as it was reported).
> > 
> > David,
> > do you have time to fix it?
> 
> I can try, but I was wondering/hoping whether Yuanminghao could take a look
> at that? I can provide guidance if necessary.


Guys?

> -- 
> Cheers,
> 
> David / dhildenb