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@xxxxxxxxxx>
> ---
> 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;
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list