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

[Markus Armbruster]

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.

> 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;
> +    }

Aha.  Shouldn't this be documented in qom.json?

> +
>      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..e9b86893a5 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
> @@ -566,6 +569,7 @@
>              '*prealloc': 'bool',
>              '*prealloc-threads': 'uint32',
>              '*share': 'bool',
> +            '*reserve': 'bool',
>              'size': 'size',
>              '*x-use-canonical-path-for-ramblock-id': 'bool' } }