[libvirt] [PATCH v2 3/3] add per cpu stats to all domain stats

[Peter Krempa]

On Thu, Feb 02, 2017 at 14:09:33 +0300, Nikolay Shirokovskiy wrote:
> Add per host cpu info provided in virDomainGetCPUStats to the
> stats provided in virConnectGetAllDomainStats. Namely:
> 
> "cpu.count" - number of host cpus
> "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds
> "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain
>                     in nanoseconds
> ---
>  docs/news.xml                    |  9 ++++++
>  include/libvirt/libvirt-domain.h |  1 +
>  src/libvirt-domain.c             |  7 +++++
>  src/qemu/qemu_driver.c           | 64 ++++++++++++++++++++++++++++++++++++++++
>  tools/virsh-domain-monitor.c     |  7 +++++
>  tools/virsh.pod                  | 11 +++++--
>  6 files changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index f408293..6e40c33 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -42,6 +42,15 @@
>            Allow setting MTU size for some types of domain interface.
>          </description>
>        </change>
> +      <change>
> +        <summary>
> +          Show per host cpu stats in all domain stats
> +        </summary>
> +        <description>
> +          Show stats provided in virDomainGetCPUStats in all domain stats
> +          as well.
> +        </description>
> +      </change>
>      </section>
>      <section title="Bug fixes">
>        <change>

Please don't put the news file update into the same patch as the code.
We did not write this rule down but in general the conflicts when
backporting any patch should not have to deal with the news file.

> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index e303140..2691ebe 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -2020,6 +2020,7 @@ typedef enum {
>      VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */
>      VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */
>      VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */
> +    VIR_DOMAIN_STATS_PER_CPU = (1 << 7), /* return domain per host CPU info */
>  } virDomainStatsTypes;
>  
>  typedef enum {
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 5b3e842..3726938 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11126,6 +11126,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   *     "cpu.system" - system cpu time spent in nanoseconds as unsigned long
>   *                    long.
>   *
> + * VIR_DOMAIN_STATS_PER_CPU:

This new stats section (which I don't really think it's necessary at
all) shares the prefix with the existing one which is confusing.

Since I don't see a reason for this section and you did not write
anything to explain why it's necessary just remove it and
put everything in the existing section.

> + *     Return per host CPU statistics
> + *     "cpu.count" - number of host cpus
> + *     "cpu.<num>.time" - total cpu time spent for this domain in nanoseconds
> + *     "cpu.<num>.vtime" - time spent in virtual cpu threads for this domain
> + *                         in nanoseconds

Sooo, these are host cpu numbers, right?

> + *
>   * VIR_DOMAIN_STATS_BALLOON:
>   *     Return memory balloon device information.
>   *     The typed parameter keys are in this format:
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 088f55e..d457a71 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18806,6 +18806,69 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>  }
>  
>  static int
> +qemuDomainGetStatsPerCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +                         virDomainObjPtr dom,
> +                         virDomainStatsRecordPtr record,
> +                         int *maxparams,
> +                         unsigned int privflags ATTRIBUTE_UNUSED)
> +{
> +    qemuDomainObjPrivatePtr priv = dom->privateData;
> +    virCgroupCpuStats stats = {0};
> +    virBitmapPtr guestvcpus = NULL;
> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
> +    int ncpus;
> +    size_t i;
> +    int ret = -1;
> +
> +    if (qemuDomainHasVcpuPids(dom))
> +        guestvcpus = virDomainDefGetOnlineVcpumap(dom->def);

Does it make sense to do this if we don't have the PIDs? Without that
it's usually one thread anyways.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170203/7cead912/attachment-0001.sig>