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

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

[Vadim Galitsyn]

Hi Guys,

This thread is a continuation of discussion started at http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.

Vadim

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

[Vadim Galitsyn]

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.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org
---

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%)

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
+@item info memory-size-summary
+@findex 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);
+        }
+
+        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)
+{
+    *mem_size = pc_existing_dimms_capacity(errp);
+    return true;
+}
+
 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.
+#
+# @hotpluggable-memory: size of hot-plugged memory.
+#
+# @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' } }
+
+##
+# @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;
+    }
+
+    /* 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);
+    } 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;
+}
-- 
2.13.1.394.g41dd433

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 <vadim.galitsyn@profitbricks.com> 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 <vasilis.liaskovitis@profitbricks.com>
> Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>
> 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
> +@item info memory-size-summary
> +@findex 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;
> +}

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Sorry for the late review, got a bit overwhelmed...
>
> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> 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.
[...]
>>  9 files changed, 113 insertions(+), 1 deletion(-)
>>  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)
>
> No test coverage?

Maybe no need, because ...

[...]
> 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?

... with (3) and its failure mode removed, there's less need for
specific tests.  The (future, hopefully near future) generic smoke test
of queries that can't fail should do.

[...]

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Sorry for the late review, got a bit overwhelmed...
> 
> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> 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 <vasilis.liaskovitis@profitbricks.com>
> > Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> > Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> > Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >
> > 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.

The HMP tester scans all 'info' commands so it's not needed for the HMP
side.

Dave

> 
> > 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
> > +@item info memory-size-summary
> > +@findex 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;
> > +}
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Sorry for the late review, got a bit overwhelmed...
>> 
>> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> 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.
[...]
>> >  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.
>
> The HMP tester scans all 'info' commands so it's not needed for the HMP
> side.

Correct.  I want a similar test for QMP, but I'm not asking Vadim to
provide it :)

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

[Vadim Galitsyn]

Hi Guys,

Thank you for the feedback! Unfortunately, I am almost off for vacation and
will not be able to provide next patch in the following couple of weeks.
Nevertheless, will do so as soon as I return back.

I still have a question whether we need to provide NUMA information here?
>From my point of view it is a little bit out of scope of these commands
since it gives a bit more detailed information while the commands aimed to
give a short summary. I have a small patch which extends "info numa" with
hotplugged memory information per NUMA node. Maybe it is better to go this
way? I can include it into this patch series.

In the next patch (2 weeks later unfortunately) I will take into account
all the remaining points from this thread:

1. Turn into patch series by splitting HMP/QMP into separate patches;
2. Drop ballooning information;
3. Include "info numa" patch which extents output with hotplugged memory
info per NUMA node.

Thank you for the feedback once again,
Vadim


On Fri, Jul 7, 2017 at 10:58 AM, Markus Armbruster <armbru@redhat.com>
wrote:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Sorry for the late review, got a bit overwhelmed...
> >>
> >> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> 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.
> [...]
> >> >  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.
> >
> > The HMP tester scans all 'info' commands so it's not needed for the HMP
> > side.
>
> Correct.  I want a similar test for QMP, but I'm not asking Vadim to
> provide it :)
>

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

[Vadim Galitsyn]

Hi Guys,

This thread is a continuation of discussion from:
  http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01565.html

I will post changes list here in cover letter.

v5:
 * hmp: Updated description and '.help' message for
   'info memory-size-summary' command.

 * hmp: Removed '-' characters from
   'info memory-size-summary' output.

 * Dropped ballooned memory information.

 * get_existing_hotpluggable_memory_size() assumed
   to never fail; routine now has no arguments and
   returns uint64_t; in case if target does not support
   memory hotplug, (uint64_t)-1 is returned.

 * MemoryInfo structure:
   * Removed @balloon-actual-memory field.
   * Field @hotpluggable-memory renamed
     to @hotunpluggable-memory.
   * Updated description for fields.

 * qmp: Updated description for
   query-memory-size-summary.

 * Patch v4 splitted into series.

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.

Best regards,
Vadim

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

[Markus Armbruster]

Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:

> Hi Guys,
>
> This thread is a continuation of discussion from:
>   http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01565.html
>
> I will post changes list here in cover letter.

Looks good from a QMP point of view.  Just clean up the naming issues.

Eduardo, Igor, through which tree should this go?

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

[Igor Mammedov]

On Mon, 14 Aug 2017 16:32:40 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
> 
> > Hi Guys,
> >
> > This thread is a continuation of discussion from:
> >   http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01565.html
> >
> > I will post changes list here in cover letter.  
> 
> Looks good from a QMP point of view.  Just clean up the naming issues.
> 
> Eduardo, Igor, through which tree should this go?
It's mostly QMP/HMP interface so it could go via QMP tree.