Re: [PATCH v25 QEMU 3/3] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[David Hildenbrand <david@redhat.com>]

> Am 24.06.2020 um 22:36 schrieb Michael S. Tsirkin <mst@redhat.com>:
> 
> ïOn Wed, Jun 24, 2020 at 06:01:02PM +0200, David Hildenbrand wrote:
>>> On 24.06.20 17:37, Michael S. Tsirkin wrote:
>>> On Wed, Jun 24, 2020 at 05:28:59PM +0200, David Hildenbrand wrote:
>>>>> So at the high level the idea was simple, we just clear the dirty bit
>>>>> when page is hinted, unless we sent a new command since. Implementation
>>>>> was reviewed by migration maintainers. If there's a consensus the code
>>>>> is written so badly we can't maintain it, maybe we should remove it.
>>>>> Which parts are unmaintainable in your eyes - migration or virtio ones?
>>>> 
>>>> QEMU implementation without a propert virtio specification. I hope that
>>>> we can *at least* finally document the expected behavior. Alex gave it a
>>>> shot, and I was hoping that Wei could jump in to clarify, help move this
>>>> forward ... after all he implemented (+designed?) the feature and the
>>>> virtio interface.
>>>> 
>>>>> Or maybe it's the general thing that interface was never specced
>>>>> properly.
>>>> 
>>>> Yes, a spec would be definitely a good starter ...
>>>> 
>>>> [...]
>>>> 
>>>>>> 
>>>>>> 1. If migration fails during RAM precopy, the guest will never receive a
>>>>>> DONE notification. Probably easy to fix.
>>>>>> 
>>>>>> 2. Unclear semantics. Alex tried to document what the actual semantics
>>>>>> of hinted pages are.
>>>>> 
>>>>> I'll reply to that now.
>>>>> 
>>>>>> Assume the following in the guest to a previously
>>>>>> hinted page
>>>>>> 
>>>>>> /* page was hinted and is reused now */
>>>>>> if (page[x] != Y)
>>>>>>    page[x] == Y;
>>>>>> /* migration ends, we now run on the destination */
>>>>>> BUG_ON(page[x] != Y);
>>>>>> /* BUG, because the content chan
>>>>> 
>>>>> The assumption hinting makes is that data in page is writtent to before it's used.
>>>>> 
>>>>> 
>>>>>> A guest can observe that. And that could be a random driver that just
>>>>>> allocated a page.
>>>>>> 
>>>>>> (I *assume* in Linux we might catch that using kasan, but I am not 100%
>>>>>> sure, also, the actual semantics to document are unclear - e.g., for
>>>>>> other guests)
>>>>> 
>>>>> I think it's basically simple: hinting means it's ok to
>>>>> fill page with trash unless it has been modified since the command
>>>>> ID supplied.
>>>> 
>>>> Yeah, I quite dislike the semantics, especially, as they are different
>>>> to well-know semantics as e.g., represent in MADV_FREE. Getting changed
>>>> content when reading is really weird. But it seemed to be easier to
>>>> implement (low hanging fruit) and nobody complained back then. Well, now
>>>> we are stuck with it.
>>>> 
>>>> [..]
>>> 
>>> The difference with MADV_FREE is
>>> - asynchronous (using cmd id to synchronize)
>>> - zero not guaranteed
>>> 
>>> right?
>> 
>> *looking into man page*, yes, when reading you either get the old
>> content or zero.
>> 
>> (I remember that a re-read also makes the content stable, but looks like
>> you really have to write to a page)
>> 
>> We should most probably do what Alex suggested and initialize pages (at
>> least write a single byte) when leaking them from the shrinker in the
>> guest while hinting is active, such that the content is stable for
>> anybody to allocate and reuse a page.
> 
> Drivers ignore old content from slab though, so I don't really see
> the point.
> 

Thatâs what weâre hoping for and what we would expect. Maybe we should just life with that assumption and hope for the best ...

>> -- 
>> Thanks,
>> 
>> David / dhildenb
>