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

[David Hildenbrand <david@redhat.com>]

On 30.05.25 13:36, Michael S. Tsirkin wrote:
> On Fri, May 30, 2025 at 01:28:58PM +0200, David Hildenbrand wrote:
>> On 30.05.25 13:18, Michael S. Tsirkin wrote:
>>> 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?
>>
>> Is the original author not interested in fixing the problem?
> 
> Given the silence I'd guess no.

SMH, why then even send patches in the first place ...

Will try finding time to look into this ...

-- 
Cheers,

David / dhildenb