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

[David Hildenbrand <david@redhat.com>]

[...]

>> --- 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