Re: Balloon pressuring page cache

[David Hildenbrand <david@redhat.com>]

On 05.02.20 11:25, Michael S. Tsirkin wrote:
> On Wed, Feb 05, 2020 at 10:58:14AM +0100, David Hildenbrand wrote:
>> On 05.02.20 10:49, Wang, Wei W wrote:
>>> On Wednesday, February 5, 2020 5:37 PM, David Hildenbrand wrote:
>>>>>
>>>>> Not sure how TCG tracks the dirty bits. But In whatever
>>>>> implementation, the hypervisor should have
>>>>
>>>> There is only a single bitmap for that purpose. (well, the one where KVM
>>>> syncs to)
>>>>
>>>>> already dealt with the race between he current round and the previous
>>>> round dirty recording.
>>>>> (the race isn't brought by this feature essentially)
>>>>
>>>> It is guaranteed to work reliably without this feature as you only clear what
>>>> *has been migrated*, 
>>>
>>> Not "clear what has been migrated" (that skips nothing..)
>>> Anyway, it's a hint used for optimization.
>>
>> Yes, an optimization that might easily lead to data corruption when the
>> two bitmaps are either not in place or don't play along in that specific
>> way (and I suspect this is the case under TCG).
> 
> So I checked and TCG has two copies too.
> Each block has bmap used for migration and also dirty_memory
> where pages are marked dirty. See cpu_physical_memory_sync_dirty_bitmap.
qemu_guest_free_page_hint() works on block->bmap.
cpu_physical_memory_set_dirty_range() works on ram_list.dirty_memory[i].

So you are right - sorry for the false alarm and thanks for verifying :)

[...]

> 
> Again a flag that tells guest it should wait until used
> could be a reasonable expension. If we stick to the shrinker
> it's actually implementable easily. With an OOM notifier - I'm not so
> sure ...

See my other mail. I think we should keep handling just as is and not
overcomplicate things (especially in our implementation as you noted)
Instead, maybe abstract the reporting feature.

> 
> And a big part of the problem is that after all this time the page
> hinting interfaces are still undocumented. Quite sad really :(

Yes, that was the source of my confusion ... the double-bitmap thingy is
non-obvious. And anybody who wants to implement that interface in a
hypervisor has to be aware that the race I explained has to be avoided
using e.g., two bitmaps and the sync.

-- 
Thanks,

David / dhildenb