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

[David Hildenbrand <david@redhat.com>]

On 04.05.21 12:12, Daniel P. Berrangé wrote:
> On Wed, Apr 28, 2021 at 03:37:49PM +0200, David Hildenbrand wrote:
>> 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>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Acked-by: Eduardo Habkost <ehabkost@redhat.com> for memory backend and machine core
>> 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            | 10 ++++++++++
>>   6 files changed, 51 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);
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index c6c1ff5b99..58fdc1b658 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -217,6 +217,11 @@ static void host_memory_backend_set_prealloc(Object *obj, bool value,
>>       Error *local_err = NULL;
>>       HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>>   
>> +    if (!backend->reserve && value) {
>> +        error_setg(errp, "'prealloc=on' and 'reserve=off' are incompatible");
>> +        return;
>> +    }
>> +
>>       if (!host_memory_backend_mr_inited(backend)) {
>>           backend->prealloc = value;
>>           return;
>> @@ -268,6 +273,7 @@ static void host_memory_backend_init(Object *obj)
>>       /* TODO: convert access to globals to compat properties */
>>       backend->merge = machine_mem_merge(machine);
>>       backend->dump = machine_dump_guest_core(machine);
>> +    backend->reserve = true;
>>       backend->prealloc_threads = 1;
>>   }
>>   
>> @@ -426,6 +432,28 @@ static void host_memory_backend_set_share(Object *o, bool value, Error **errp)
>>       backend->share = value;
>>   }
>>   
>> +static bool host_memory_backend_get_reserve(Object *o, Error **errp)
>> +{
>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>> +
>> +    return backend->reserve;
>> +}
>> +
>> +static void host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
>> +{
>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>> +
>> +    if (host_memory_backend_mr_inited(backend)) {
>> +        error_setg(errp, "cannot change property value");
>> +        return;
>> +    }
>> +    if (backend->prealloc && !value) {
>> +        error_setg(errp, "'prealloc=on' and 'reserve=off' are incompatible");
>> +        return;
>> +    }
>> +    backend->reserve = value;
>> +}
>> +
>>   static bool
>>   host_memory_backend_get_use_canonical_path(Object *obj, Error **errp)
>>   {
>> @@ -494,6 +522,10 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
>>           host_memory_backend_get_share, host_memory_backend_set_share);
>>       object_class_property_set_description(oc, "share",
>>           "Mark the memory as private to QEMU or shared");
>> +    object_class_property_add_bool(oc, "reserve",
>> +        host_memory_backend_get_reserve, host_memory_backend_set_reserve);
>> +    object_class_property_set_description(oc, "reserve",
>> +        "Reserve swap space (or huge pages) if applicable");
>>       /*
>>        * Do not delete/rename option. This option must be considered stable
>>        * (as if it didn't have the 'x-' prefix including deprecation period) as
>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>> index df5644723a..9ff5c16963 100644
>> --- a/include/sysemu/hostmem.h
>> +++ b/include/sysemu/hostmem.h
>> @@ -64,7 +64,7 @@ struct HostMemoryBackend {
>>       /* protected */
>>       uint64_t size;
>>       bool merge, dump, use_canonical_path;
>> -    bool prealloc, is_mapped, share;
>> +    bool prealloc, is_mapped, share, reserve;
>>       uint32_t prealloc_threads;
>>       DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>>       HostMemPolicy policy;
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index cd0e76d564..4fa3137aab 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -545,6 +545,9 @@
>>   # @share: if false, the memory is private to QEMU; if true, it is shared
>>   #         (default: false)
>>   #
>> +# @reserve: if true, reserve swap space (or huge pages) if applicable
>> +#           default: true)
>> +#
>>   # @size: size of the memory region in bytes
>>   #
>>   # @x-use-canonical-path-for-ramblock-id: if true, the canoncial path is used
>> @@ -556,6 +559,12 @@
>>   #                                        false generally, but true for machine
>>   #                                        types <= 4.0)
>>   #
>> +# Note: prealloc=true and reserve=false cannot be set at the same time. With
>> +#       reserve=true, the behavior depends on the operating system: for example,
>> +#       Linux will not reserve swap space for shared file mappings --
>> +#       "not applicable". In contrast, reserve=false will bail out if it cannot
>> +#       be configured accordingly.
>> +#
>>   # Since: 2.1
>>   ##
>>   { 'struct': 'MemoryBackendProperties',
>> @@ -566,6 +575,7 @@
>>               '*prealloc': 'bool',
>>               '*prealloc-threads': 'uint32',
>>               '*share': 'bool',
>> +            '*reserve': 'bool',
>>               'size': 'size',
>>               '*x-use-canonical-path-for-ramblock-id': 'bool' } }
> 
> IIUC from the previous patch in the series, 'reserve' is only implemented
> on Linux.  If we make this QAPI prop dependant on CONFIG_LINUX, then mgmt
> apps will do the right thing to detect what platform(s) it works on.

Would that mean only conditionally calling (or ifdeffing) the 
object_property_add_* in backends/hostmem.c?

Note that the "share" property is basically ignored on Windows and only 
implemented on POSIX and we don't even bail out when set ...

Thanks!

-- 
Thanks,

David / dhildenb