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

[Vadim Galitsyn <vadim.galitsyn@profitbricks.com>]

Hi David,

Thank you for the input. Please find my comments below.

> Is the idea to have something like hmp_info_numa() ("info NUMA"), just
> for qmp? And so it also works without NUMA?
..
> We also have query-memory-devices for that already.

The initial idea was to have a command for both HMP and QMP which would
report
a short summary regarding to memory which was assigned to a VM taking into
account
all the relevant "kinds" of memory (base/static, hot-plugged etc). Instead
of calling
"info memdev" + "info numa" + "info balloon" (if relevant) -- "info memory"
/ "query-memory"
can provide short summary within single monitor call. What do you think
about this in general?

> I don't think ballooning belongs into this at all.
..
> 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.

This can be removed from patch if it's confusing. The reason why it
appeared here is
the short discussion in https://lists.gnu.org/archive/
html/qemu-devel/2012-07/msg01472.html
where it was proposed.

> 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.

In patch v3 (
http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg03475.html)
the command reports zero hot-plugged memory on machines which have
CONFIG_MEM_HOTPLUG disabled. No error emitted for such machines. Patch was
submitted before I received input from you and of course v4 will consider
all the points.

> So what you could do:
>
> - total_base memory
> - total hotplugged memory
> - base_memory per NUMA node
> - hotplugged memory per NUMA node

I will provide an extra patch which will extend "info numa" output to
additionally provide
information about hot-plugged memory per NUMA node and current patch will
utilize
this functionality. Actual "info memory" command (as well as
"query-memory") will provide
information as you suggested.

Thank you,
Vadim


On Mon, Jun 19, 2017 at 9:47 AM, David Hildenbrand <david@redhat.com> wrote:

> 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 <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: qemu-devel@nongnu.org
> > ---
> [...]
> > +
> > +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
>



-- 
Vadim Galitsin
Software Engineer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Fax:      +49 30 577 008 299
Email:    vadim.galitsyn@profitbricks.com
URL:      https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss