Re: [libvirt] [PATCH 2/3 V7] qemu driver for virDomainGetCPUstats using cpuacct cgroup.

On 03/01/2012 07:54 PM, Lai Jiangshan wrote: Stale commit message; this change was committed earlier. Indentation. Same issue with 'const' as in 1/3. The ? makes it look like you weren't sure. Actually, rv must be max_id + 1 (the size of the array, not the index of the last element in the array). Hmm - start_cpu and ncpus are user-supplied values; their sum could overflow, and we should not misbehave in that case. If the user passes a start_cpu of 1000000, it's probably better to return an error stating that start_cpu is out of range, rather than returning 1 without populating information. On the other hand, if max_id is 3, and the user passes start_cpu 3 and ncpus 2, it makes sense to return just the one in-range entity, rather than erroring out because ncpus was too large. Hmm - this leaves the return struct unpopulated for offline cpus, but the RPC protocol strips out holes on the sending side, and reconstructing on the receiving side won't know where to restore holes. It is only safe to leave holes at the end of the array (if ncpus goes beyond max_id), and we must explicitly populate 0 rather than skipping offline cpus, if they are in range. I confirmed that it also means we need to tweak the RPC code to allow reconstruction with fewer elements than ncpus, in the case where ncpus is too large. But I will submit that as a separate patch. If start_cpu is -1, then ncpus is non-zero; therefore, this conditional could safely be deferred to be second, at which point it becomes redundant. Missing an export for this one. ACK with this squashed in: diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms index 1f58832..c44c617 100644 --- i/src/libvirt_private.syms +++ w/src/libvirt_private.syms @@ -74,16 +74,17 @@ virCgroupForDriver; virCgroupForVcpu; virCgroupFree; virCgroupGetBlkioWeight; -virCgroupGetCpuShares; virCgroupGetCpuCfsPeriod; virCgroupGetCpuCfsQuota; +virCgroupGetCpuShares; +virCgroupGetCpuacctPercpuUsage; virCgroupGetCpuacctUsage; virCgroupGetCpusetMems; virCgroupGetFreezerState; +virCgroupGetMemSwapHardLimit; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryUsage; -virCgroupGetMemSwapHardLimit; virCgroupKill; virCgroupKillPainfully; virCgroupKillRecursive; @@ -92,15 +93,15 @@ virCgroupPathOfController; virCgroupRemove; virCgroupSetBlkioDeviceWeight; virCgroupSetBlkioWeight; -virCgroupSetCpuShares; virCgroupSetCpuCfsPeriod; virCgroupSetCpuCfsQuota; +virCgroupSetCpuShares; virCgroupSetCpusetMems; virCgroupSetFreezerState; +virCgroupSetMemSwapHardLimit; virCgroupSetMemory; virCgroupSetMemoryHardLimit; virCgroupSetMemorySoftLimit; -virCgroupSetMemSwapHardLimit; # command.h diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 98b3c25..538a419 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -12098,7 +12098,7 @@ cleanup: /* qemuDomainGetCPUStats() with start_cpu == -1 */ static int qemuDomainGetTotalcpuStats(virCgroupPtr group, - virTypedParameterPtr params, + virTypedParameterPtr params, int nparams) { unsigned long long cpu_time; @@ -12119,14 +12119,15 @@ qemuDomainGetTotalcpuStats(virCgroupPtr group, return 1; } -static int qemuDomainGetPercpuStats(virDomainPtr domain, - virCgroupPtr group, - virTypedParameterPtr params, - unsigned int nparams, - int start_cpu, - unsigned int ncpus) +static int +qemuDomainGetPercpuStats(virDomainPtr domain, + virCgroupPtr group, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus) { - const char *map = NULL; + char *map = NULL; int rv = -1; int i, max_id; char *pos; @@ -12134,7 +12135,7 @@ static int qemuDomainGetPercpuStats(virDomainPtr domain, virTypedParameterPtr ent; int param_idx; - /* return the number of supported params ? */ + /* return the number of supported params */ if (nparams == 0 && ncpus != 0) return 1; /* only cpu_time is supported */ @@ -12146,23 +12147,31 @@ static int qemuDomainGetPercpuStats(virDomainPtr domain, return rv; if (ncpus == 0) { /* returns max cpu ID */ - rv = max_id; + rv = max_id + 1; + goto cleanup; + } + + if (start_cpu > max_id) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("start_cpu %d larger than maximum of %d"), + start_cpu, max_id); goto cleanup; } + /* we get percpu cputime accounting info. */ if (virCgroupGetCpuacctPercpuUsage(group, &buf)) goto cleanup; pos = buf; - if (max_id > start_cpu + ncpus - 1) + if (max_id - start_cpu > ncpus - 1) max_id = start_cpu + ncpus - 1; for (i = 0; i <= max_id; i++) { unsigned long long cpu_time; - if (!map[i]) - continue; - if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { + if (!map[i]) { + cpu_time = 0; + } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("cpuacct parse error")); goto cleanup; @@ -12225,9 +12234,7 @@ qemuDomainGetCPUStats(virDomainPtr domain, goto cleanup; } - if (nparams == 0 && ncpus == 0) /* returns max cpu id */ - ret = qemuDomainGetPercpuStats(domain, group, NULL, 0, 0, 0); - else if (start_cpu == -1) /* get total */ + if (start_cpu == -1) ret = qemuDomainGetTotalcpuStats(group, params, nparams); else ret = qemuDomainGetPercpuStats(domain, group, params, nparams, -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org