Re: [PATCH v3 1/3] vhost: Rework memslot filtering and fix "used_memslot" tracking

[David Hildenbrand]

[...]

> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -46,20 +46,33 @@
> >   static struct vhost_log *vhost_log;
> >   static struct vhost_log *vhost_log_shm;
> >   
> > +/* Memslots used by backends that support private memslots (without an fd). */
> >   static unsigned int used_memslots;
> > +
> > +/* Memslots used by backends that only support shared memslots (with an fd). */
> > +static unsigned int used_shared_memslots;
>
> It's just that these vars are updated multiple times when >1 vhost is
> there, accessing these fields are still a bit confusing - I think it's
> implicitly protected by BQL so looks always safe.

Yes, like the existing variable.

> Since we already have the shared/private handling, maybe for the long term
> it'll be nicer to just keep such info per-device e.g. in vhost_dev so we
> can also drop vhost_backend_no_private_memslots().  Anyway the code is
> internal so can be done on top even if worthwhile.

Might be possible, but I remember there was a catch to it when 
hotplugging a device.

> > +
> >   static QLIST_HEAD(, vhost_dev) vhost_devices =
> >       QLIST_HEAD_INITIALIZER(vhost_devices);
> >   
> >   bool vhost_has_free_slot(void)
> >   {
> > -    unsigned int slots_limit = ~0U;
> > +    unsigned int free = UINT_MAX;
> >       struct vhost_dev *hdev;
> >   
> >       QLIST_FOREACH(hdev, &vhost_devices, entry) {
> >           unsigned int r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> > -        slots_limit = MIN(slots_limit, r);
> > +        unsigned int cur_free;
> > +
> > +        if (hdev->vhost_ops->vhost_backend_no_private_memslots &&
> > +            hdev->vhost_ops->vhost_backend_no_private_memslots(hdev)) {
> > +            cur_free = r - used_shared_memslots;
> > +        } else {
> > +            cur_free = r - used_memslots;
> > +        }
> > +        free = MIN(free, cur_free);
> >       }
> > -    return slots_limit > used_memslots;
> > +    return free > 1;
>
> Should here be "free > 0" instead?
>
> Trivial but maybe still matter when some device used exactly the size of
> all memslots of a device..

Very good catch, thanks Peter!

--
Thanks,

David / dhildenb