Re: [libvirt] [PATCHv3 2/8] qemu: bulk stats: implement CPU stats group

----- Original Message ----- > From: "Wang Rui" <moon.wangrui(a)huawei.com&gt; > To: "Francesco Romani" <fromani(a)redhat.com&gt; > Cc: libvir-list(a)redhat.com, pkrempa(a)redhat.com > Sent: Wednesday, September 10, 2014 10:56:47 AM > Subject: Re: [libvirt] [PATCHv3 2/8] qemu: bulk stats: implement CPU stats group > > On 2014/9/8 21:05, Francesco Romani wrote: >> This patch implements the VIR_DOMAIN_STATS_CPU_TOTAL >> group of statistics. >> >> Signed-off-by: Francesco Romani <fromani(a)redhat.com&gt; >> --- >> include/libvirt/libvirt.h.in | 1 + >> src/libvirt.c | 9 ++++++++ >> src/qemu/qemu_driver.c | 51 >> ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 61 insertions(+) > [...] >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 2950a4b..cfc5941 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -96,6 +96,7 @@ >> #include "storage/storage_driver.h" >> #include "virhostdev.h" >> #include "domain_capabilities.h" >> +#include "vircgroup.h" > > Hi, Francesco. > I see the file including relationship. 'qemu_driver.c' includes > 'qemu_cgroup.h' which > includes 'vircgroup.h'. There are other virCgroupGet* functions called in > qemu_driver.c > now. So I think here "include vircgroup.h" is not necessary. Thanks for the research, I'll remove

>> #define VIR_FROM_THIS VIR_FROM_QEMU >> >> @@ -17338,6 +17339,55 @@ qemuDomainGetStatsState(virConnectPtr conn >> ATTRIBUTE_UNUSED, >> } >> >> >> +static int >> +qemuDomainGetStatsCpu(virConnectPtr conn ATTRIBUTE_UNUSED, >> + virDomainObjPtr dom, >> + virDomainStatsRecordPtr record, >> + int *maxparams, >> + unsigned int privflags ATTRIBUTE_UNUSED) >> +{ >> + qemuDomainObjPrivatePtr priv = dom->privateData; >> + unsigned long long cpu_time = 0; >> + unsigned long long user_time = 0; >> + unsigned long long sys_time = 0; >> + int ncpus = 0; >> + int err; >> + >> + ncpus = nodeGetCPUCount(); >> + if (ncpus > 0 && >> + virTypedParamsAddUInt(&record->params, >> + &record->nparams, >> + maxparams, >> + "cpu.count", >> + (unsigned int)ncpus) < 0) >> + return -1; >> + >> + err = virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time); >> + if (!err && virTypedParamsAddULLong(&record->params, >> + &record->nparams, >> + maxparams, >> + "cpu.time", >> + cpu_time) < 0) >> + return -1; >> + err = virCgroupGetCpuacctStat(priv->cgroup, &user_time, &sys_time); >> + if (!err && virTypedParamsAddULLong(&record->params, >> + &record->nparams, >> + maxparams, >> + "cpu.user", >> + user_time) < 0) >> + return -1; >> + if (!err && virTypedParamsAddULLong(&record->params, >> + &record->nparams, >> + maxparams, >> + "cpu.system", >> + sys_time) < 0) >> + return -1; > > 1. If any of the 'err's is not zero, the function may returns 0 as success. > Is this the expected return? Or at least we can give a warning that we > miss some parameters. > 2. I think it's better to report an error or warning log before return -1. The idea here (well, at least my idea :) ) is to gather as much as data as possible, and to silently skip failures here. The lack of expected output is a good enough indicator that a domain is unresponsive. -1 is reported for really unrecoverable error, like memory (re)allocation failure.

Otherwise, how could the client code be meaningfully informed that some group failed, maybe partially? It is possible that different groups fail for different domains: how could we convey this information?

I have no problems to go this route if there is consensus this is the preferred way to go, but then we'll need to discuss how to convey a meaningful error convention. Bests,