Re: [PATCH v6 10/15] hostmem: Wire up RAM_NORESERVE via "reserve" property

[Markus Armbruster <armbru@redhat.com>]

David Hildenbrand <david@redhat.com> writes:

> On 23.04.21 13:14, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> Let's provide a way to control the use of RAM_NORESERVE via memory
>>> backends using the "reserve" property which defaults to true (old
>>> behavior).
>>>
>>> Only Linux currently supports clearing the flag (and support is checked at
>>> runtime, depending on the setting of "/proc/sys/vm/overcommit_memory").
>>> Windows and other POSIX systems will bail out with "reserve=false".
>>>
>>> The target use case is virtio-mem, which dynamically exposes memory
>>> inside a large, sparse memory area to the VM. This essentially allows
>>> avoiding to set "/proc/sys/vm/overcommit_memory == 0") when using
>>> virtio-mem and also supporting hugetlbfs in the future.
>>>
>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Cc: Eric Blake <eblake@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   backends/hostmem-file.c  | 11 ++++++-----
>>>   backends/hostmem-memfd.c |  1 +
>>>   backends/hostmem-ram.c   |  1 +
>>>   backends/hostmem.c       | 32 ++++++++++++++++++++++++++++++++
>>>   include/sysemu/hostmem.h |  2 +-
>>>   qapi/qom.json            |  4 ++++
>>>   6 files changed, 45 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>>> index b683da9daf..9d550e53d4 100644
>>> --- a/backends/hostmem-file.c
>>> +++ b/backends/hostmem-file.c
>>> @@ -40,6 +40,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>>                  object_get_typename(OBJECT(backend)));
>>>   #else
>>>       HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>>> +    uint32_t ram_flags;
>>>       gchar *name;
>>>         if (!backend->size) {
>>> @@ -52,11 +53,11 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>>       }
>>>         name = host_memory_backend_get_name(backend);
>>> -    memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>>> -                                     name,
>>> -                                     backend->size, fb->align,
>>> -                                     (backend->share ? RAM_SHARED : 0) |
>>> -                                     (fb->is_pmem ? RAM_PMEM : 0),
>>> +    ram_flags = backend->share ? RAM_SHARED : 0;
>>> +    ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>>> +    ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
>>> +    memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
>>> +                                     backend->size, fb->align, ram_flags,
>>>                                        fb->mem_path, fb->readonly, errp);
>>>       g_free(name);
>>>   #endif
>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>>> index 93b5d1a4cf..f3436b623d 100644
>>> --- a/backends/hostmem-memfd.c
>>> +++ b/backends/hostmem-memfd.c
>>> @@ -55,6 +55,7 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>>         name = host_memory_backend_get_name(backend);
>>>       ram_flags = backend->share ? RAM_SHARED : 0;
>>> +    ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>>>       memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
>>>                                      backend->size, ram_flags, fd, 0, errp);
>>>       g_free(name);
>>> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
>>> index 741e701062..b8e55cdbd0 100644
>>> --- a/backends/hostmem-ram.c
>>> +++ b/backends/hostmem-ram.c
>>> @@ -29,6 +29,7 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>>         name = host_memory_backend_get_name(backend);
>>>       ram_flags = backend->share ? RAM_SHARED : 0;
>>> +    ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>>>       memory_region_init_ram_flags_nomigrate(&backend->mr, OBJECT(backend), name,
>>>                                              backend->size, ram_flags, errp);
>>>       g_free(name);
>>
>> As the commit message says, @reserve translates to RAM_NORESERVE.
>> Good.
>> I figure passing RAM_NORESERVE can't make these functions fail.
>> Correct?
>> @reserve defaults to true.  The commit message assures us this gives
>> us
>> the old behavior.  Good.  But the patch *adds* flag RAM_NORESERVE when
>> it is true.  Now I'm confused.
>
> ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>
> translates to
>
> if (!backend->reserve)
> 	ram_flags |= RAM_NORESERVE;

You're right!  /me uncrosses eyes...

> I thought for a while if calling the property "noreserve" would be
> cleaner, but decided against it.

I dislike "negative" flag names, too.