[PATCH v3 2/2] qemu: Introduce qemuDomainGetStatsCpuHaltPollTime

[Yang Fei]

On 2021/7/19 17:07, Peter Krempa wrote:
> 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.
> 

OK, I'll modify it in next version.

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

>From my understanding is that we should avoid recording any error messages when
calling this function(now the virFileReadValueUllong() and virDirOpenIfExists()
will record errors), just let it execute, even if it fails. Have I got that right?

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