Re: [PATCH v1 01/17] exec: Introduce ram_block_discard_set_(unreliable|required)()

[David Hildenbrand <david@redhat.com>]

On 15.05.20 11:54, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> We want to replace qemu_balloon_inhibit() by something more generic.
>> Especially, we want to make sure that technologies that really rely on
>> RAM block discards to work reliably to run mutual exclusive with
>> technologies that break it.
>>
>> E.g., vfio will usually pin all guest memory, turning the virtio-balloon
>> basically useless and make the VM consume more memory than reported via
>> the balloon. While the balloon is special already (=> no guarantees, same
>> behavior possible afer reboots and with huge pages), this will be
>> different, especially, with virtio-mem.
>>
>> Let's implement a way such that we can make both types of technology run
>> mutually exclusive. We'll convert existing balloon inhibitors in successive
>> patches and add some new ones. Add the check to
>> qemu_balloon_is_inhibited() for now. We might want to make
>> virtio-balloon an acutal inhibitor in the future - however, that
>> requires more thought to not break existing setups.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  balloon.c             |  3 ++-
>>  exec.c                | 48 +++++++++++++++++++++++++++++++++++++++++++
>>  include/exec/memory.h | 41 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 91 insertions(+), 1 deletion(-)
>>
>> diff --git a/balloon.c b/balloon.c
>> index f104b42961..c49f57c27b 100644
>> --- a/balloon.c
>> +++ b/balloon.c
>> @@ -40,7 +40,8 @@ static int balloon_inhibit_count;
>>  
>>  bool qemu_balloon_is_inhibited(void)
>>  {
>> -    return atomic_read(&balloon_inhibit_count) > 0;
>> +    return atomic_read(&balloon_inhibit_count) > 0 ||
>> +           ram_block_discard_is_broken();
>>  }
>>  
>>  void qemu_balloon_inhibit(bool state)
>> diff --git a/exec.c b/exec.c
>> index 2874bb5088..52a6e40e99 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -4049,4 +4049,52 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root)
>>      }
>>  }
>>  
>> +static int ram_block_discard_broken;
> 
> This could do with a comment; if I'm reading this right then
>   +ve means broken
>   -ve means required
> 

I'll add to ram_block_discard_broken:

"If positive, discarding RAM is broken. If negative, discarding of RAM
is required to work correctly."

[...]

>>  
>> +/*
>> + * Inhibit technologies that rely on discarding of parts of RAM blocks to work
>> + * reliably, e.g., to manage the actual amount of memory consumed by the VM
>> + * (then, the memory provided by RAM blocks might be bigger than the desired
>> + * memory consumption). This *must* be set if:
> 
> 'technologies that rely on discarding of parts of RAM blocks to work
> reliably' is pretty long; I'm not sure of a better way of saying it
> though.

Maybe simply

"Inhibit technologies that rely on discarding of pages in RAM blocks to
work"?

?

-- 
Thanks,

David / dhildenb