On 03/01/2012 07:54 PM, Lai Jiangshan wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>
> * Now, only "cpu_time" is supported.
> * cpuacct cgroup is used for providing percpu cputime information.
>
> * include/libvirt/libvirt.h.in - defines VIR_DOMAIN_CPU_STATS_CPUTIME
Stale commit message; this change was committed earlier.
> * src/qemu/qemu.conf - take care of cpuacct cgroup.
> * src/qemu/qemu_conf.c - take care of cpuacct cgroup.
> * src/qemu/qemu_driver.c - added an interface
> * src/util/cgroup.c/h - added interface for getting percpu cputime
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
> ---
> src/qemu/qemu.conf | 3 +-
> src/qemu/qemu_conf.c | 3 +-
> src/qemu/qemu_driver.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/cgroup.c | 6 ++
> src/util/cgroup.h | 1 +
> 5 files changed, 157 insertions(+), 2 deletions(-)
>
> +++ b/src/qemu/qemu_driver.c
> @@ -12087,6 +12087,151 @@ cleanup:
> return ret;
> }
>
> +/* qemuDomainGetCPUStats() with start_cpu == -1 */
> +static int
> +qemuDomainGetTotalcpuStats(virCgroupPtr group,
> + virTypedParameterPtr params,
> + int nparams)
Indentation.
> +static int qemuDomainGetPercpuStats(virDomainPtr domain,
> + virCgroupPtr group,
> + virTypedParameterPtr params,
> + unsigned int nparams,
> + int start_cpu,
> + unsigned int ncpus)
> +{
> + const char *map = NULL;
Same issue with 'const' as in 1/3.
> + int rv = -1;
> + int i, max_id;
> + char *pos;
> + char *buf = NULL;
> + virTypedParameterPtr ent;
> + int param_idx;
> +
> + /* return the number of supported params ? */
The ? makes it look like you weren't sure.
> + if (ncpus == 0) { /* returns max cpu ID */
> + rv = max_id;
> + goto cleanup;
> + }
Actually, rv must be max_id + 1 (the size of the array, not the index of
the last element in the array).
> + 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;
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.
> + if (!map[i])
> + continue;
> + if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cpuacct parse error"));
> + goto cleanup;
> + }
> + if (i < start_cpu)
> + continue;
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.
> +
> +static int
> +qemuDomainGetCPUStats(virDomainPtr domain,
> + virTypedParameterPtr params,
> + unsigned int nparams,
> + int start_cpu,
> + unsigned int ncpus,
> + unsigned int flags)
> +{
> +
> + if (nparams == 0 && ncpus == 0) /* returns max cpu id */
> + ret = qemuDomainGetPercpuStats(domain, group, NULL, 0, 0, 0);
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.
> + else if (start_cpu == -1) /* get total */
> + ret = qemuDomainGetTotalcpuStats(group, params, nparams);
> + else
> + ret = qemuDomainGetPercpuStats(domain, group, params, nparams,
> + start_cpu, ncpus);
> +++ b/src/util/cgroup.c
> @@ -1555,6 +1555,12 @@ int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage)
> "cpuacct.usage", usage);
> }
>
> +int virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage)
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list