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

[David Hildenbrand <david@redhat.com>]

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