Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Markus Armbruster]

Sorry for the late review, got a bit overwhelmed...

Vadim Galitsyn <address@hidden> writes:

> Commands above provide the following memory information in bytes:
>
>   * base-memory - amount of unremovable memory specified
>     with '-m' option at the start of the QEMU process.
>
>   * hotpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
>
>   * balloon-actual-memory - size of the memory that remains
>     available to the guest after ballooning, as reported by the
>     guest. If the guest has not reported its memory, this value
>     equals to @base-memory + @hot-plug-memory. If ballooning
>     is not enabled, no value is reported.
>
> NOTE:
>
>     Parameter @balloon-actual-memory reports the same as
>     "info balloon" command when ballooning is enabled. The idea
>     to have it in scope of this command(s) comes from
>     https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01472.html.

Should we deprecate qmp-query-balloon?  Hmm, see qmp.c below.

> Signed-off-by: Vasilis Liaskovitis <address@hidden>
> Signed-off-by: Mohammed Gamal <address@hidden>
> Signed-off-by: Eduardo Otubo <address@hidden>
> Signed-off-by: Vadim Galitsyn <address@hidden>
> Reviewed-by: Eugene Crosser <address@hidden>
> Cc: Dr. David Alan Gilbert <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: address@hidden
> ---
>
> v4:
>  * Commands "info memory" and "query-memory" were renamed
>    to "info memory-size-summary" and "query-memory-size-summary"
>    correspondingly.
>  * Descriptions for both commands as well as MemoryInfo structure
>    fields were updated/renamed according to
>    http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.
>  * In MemoryInfo structure following fields are now optional:
>    hotpluggable-memory and balloon-actual-memory.
>  * Field "hotpluggable-memory" now not displayed in HMP if target
>    has no CONFIG_MEM_HOTPLUG enabled.
>  * Field "balloon-actual-memory" now not displayed in HMP if
>    ballooning not enabled.
>  * qapi_free_MemoryInfo() used in order to free corresponding memory
>    instead of g_free().
>  * #ifdef CONFIG_MEM_HOTPLUG was removed and replaced with stubs/ approach.
>    get_exiting_hotpluggable_memory_size() function was introduced in
>    hw/mem/pc-dimm.c (available for all targets which have CONFIG_MEM_HOTPLUG
>    enabled). For other targets, there is a stub in stubs/qmp_pc_dimm.c.
>    In addition, stubs/qmp_pc_dimm_device_list.c was renamed to
>    stubs/qmp_pc_dimm.c in order to reflect actual source file content.
>  * Commit message was updated in order to reflect what was changed.
>
> v3:
>  * Use PRIu64 instead of 'lu' when printing results via HMP.
>  * Report zero hot-plugged memory instead of reporting error
>    when target architecture has no CONFIG_MEM_HOTPLUG enabled.
>
> v2:
>  * Fixed build for targets which do not have CONFIG_MEM_HOTPLUG
>    enabled.
>
>  hmp-commands-info.hx                               | 17 ++++++++++++
>  hmp.c                                              | 23 ++++++++++++++++
>  hmp.h                                              |  1 +
>  hw/mem/pc-dimm.c                                   |  6 +++++
>  include/hw/mem/pc-dimm.h                           |  1 +
>  qapi-schema.json                                   | 28 +++++++++++++++++++
>  qmp.c                                              | 31 
> ++++++++++++++++++++++
>  stubs/Makefile.objs                                |  2 +-
>  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 ++++
>  9 files changed, 113 insertions(+), 1 deletion(-)
>  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)

No test coverage?

I prefer to add pairs of QMP / HMP commands in separate patches, QMP
first, for easier review.  This patch seems small enough to tolerate
adding them in a single patch.  But do consider splitting if you have to
respin.

> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ba98e581ab..a535960157 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -829,6 +829,23 @@ ETEXI
>          .cmd = hmp_info_vm_generation_id,
>      },
>  
> +STEXI
> address@hidden info memory-size-summary
> address@hidden memory-size-summary
> +Display the amount of initially allocated, hot-plugged (if enabled)
> +and ballooned (if enabled) memory in bytes.
> +ETEXI
> +
> +    {
> +        .name       = "memory-size-summary",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the amount of initially allocated, "
> +                      "hot-plugged (if enabled) and ballooned (if enabled) "
> +                      "memory in bytes.",
> +        .cmd        = hmp_info_memory_size_summary,
> +    },
> +
>  STEXI
>  @end table
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index dee40284c1..15f632481c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2828,3 +2828,26 @@ void hmp_info_vm_generation_id(Monitor *mon, const 
> QDict *qdict)
>      hmp_handle_error(mon, &err);
>      qapi_free_GuidInfo(info);
>  }
> +
> +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    MemoryInfo *info = qmp_query_memory_size_summary(&err);
> +    if (info) {
> +        monitor_printf(mon, "base-memory: %" PRIu64 "\n",
> +                       info->base_memory);
> +
> +        if (info->has_hotpluggable_memory) {
> +            monitor_printf(mon, "hotpluggable-memory: %" PRIu64 "\n",
> +                           info->hotpluggable_memory);
> +        }
> +
> +        if (info->has_balloon_actual_memory) {
> +            monitor_printf(mon, "balloon-actual-memory: %" PRIu64 "\n",
> +                           info->balloon_actual_memory);
> +        }

Why-do-you-separate-words-by-dashes?  Separating them by spaces has been
the custom since about the tenth century :)

