David Hildenbrand <da...@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 <pet...@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> Cc: Markus Armbruster <arm...@redhat.com>
> Cc: Eric Blake <ebl...@redhat.com>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Signed-off-by: David Hildenbrand <da...@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' } }
