Re: [Qemu-devel] [PATCH] hmp, qmp: introduce "info memory" and "query-memory" commands

[David Hildenbrand]

On 13.06.2017 14:55, Vadim Galitsyn wrote:
> Commands above provide the following memory information in bytes:

Is the idea to have something like hmp_info_numa() ("info NUMA"), just
for qmp? And so it also works without NUMA?

I think, for this command to be helpful, you should include a per-NUMA
node information.

So what you could do:

- total_base memory
- total hotplugged memory
- base_memory per NUMA node
- hotplugged memory per NUMA node

> 
>   * hot-plug-memory - amount of memory that was hot-plugged.
> 

We also have query-memory-devices for that already.

>   * ballooned-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, zero value is reported.

I don't think ballooning belongs into this at all.

> 
> NOTE:
> 
>     Parameter @ballooned-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.

Can't you simply change query-balloon to show 0 in case no balloon is
around? Or why can't your caller simply deal with the fact that querying
the balloon might fail? Making a qmp interface directly copy the data
from another qmp interface looks strange.

> 
> 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: address@hidden
> ---
[...]
> +
> +MemoryInfo *qmp_query_memory(Error **errp)
> +{
> +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> +    BalloonInfo *balloon_info;
> +    Error *local_err = NULL;
> +
> +    mem_info->base_memory = ram_size;
> +    mem_info->hot_plug_memory = pc_existing_dimms_capacity(&local_err);

This seems to be x86 specific.
Some machines (e.g. s390x) will not be properly accounted here.
(e.g. they round ram_size up/down) or don't use DIMMs. I think we should
query the machines instead.

> +    if (local_err) {
> +        error_setg(errp, "could not get hot-plug memory info: %s",
> +                   error_get_pretty(local_err));
> +        g_free(mem_info);
> +        return NULL;
> +    }
> +
> +    /* 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->ballooned_actual_memory = 0;
> +        error_free(local_err);
> +    } else {
> +        mem_info->ballooned_actual_memory = balloon_info->actual;
> +    }

-- 

Thanks,

David