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

[David Hildenbrand]

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.

--
Thanks,

David / dhildenb