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

[Daniel P . Berrangé]

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.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|