Re: [PATCH v6 14/15] qmp: Include "reserve" property of memory backends

[David Hildenbrand]

On 23.04.21 14:29, David Hildenbrand wrote:
> On 23.04.21 14:13, Markus Armbruster wrote:
> > David Hildenbrand <david@redhat.com> writes:
> >
> > > On 23.04.21 13:21, Markus Armbruster wrote:
> > > > David Hildenbrand <david@redhat.com> writes:
> > > >
> > > > > On 23.04.21 13:00, Markus Armbruster wrote:
> > > > > > David Hildenbrand <david@redhat.com> writes:
> > > > > >
> > > > > > > Let's include the new property.
> > > > > > >
> > > > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > > > > Cc: Eric Blake <eblake@redhat.com>
> > > > > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > > > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > > > > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > > > > > ---
> > > > > > >      hw/core/machine-qmp-cmds.c | 1 +
> > > > > > >      qapi/machine.json          | 4 ++++
> > > > > > >      2 files changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> > > > > > > index d41db5b93b..2d135ecdd0 100644
> > > > > > > --- a/hw/core/machine-qmp-cmds.c
> > > > > > > +++ b/hw/core/machine-qmp-cmds.c
> > > > > > > @@ -175,6 +175,7 @@ static int query_memdev(Object *obj, void *opaque)
> > > > > > >              m->dump = object_property_get_bool(obj, "dump", &error_abort);
> > > > > > >              m->prealloc = object_property_get_bool(obj, "prealloc", 
> > > > > > > &error_abort);
> > > > > > >              m->share = object_property_get_bool(obj, "share", &error_abort);
> > > > > > > +        m->reserve = object_property_get_bool(obj, "reserve", &error_abort);
> > > > > > >              m->policy = object_property_get_enum(obj, "policy", 
> > > > > > > "HostMemPolicy",
> > > > > > >                                                   &error_abort);
> > > > > > >              host_nodes = object_property_get_qobject(obj,
> > > > > > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > > > > > index 32650bfe9e..5932139d20 100644
> > > > > > > --- a/qapi/machine.json
> > > > > > > +++ b/qapi/machine.json
> > > > > > > @@ -798,6 +798,9 @@
> > > > > > >      #
> > > > > > >      # @share: whether memory is private to QEMU or shared (since 6.1)
> > > > > > >      #
> > > > > > > +# @reserve: whether swap space (or huge pages) was reserved if applicable
> > > > > > > +#           (since 6.1)
> > > > > > > +#
> > > > > > >      # @host-nodes: host nodes for its memory policy
> > > > > > >      #
> > > > > > >      # @policy: memory policy of memory backend
> > > > > > > @@ -812,6 +815,7 @@
> > > > > > >          'dump':       'bool',
> > > > > > >          'prealloc':   'bool',
> > > > > > >          'share':      'bool',
> > > > > > > +    'reserve':    'bool',
> > > > > > >          'host-nodes': ['uint16'],
> > > > > > >          'policy':     'HostMemPolicy' }}
> > > > > >
> > > > > > Double-checking: true means definitely reserved, and false means
> > > > > > definitely not reserved.  Correct?
> > > > >
> > > > > True means "reserved if applicable" which means "not reserved if not
> > > > > applicable". False means "definitely not reserved".
> > > > >
> > > > > (any recommendation how to rephrase are appreciated; I tried my best --
> > > > > this interface here makes it especially hard -- it's easier for the
> > > > > property itself)
> > > >
> > > > When is it "applicable"?
> > >
> > > When the OS supports it for the memory type and it hasn't been disabled.
> > >
> > > Linux handling as described in
> > >     [PATCH v6 09/15] util/mmap-alloc: Support RAM_NORESERVE via
> > >     MAP_NORESERVE under Linux
> > > and
> > >
> > >     https://www.kernel.org/doc/Documentation/vm/overcommit-accounting
> > >
> > > Summary *without* MAP_NORESERVE:
> > >
> > > a) !Hugetlbfs with Memory overcommit disabled via
> > >       ("/proc/sys/vm/overcommit_memory == 2"): never
> > >
> > > b) !Hugetlbfs with Memory overcommit enabled
> > >     1) Shared mappings on files: never
> > >
> > >     2) Private mappings on files: only when writable (for us always)
> > >
> > >     3) Shared anonymous memory: always
> > >
> > >     4) Private anonymous memory: only when writable (for us always)
> > >
> > > c) Hugetlbfs: Always

Actually, it's a bit simpler in our case (writable mappings). The more complex 
part is the MAP_NORESERVE handling.

a) !Hugetlbfs
        1) Shared mappings on files: never reserve swap space

        2) Other mappings: always reserve swap space

b) Hugetlbfs: Always reserve huge pages.

> > > Under Windows: always. On other POSIX besides Linux -- don't know.
> >
> > Would working some of this into the doc comment help users of the
> > interface?  Up to you.
>
> I'll give it a thought. Most people will only care about explicitly
> disabling it, where we'll bail out if that doesn't work. Otherwise, they
> just use the OS default (== reserve if supported/applicable/not
> explicitly disabled).

I think something like the following might make sense:

# @reserve: whether swap space (or huge pages) was reserved if applicable.
#           This corresponds to the user configuration and not the actual
#           behavior implemented in the OS to perform a reservation;
#           For example, Linux will never reserve swap space for shared
#           file mappings. (since 6.1)

Thanks!

--
Thanks,

David / dhildenb