According to your cover letter, "balloon actual memory" is the "size of
the memory that remains available to the guest after ballooning".
That's not obvious.  It could just as well be the size of the balloon.
Can we find a more self-explanatory wording that's still short enough?

> +
> +        qapi_free_MemoryInfo(info);
> +    }
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 214b2617e7..8c5398ea7a 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -144,5 +144,6 @@ void hmp_info_dump(Monitor *mon, const QDict *qdict);
>  void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
>  void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
> +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index b72258e28f..ea81b1384a 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -159,6 +159,12 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
>      return cap.size;
>  }
>  
> +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)

Why is my hot-pluggable memory exiting, and where's it going?  Or do you
mean "exciting"?  Or "existing", perhaps?

> +{
> +    *mem_size = pc_existing_dimms_capacity(errp);
> +    return true;
> +}

What if pc_existing_dimms_capacity() fails?  Shouldn't you return false
then?

Hmm, get_exiting_hotpluggable_memory_size()'s only caller passes
&error_abort, which means you think it can't fail.  Drop the errp
parameter and pass &error_abort here then.

I find "on success store through parameter and return true, on failure
return false" awkward.  I'd return the size on success and (uint64_t)-1
on failure.  Matter of taste.

> +
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>      MemoryDeviceInfoList ***prev = opaque;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2670..738343df32 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, 
> Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp);
>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>                           MemoryRegion *mr, uint64_t align, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 37c4b95aad..683da8a711 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4327,6 +4327,34 @@
>    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
>              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
>  
> +##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of unremovable memory which is specified
> +#               with '-m size' CLI option.

The relative clause suggests that there may be other unremovable memory,
but base-memory reflects only the unremovable memory specified with -m.
Suggest something like

     @base-memory: size of "base" memory specified with command line
                   option -m

> +#
> +# @hotpluggable-memory: size of hot-plugged memory.

I suspect this is actually the size of memory that can be hot-unplugged.
If true, let's say so:

   # @hotpluggable-memory: size memory that can be hot-unplugged

> +#
> +# @balloon-actual-memory: amount of guest memory available after ballooning.
> +#
> +# Since: 2.10.0
> +##
> +{ 'struct': 'MemoryInfo',
> +  'data'  : { 'base-memory': 'int', '*hotpluggable-memory': 'int',
> +              '*balloon-actual-memory': 'int' } }

I think these should all be 'size'.

We suck at using 'size' correctly.  For instance, @BalloonInfo also uses
'int' instead of 'size'.  Looks fixable to me.

Apropos BalloonInfo: you effectively inline it here.  What if we ever
add something to BalloonInfo?  Wouldn't 'balloon': 'BalloonInfo' be
cleaner?

See also my comment after next.

> +
> +##
> +# @query-memory-size-summary:
> +#
> +# Return the amount of initially allocated, hot-plugged (if enabled)
> +# and ballooned (if enabled) memory in bytes.
> +#
> +# Since: 2.10.0
> +##
> +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> +
>  ##
>  # @query-cpu-definitions:
>  #
> diff --git a/qmp.c b/qmp.c
> index 7ee9bcfdcf..a863726ad6 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -712,3 +712,34 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
>  
>      return head;
>  }
> +
> +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> +{
> +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> +    BalloonInfo *balloon_info;
> +    uint64_t hotpluggable_memory = 0;
> +    Error *local_err = NULL;
> +
> +    mem_info->base_memory = ram_size;
> +
> +    mem_info->has_hotpluggable_memory =
> +        get_exiting_hotpluggable_memory_size(&hotpluggable_memory,
> +                                             &error_abort);
> +    if (mem_info->has_hotpluggable_memory) {
> +        mem_info->hotpluggable_memory = hotpluggable_memory;
> +    }

Why the @hotpluggable_memory middleman?  Why not

       mem_info->has_hotpluggable_memory =
           get_exiting_hotpluggable_memory_size(&mem_info->hotpluggable_memory,
                                                &error_abort);

> +
> +    /* In case if it is not possible to get balloon info, just ignore it. */
> +    balloon_info = qmp_query_balloon(&local_err);
> +    if (local_err) {
> +        mem_info->has_balloon_actual_memory = false;
> +        error_free(local_err);

Since we throw away the error, query-memory-size-summary is *not* a
replacement for query-balloon.

query-memory-size-summary combines three queries:

(1) base-memory (query can't fail)

(2) hotpluggable-memory (query must not fail, i.e. &error_abort)

(3) balloon-actual-memory (query can fail, and we throw away the error
    then)

Including (3) adds a failure mode.  The fact that we sweep the failure
under the rug doesn't change that.

Why is this a good idea?

> +    } else {
> +        mem_info->has_balloon_actual_memory = true;
> +        mem_info->balloon_actual_memory = balloon_info->actual;
> +    }
> +
> +    qapi_free_BalloonInfo(balloon_info);
> +
> +    return mem_info;
> +}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f5b47bfd74..f7cab5b11c 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
>  stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
> -stub-obj-y += qmp_pc_dimm_device_list.o
> +stub-obj-y += qmp_pc_dimm.o
>  stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += pc_madt_cpu_entry.o
> diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> similarity index 60%
> rename from stubs/qmp_pc_dimm_device_list.c
> rename to stubs/qmp_pc_dimm.c
> index def211564d..f50029326e 100644
> --- a/stubs/qmp_pc_dimm_device_list.c
> +++ b/stubs/qmp_pc_dimm.c
> @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>     return 0;
>  }
> +
> +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)
> +{
> +    return false;
> +}