On 12/10/2015 09:41 AM, Daniel P. Berrange wrote:
The VIR_DOMAIN_STATS_VCPU flag to virDomainListGetStats enables reporting of stats about vCPUs. Currently we only report the cumulative CPU running time and the execution state.
This adds reporting of the wait time - time the vCPU wants to run, but the host schedular has something else
scheduler ?
running ahead of it.
The data is reported per-vCPU eg
$ virsh domstats --vcpu demo Domain: 'demo' vcpu.current=4 vcpu.maximum=4 vcpu.0.state=1 vcpu.0.time=1420000000 vcpu.0.wait=18403928 vcpu.1.state=1 vcpu.1.time=130000000 vcpu.1.wait=10612111 vcpu.2.state=1 vcpu.2.time=110000000 vcpu.2.wait=12759501 vcpu.3.state=1 vcpu.3.time=90000000 vcpu.3.wait=21825087
In implementing this I notice our reporting of CPU execute time has very poor granularity, since we are getting it from /proc/$PID/stat. As a future enhancement we should prefer to get CPU execute time from /proc/$PID/schedstat or /proc/$PID/sched (if either exist on the running kernel)
Probably shouldn't lose this comment ;-) Maybe lift part of this as an XXX in the qemuGetProcessInfo? Once it's there, the algorithm to split and read lines could be made generic for the input required in order to process the fields from sched
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_driver.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 96 insertions(+), 4 deletions(-)
Because usually these types of requests lead to more requests - should the *cpuWait be the only member of some new private structure rather than an unsigned long long *? Of course it makes the changes here that much more complicated. Could always leave it for future work too. wait_sum, iowait_sum, sum_sleep_runtime...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 783a7cd..5293294 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1361,6 +1361,80 @@ static char *qemuConnectGetCapabilities(virConnectPtr conn) {
static int +qemuGetSchedInfo(unsigned long long *cpuWait, + pid_t pid, pid_t tid) +{ + char *proc = NULL; + char *data = NULL; + char **lines = NULL; + size_t i; + int ret = -1; + double val; + + *cpuWait = 0; + + /* In general, we cannot assume pid_t fits in int; but /proc parsing + * is specific to Linux where int works fine. */ + if (tid) + ret = virAsprintf(&proc, "/proc/%d/task/%d/sched", (int)pid, (int)tid); + else + ret = virAsprintf(&proc, "/proc/%d/sched", (int)pid); + if (ret < 0) + goto cleanup; + + /* The file is not guaranteed to exist (needs CONFIG_SCHED_DEBUG) */ + if (access(proc, R_OK) < 0) + return 0;
Leaks 'proc'. So either go with "ret = 0; goto cleanup;" or "VIR_FREE(proc); return 0;"
+ + if (virFileReadAll(proc, (1<<16), &data) < 0) + goto cleanup; + + lines = virStringSplit(data, "\n", 0); + if (!lines) + goto cleanup; + + for (i = 0; lines[i] != NULL; i++) { + const char *line = lines[i]; + + /* Needs CONFIG_SCHEDSTATS. The second check + * is the old name the kernel used in past */ + if (STRPREFIX(line, "se.statistics.wait_sum") || + STRPREFIX(line, "se.wait_sum")) { + line = strchr(line, ':');
I have a recollection of some code which uses virStringSplit again in order to get the cells and then ensures there's two fields. This works, so it feels like overkill to call virStringSplit again, but you could. Just a suggestion.
+ if (!line) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing separate in sched info '%s'"), + lines[i]); + goto cleanup; + } + line++; + while (*line == ' ') { + line++; + }
This passes syntax-check with the single line and {}? Also, I think virSkipSpaces() is what should be used.
+ + if (virStrToDouble(line, NULL, &val) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse sched info value '%s'"), + line); + goto cleanup; + } + + *cpuWait = (unsigned long long)(val * 1000000); + break; + } + } + + ret = 0; + + cleanup: + VIR_FREE(data); + VIR_FREE(proc); + VIR_FREE(lines); + return ret; +} + + +static int qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, pid_t pid, int tid) { @@ -1424,6 +1498,7 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, static int qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info, + unsigned long long *cpuwait, int maxinfo, unsigned char *cpumaps, int maplen) @@ -1476,6 +1551,11 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, virBitmapFree(map); }
+ if (cpuwait) { + if (qemuGetSchedInfo(&(cpuwait[i]), vm->pid, vcpupid) < 0) + return -1; + } +
Move up directly under "if (info)" just to keep things together? ACK with vir_free and parsing cleanups (pass syntax-check)... Leaving the use of structure or further breakup of function to process requested names is something that can be left as a future exercise. John
ncpuinfo++; }
@@ -5517,7 +5597,7 @@ qemuDomainGetVcpus(virDomainPtr dom, goto cleanup; }
- ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen); + ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
cleanup: virDomainObjEndAPI(&vm); @@ -19089,6 +19169,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, int ret = -1; char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; virVcpuInfoPtr cpuinfo = NULL; + unsigned long long *cpuwait = NULL;
if (virTypedParamsAddUInt(&record->params, &record->nparams, @@ -19104,10 +19185,12 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainDefGetVcpusMax(dom->def)) < 0) return -1;
- if (VIR_ALLOC_N(cpuinfo, virDomainDefGetVcpus(dom->def)) < 0) - return -1; + if (VIR_ALLOC_N(cpuinfo, virDomainDefGetVcpus(dom->def)) < 0 || + VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) + goto cleanup;
- if (qemuDomainHelperGetVcpus(dom, cpuinfo, virDomainDefGetVcpus(dom->def), + if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, + virDomainDefGetVcpus(dom->def), NULL, 0) < 0) { virResetLastError(); ret = 0; /* it's ok to be silent and go ahead */ @@ -19136,12 +19219,21 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, param_name, cpuinfo[i].cpuTime) < 0) goto cleanup; + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, + "vcpu.%zu.wait", i); + if (virTypedParamsAddULLong(&record->params, + &record->nparams, + maxparams, + param_name, + cpuwait[i]) < 0) + goto cleanup; }
ret = 0;
cleanup: VIR_FREE(cpuinfo); + VIR_FREE(cpuwait); return ret; }