diff mbox series

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

Message ID 20210421122624.12292-11-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series RAM_NORESERVE, MAP_NORESERVE and hostmem "reserve" property | expand

Commit Message

David Hildenbrand April 21, 2021, 12:26 p.m. UTC 
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(-)

Comments

Markus Armbruster April 23, 2021, 11:14 a.m. UTC | #1 
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' } }
David Hildenbrand April 23, 2021, 11:16 a.m. UTC | #2 
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;

I thought for a while if calling the property "noreserve" would be 
cleaner, but decided against it.
David Hildenbrand April 23, 2021, 11:18 a.m. UTC | #3 
>> 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?
> 

Whoops, skipped that comment. Can add it if that's the place to document 
that.
Markus Armbruster April 23, 2021, 12:10 p.m. UTC | #4 
David Hildenbrand <david@redhat.com> writes:

>>> 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?
>
> Whoops, skipped that comment. Can add it if that's the place to
> document that.

Yes, please.  .json doc comments is where we document the external
interface.
Markus Armbruster April 23, 2021, 12:11 p.m. UTC | #5 
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.
David Hildenbrand April 23, 2021, 2:21 p.m. UTC | #6 
On 23.04.21 14:10, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>>>> 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?
>>
>> Whoops, skipped that comment. Can add it if that's the place to
>> document that.
> 
> Yes, please.  .json doc comments is where we document the external
> interface.
> 

What about something like this:

diff --git a/qapi/qom.json b/qapi/qom.json
index e9b86893a5..4fa3137aab 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -559,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',
Markus Armbruster April 23, 2021, 2:53 p.m. UTC | #7 
David Hildenbrand <david@redhat.com> writes:

> On 23.04.21 14:10, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>>>> 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?
>>>
>>> Whoops, skipped that comment. Can add it if that's the place to
>>> document that.
>> 
>> Yes, please.  .json doc comments is where we document the external
>> interface.
>> 
>
> What about something like this:
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index e9b86893a5..4fa3137aab 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -559,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',

Thanks!

Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff mbox series

Patch

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..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' } }