Re: [PATCH v4 13/14] qmp: Include "reserve" property of memory backends

[David Hildenbrand]

On 19.03.21 17:32, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
> > On 19.03.21 16:40, Markus Armbruster wrote:
> > > David Hildenbrand <david@redhat.com> writes:
> > >
> > > > Let's include the new property.
> > > >
> > > > Cc: Eric Blake <eblake@redhat.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > > ---
> > > >    hw/core/machine-qmp-cmds.c | 1 +
> > > >    qapi/machine.json          | 6 ++++++
> > > >    2 files changed, 7 insertions(+)
> > > >
> > > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> > > > index 68a942595a..bd2a7f2dd0 100644
> > > > --- a/hw/core/machine-qmp-cmds.c
> > > > +++ b/hw/core/machine-qmp-cmds.c
> > > > @@ -174,6 +174,7 @@ static int query_memdev(Object *obj, void *opaque)
> > > >            m->merge = object_property_get_bool(obj, "merge", &error_abort);
> > > >            m->dump = object_property_get_bool(obj, "dump", &error_abort);
> > > >            m->prealloc = object_property_get_bool(obj, "prealloc", 
> > > > &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 c0c52aef10..12860a1f79 100644
> > > > --- a/qapi/machine.json
> > > > +++ b/qapi/machine.json
> > > > @@ -814,6 +814,11 @@
> > > >    #
> > > >    # @prealloc: enables or disables memory preallocation
> > > >    #
> > > > +# @reserve: enables or disables reservation of swap space (or huge pages
> > > > +#           if applicable). If reservation is enabled (default), actual
> > > > +#           reservation depends on underlying OS support. In contrast,
> > > > +#           disabling reservation without OS support will bail out. (since 6.1)
> > > > +#
> > >
> > > Provides two settings: "enable reservation if possible", and "disable
> > > reservation or else fail".
> > >
> > > Does "enable reservation or else fail" make no sense, or is it merely
> > > unimplemented?
> >
> > The default for now used to be "enable reservation if possible". For
> > example, Windows always reserves/commits the whole region. Under
> > Linux, reservation is always done for private memory mappings,
> > however, especially for basically all (with one exception) shared
> > memory there is no reservation of any kind (with another exception).
> >
> > For example, it does not make sense to reserve swap space for a
> > file-backed mapping; we can just writeback to the file in case we run
> > out of memory. Therefore, Linux will never reserve swap space in that case.
> >
> > So if we were to implement a "enable reservation or else fail", the
> > default ("true") would no longer work for existing setups.
> >
> > Usually we want "enable reservation if possible" unless in spacial
> > cases ("definitely avoid the reservation")
>
> Wait a second...  struct Memdev is actually the result of query-memdev,
> and *not* a command or option argument.
>
> Saying "enables or disables reservation of swap space" is misleading.
> This isn't ever about enabling or disabling things, it's about querying
> whether things are enabled or disabled.
>
> Existing member documentation has the same issue:
>
>      # @merge: enables or disables memory merge support
>      #
>      # @dump: includes memory backend's memory in a core dump or not
>      #
>      # @prealloc: enables or disables memory preallocation

Yes, I was only playing along although it looked kind of weird ...

> Should be something like
>
>      # @merge: whether memory merge support is enabled
>      #
>      # @dump: whether the memory backend's memory is included in a core dump
>      #
>      # @prealloc: whether memory is preallocated

I'll include a cleanup for these in the next version.


> The new member could be phrased like:
>
>      # @reserved: whether swap space (or huge pages if applicable) have
>      # been reserved.
>
> Mind, I'm proposing how to phrase things, not how things are.  You'll
> likely have to adjust the contents of my proposal to match reality.
>
> If we can't always tell whether swap space (or whatever) has been
> reserved, then
>
> * If we can only ever tell when it has *not* been reserved, make false
>    mean "not reserved", and true mean "dunno".
>
> * If we can tell sometimes
>
>    - but nobody cares for the difference between "reserved" and "dunno",
>      same as above.
>
>    - and users may care for the difference, we need three values: "not
>      reserved", "reserved", and "dunno".  There are various ways to do
>      that.  No use talking about them before we know we need one of them.

Right, usually we care about "reserve is a reservation makes sense and 
is possible" - decided by the OS and "definitely don't reserve if you 
would have reserved anything".

Thanks!

--
Thanks,

David / dhildenb