[PATCH v3 2/2] qemu: Introduce qemuDomainGetStatsCpuHaltPollTime

[Peter Krempa]

On Fri, Jul 16, 2021 at 18:42:23 +0800, Yang Fei wrote:
> This function add halt polling time interface in domstats. So that
> we can use command 'virsh domstats VM' to get the data if system
> support.
> 
> Signed-off-by: Yang Fei <yangfei85 at huawei.com>
> ---
>  src/libvirt-domain.c   |  7 +++++++
>  src/qemu/qemu_driver.c | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 750e32f0ca..8e58c1b43f 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11625,6 +11625,13 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   *     "cpu.user" - user cpu time spent in nanoseconds as unsigned long long.
>   *     "cpu.system" - system cpu time spent in nanoseconds as unsigned long
>   *                    long.
> + *     "haltpollsuccess.time" - halt-polling cpu usage about the VCPU polled
> + *                              until a virtual interrupt was delivered in
> + *                              nanoseconds as unsigned long long.
> + *     "haltpollfail.time" - halt-polling cpu usage about the VCPU had to schedule
> + *                           out (either because the maximum poll time was reached
> + *                           or it needed to yield the CPU) in nanoseconds as
> + *                           unsigned long long.

These don't conform with the other fields as they don't have the 'cpu.'
prefix.

Without the prefix it makes them a group of their own which would have
other implications and that's probably not desired.

Additionally the format is weird. I'd suggest:

cpu.haltpoll.success.time
cpu.haltpoll.fail.time

or something similar, but it must be hierarchical and must make sense.

Additionally the same kind of docs is in virsh's man page, so don't
forget to add it there too.

>   *     "cpu.cache.monitor.count" - the number of cache monitors for this domain
>   *     "cpu.cache.monitor.<num>.name" - the name of cache monitor <num>
>   *     "cpu.cache.monitor.<num>.vcpus" - vcpu list of cache monitor <num>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 235f575901..adb4628558 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17839,6 +17839,23 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
>      return 0;
>  }
>  
> +static int
> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> +                                  virTypedParamList *params)
> +{
> +    unsigned long long haltPollSuccess = 0;
> +    unsigned long long haltPollFail = 0;
> +    pid_t pid = dom->pid;
> +
> +    if (virGetCpuHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0)

This still can reportserrors which would be logged and then igored and
thus would pollute the logs. This is not acceptable in the stats API as
it's being called fairly often.

You must ensure that in any failure case, this doesn't log anything and
doesn't make the stats API return failure. Just silently skip the
output.


> +        return 0;
> +
> +    if (virTypedParamListAddULLong(params, haltPollSuccess, "haltpollsuccess.time") < 0 ||
> +        virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time") < 0)
> +        return -1;
> +
> +    return 0;
> +}