Re: [PATCH v2 1/4] softmmu/physmem: Warn with ram_block_discard_range() on MAP_PRIVATE file mapping

[Juan Quintela <quintela@redhat.com>]

David Hildenbrand <david@redhat.com> wrote:
> On 06.07.23 10:10, Juan Quintela wrote:
>> David Hildenbrand <david@redhat.com> wrote:
>>> ram_block_discard_range() cannot possibly do the right thing in
>>> MAP_PRIVATE file mappings in the general case.
>>>
>>> To achieve the documented semantics, we also have to punch a hole into
>>> the file, possibly messing with other MAP_PRIVATE/MAP_SHARED mappings
>>> of such a file.
>>>
>>> For example, using VM templating -- see commit b17fbbe55cba ("migration:
>>> allow private destination ram with x-ignore-shared") -- in combination with
>>> any mechanism that relies on discarding of RAM is problematic. This
>>> includes:
>>> * Postcopy live migration
>>> * virtio-balloon inflation/deflation or free-page-reporting
>>> * virtio-mem
>>>
>>> So at least warn that there is something possibly dangerous is going on
>>> when using ram_block_discard_range() in these cases.
>>>
>>> Acked-by: Peter Xu <peterx@redhat.com>
>>> Tested-by: Mario Casquero <mcasquer@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> (at least we give a warning)
>> But I wonder if we can do better and test that:
>>   * Postcopy live migration
>>     We can check if we are on postcopy, or put a marker so we know
>> that
>>     postcopy can have problems when started.
>>   * virtio-balloon inflation/deflation or free-page-reporting
>>     We can check if we have ever used virtio-balloon.
>>   * virtio-mem
>>     We can check if we have used virtio-men
>> I am just wondering if that is even possible?
>
> Now we warn when any of these features actually tries discarding RAM
> (calling ram_block_discard_range()).
>
> As these features trigger discarding of RAM, once we reach this point
> we know that they are getting used. (in comparison to default libvirt
> attaching a virtio-balloon device without anybody ever using it)
>
>
> The alternative would be checking the RAM for compatibility when
> configuring each features. I decided to warn at a central place for
> now, which covers any users.

I think this is the right thing to do.

Patient: It hurts when I do this.
Doctor: Don't do that.

O:-)

Now more seriously, at this point we are very late to do anything
sensible.  I think that the normal thing when we are configuring
incompatible things just flag it.

We are following that approach with migration for some time now, and
everybody is happier.

Later, Juan.