[libvirt] [PATCH v7 5/7] qemu: return balloon statistics when all domain statistics reported

[Pavel Hrdina]

On Wed, Jul 13, 2016 at 01:42:16PM +0300, Derbyshev Dmitriy wrote:
> From: Derbyshev Dmitry <dderbyshev at virtuozzo.com>
> 
> To collect all balloon statistics for all guests it was necessary to make
> several libvirt requests. Now it's possible to get all balloon statiscs via
> single connectGetAllDomainStats call.
> 
> Signed-off-by: Derbyshev Dmitry <dderbyshev at virtuozzo.com>
> ---
>  src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++++++++++++++--
>  tools/virsh.pod        | 12 +++++++++++-
>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6fa8d01..70f3faa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18511,15 +18511,29 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>      return 0;
>  }
>  
> +
> +#define STORE_MEM_RECORD(TAG, NAME) {                  \
> +    if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ ##TAG) \
> +        if (virTypedParamsAddULLong(&record->params,   \
> +                                    &record->nparams,  \
> +                                    maxparams,         \
> +                                    "balloon." NAME,   \
> +                                    stats[i].val) < 0) \
> +            return -1;                                 \
> +}

This isn't a correct way how to write a MACRO.  For starters, the syntax isn't
the same as for functions.  If you need to ensure that it is a separate block
the correct way is to do something like this:

#define MACRO()             \
do {                        \
    #body of the macro      \
} while (0)

But in this case it's not required to use the do-while construct.

Next point to this MACRO is that it uses other variables not passed as
parameters.  It's OK to do that but only inside a function and right before its
usage and also you should #undef it right away.  For example:

#define MACRO()

MACRO()
MACRO()
MACRO()

#undef MACRO

> +
>  static int
> -qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +qemuDomainGetStatsBalloon(virQEMUDriverPtr driver,
>                            virDomainObjPtr dom,
>                            virDomainStatsRecordPtr record,
>                            int *maxparams,
> -                          unsigned int privflags ATTRIBUTE_UNUSED)
> +                          unsigned int privflags)
>  {
>      qemuDomainObjPrivatePtr priv = dom->privateData;
> +    virDomainMemoryStatStruct stats[VIR_DOMAIN_MEMORY_STAT_NR];
> +    int nr_stats;
>      unsigned long long cur_balloon = 0;
> +    size_t i;
>      int err = 0;
>  
>      if (!virDomainDefHasMemballoon(dom->def)) {
> @@ -18544,8 +18558,29 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                                  virDomainDefGetMemoryTotal(dom->def)) < 0)
>          return -1;
>  
> +    if (err || !HAVE_JOB(privflags))
> +        return 0;

There is no need to check the *err* variable, it's used only to indicate whether
the "balloon.current" should be reported or not.  I would change the condition
to this:

    if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
        return 0;

> +
> +    nr_stats = qemuDomainMemoryStatsInternal(driver, dom, stats,
> +                                             VIR_DOMAIN_MEMORY_STAT_NR);
> +    if (nr_stats < 0)
> +        return 0;
> +
> +    for (i = 0; i < nr_stats; i++) {
> +        STORE_MEM_RECORD(SWAP_IN, "swap_in")
> +        STORE_MEM_RECORD(SWAP_OUT, "swap_out")
> +        STORE_MEM_RECORD(MAJOR_FAULT, "major_fault")
> +        STORE_MEM_RECORD(MINOR_FAULT, "minor_fault")
> +        STORE_MEM_RECORD(UNUSED, "unused")
> +        STORE_MEM_RECORD(AVAILABLE, "available")
> +        STORE_MEM_RECORD(RSS, "rss")
> +        STORE_MEM_RECORD(LAST_UPDATE, "last-update")
> +        STORE_MEM_RECORD(USABLE, "usable")
> +    }
> +
>      return 0;
>  }
> +#undef STORE_MEM_RECORD
>  
>  
>  static int
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 6af94d1..cda1874 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -899,7 +899,17 @@ I<--cpu-total> returns:
>  
>  I<--balloon> returns:
>  "balloon.current" - the memory in kiB currently used,
> -"balloon.maximum" - the maximum memory in kiB allowed
> +"balloon.maximum" - the maximum memory in kiB allowed,
> +"balloon.swap_in" - the amount of data read from swap space (in kB),
> +"balloon.swap_out" - the amount of memory written out to swap space (in kB),
> +"balloon.major_fault" - the number of page faults then disk IO was required,
> +"balloon.minor_fault" - the number of other page faults,
> +"balloon.unused" - the amount of memory left unused by the system (in kB),
> +"balloon.available" - the amount of usable memory as seen by the domain (in kB),
> +"balloon.rss" - Resident Set Size of running domain's process (in kB),
> +"balloon.usable" - the amount of memory which can be reclaimed by balloon
> +without causing host swapping (in KB),
> +"balloon.last-update" - timestamp of the last update of statistics (in seconds),
>  
>  I<--vcpu> returns:
>  "vcpu.current" - current number of online virtual CPUs,
> -- 
> 1.9.5.msysgit.0
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list