[libvirt] [PATCH 1/3] cgroup: read more data from cgroup cpuacct.usage_percpu
On NUMA machine, the length of string got from file cpuacct.usage_percpu is quite large, so expand the limit of 1024 bytes. errors like: Failed to read file \ '/cgroup/cpuacct/libvirt/qemu/rhel6q/cpuacct.usage_percpu': \ Value too large for defined data type --- src/util/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 8541c7f..5dc0764 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -360,7 +360,7 @@ static int virCgroupGetValueStr(virCgroupPtr group, VIR_DEBUG("Get value %s", keypath); - rc = virFileReadAll(keypath, 1024, value); + rc = virFileReadAll(keypath, 1024*1024, value); if (rc < 0) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath); -- 1.7.11.4
--- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5670ca0..5081b52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13460,7 +13460,7 @@ getSumVcpuPercpuStats(virCgroupPtr group, goto cleanup; } - if (virCgroupGetCpuacctPercpuUsage(group, &buf) < 0) + if (virCgroupGetCpuacctPercpuUsage(group_vcpu, &buf) < 0) goto cleanup; pos = buf; -- 1.7.11.4
On 2012年08月31日 17:22, Guannan Ren wrote:
--- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5670ca0..5081b52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13460,7 +13460,7 @@ getSumVcpuPercpuStats(virCgroupPtr group, goto cleanup; }
- if (virCgroupGetCpuacctPercpuUsage(group,&buf)< 0) + if (virCgroupGetCpuacctPercpuUsage(group_vcpu,&buf)< 0) goto cleanup;
pos = buf;
ACK.
The variable max_id is initialized again in the step of getting cpu mapping variable map2. But in the next for loop we still expect original value of max_id, the bug will crash libvirtd when using on NUMA machine with big number of cpus. --- src/qemu/qemu_driver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5081b52..53d6e5b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13496,7 +13496,7 @@ qemuDomainGetPercpuStats(virDomainPtr domain, char *map = NULL; char *map2 = NULL; int rv = -1; - int i, max_id; + int i, id, max_id; char *pos; char *buf = NULL; unsigned long long *sum_cpu_time = NULL; @@ -13537,10 +13537,13 @@ qemuDomainGetPercpuStats(virDomainPtr domain, /* return percpu cputime in index 0 */ param_idx = 0; + /* number of cpus to compute */ + id = max_id; + if (max_id - start_cpu > ncpus - 1) - max_id = start_cpu + ncpus - 1; + id = start_cpu + ncpus - 1; - for (i = 0; i <= max_id; i++) { + for (i = 0; i <= id; i++) { if (!map[i]) { cpu_time = 0; } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { @@ -13580,7 +13583,7 @@ qemuDomainGetPercpuStats(virDomainPtr domain, } sum_cpu_pos = sum_cpu_time; - for (i = 0; i <= max_id; i++) { + for (i = 0; i <= id; i++) { if (!map[i]) cpu_time = 0; else -- 1.7.11.4
On Fri, Aug 31, 2012 at 05:22:59PM +0800, Guannan Ren wrote:
The variable max_id is initialized again in the step of getting cpu mapping variable map2. But in the next for loop we still expect original value of max_id, the bug will crash libvirtd when using on NUMA machine with big number of cpus. --- src/qemu/qemu_driver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5081b52..53d6e5b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13496,7 +13496,7 @@ qemuDomainGetPercpuStats(virDomainPtr domain, char *map = NULL; char *map2 = NULL; int rv = -1; - int i, max_id; + int i, id, max_id; char *pos; char *buf = NULL; unsigned long long *sum_cpu_time = NULL; @@ -13537,10 +13537,13 @@ qemuDomainGetPercpuStats(virDomainPtr domain, /* return percpu cputime in index 0 */ param_idx = 0;
+ /* number of cpus to compute */ + id = max_id; + if (max_id - start_cpu > ncpus - 1) - max_id = start_cpu + ncpus - 1; + id = start_cpu + ncpus - 1;
- for (i = 0; i <= max_id; i++) { + for (i = 0; i <= id; i++) { if (!map[i]) { cpu_time = 0; } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { @@ -13580,7 +13583,7 @@ qemuDomainGetPercpuStats(virDomainPtr domain, }
sum_cpu_pos = sum_cpu_time; - for (i = 0; i <= max_id; i++) { + for (i = 0; i <= id; i++) { if (!map[i]) cpu_time = 0; else
ACK, that sounds right, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 2012年08月31日 17:22, Guannan Ren wrote:
On NUMA machine, the length of string got from file cpuacct.usage_percpu is quite large, so expand the limit of 1024 bytes.
errors like: Failed to read file \ '/cgroup/cpuacct/libvirt/qemu/rhel6q/cpuacct.usage_percpu': \ Value too large for defined data type --- src/util/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 8541c7f..5dc0764 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -360,7 +360,7 @@ static int virCgroupGetValueStr(virCgroupPtr group,
VIR_DEBUG("Get value %s", keypath);
- rc = virFileReadAll(keypath, 1024, value); + rc = virFileReadAll(keypath, 1024*1024, value);
Can it be that large?
if (rc< 0) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath);
On 08/31/2012 05:43 PM, Osier Yang wrote:
On 2012年08月31日 17:22, Guannan Ren wrote:
On NUMA machine, the length of string got from file cpuacct.usage_percpu is quite large, so expand the limit of 1024 bytes.
errors like: Failed to read file \ '/cgroup/cpuacct/libvirt/qemu/rhel6q/cpuacct.usage_percpu': \ Value too large for defined data type --- src/util/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 8541c7f..5dc0764 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -360,7 +360,7 @@ static int virCgroupGetValueStr(virCgroupPtr group,
VIR_DEBUG("Get value %s", keypath);
- rc = virFileReadAll(keypath, 1024, value); + rc = virFileReadAll(keypath, 1024*1024, value);
Can it be that large?
I don't know the good limit for this value. According to my testing machine that has 40 physical cpus, 160 logical cpus from /proc/cpuinfo. The size of cpuacct.usage_percpu is 1366
On Fri, Aug 31, 2012 at 05:22:57PM +0800, Guannan Ren wrote:
On NUMA machine, the length of string got from file cpuacct.usage_percpu is quite large, so expand the limit of 1024 bytes.
errors like: Failed to read file \ '/cgroup/cpuacct/libvirt/qemu/rhel6q/cpuacct.usage_percpu': \ Value too large for defined data type --- src/util/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 8541c7f..5dc0764 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -360,7 +360,7 @@ static int virCgroupGetValueStr(virCgroupPtr group,
VIR_DEBUG("Get value %s", keypath);
- rc = virFileReadAll(keypath, 1024, value); + rc = virFileReadAll(keypath, 1024*1024, value); if (rc < 0) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath);
I'm a bit worried about the arbitrary 1MB limit, let's see... The problem of that function is that it's completely generic, it assume nothing about the content. In our case that MAX_CPU * size(counter) . A counter should not be more than 20 char (very large already) and MAX_CPU of 4096 doesn't sound crazy though I'm sure we will have bigger. So 100KB is already a maxed out value ... for that test case. Since virFileReadAll will allocate (a bit more) up to the size of the file, then okay, 1MB is not insanely large limit, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/31/2012 05:22 PM, Guannan Ren wrote:
On NUMA machine, the length of string got from file cpuacct.usage_percpu is quite large, so expand the limit of 1024 bytes.
errors like: Failed to read file \ '/cgroup/cpuacct/libvirt/qemu/rhel6q/cpuacct.usage_percpu': \ Value too large for defined data type --- src/util/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 8541c7f..5dc0764 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -360,7 +360,7 @@ static int virCgroupGetValueStr(virCgroupPtr group,
VIR_DEBUG("Get value %s", keypath);
- rc = virFileReadAll(keypath, 1024, value); + rc = virFileReadAll(keypath, 1024*1024, value); if (rc < 0) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath);
Thanks for these reviews. pushed. Guannan Ren
participants (3)
-
Daniel Veillard -
Guannan Ren -
Osier Yang