[libvirt] [PATCH 1/1] virsh nodecpustats erroneously returns stats for offline cpus on Linux
From: "Pradipta Kr. Banerjee" <bpradip@in.ibm.com> virsh nodecpustats erroneously returns stats for offline cpus on Linux To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0 and cpu 1,2 are offline, then 'virsh nodecpustats 1' displays data for cpu12 whereas virsh nodecpustats 2 correctly throws the following error "error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats" This patch fixes the above mentioned problem Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_fmt[8]; if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat, if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", 3); } else { snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); } while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; } + /* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid + */ + sscanf(buf, cpu_header_fmt, cpu_header_read); + if (STRNEQ(cpu_header, cpu_header_read)) + continue; + for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i]; -- 1.8.3.1
On 12/19/2013 12:40 PM, Pradipta Kr. Banerjee wrote:
From: "Pradipta Kr. Banerjee" <bpradip@in.ibm.com>
virsh nodecpustats erroneously returns stats for offline cpus on Linux
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0
and cpu 1,2 are offline, then
'virsh nodecpustats 1' displays data for cpu12
whereas virsh nodecpustats 2 correctly throws the following error
"error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats"
This patch fixes the above mentioned problem
Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_fmt[8];
if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat,
if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", 3); } else { snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); }
while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
+ /* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid + */ + sscanf(buf, cpu_header_fmt, cpu_header_read); + if (STRNEQ(cpu_header, cpu_header_read)) + continue; + for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i];
What about this? diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat, while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2); - if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i; if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } + virStringFreeList(buf_header); } virReportInvalidArg(cpuNum,
-- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/19/2013 02:02 PM, Bing Bu Cao wrote:
On 12/19/2013 12:40 PM, Pradipta Kr. Banerjee wrote:
From: "Pradipta Kr. Banerjee" <bpradip@in.ibm.com>
virsh nodecpustats erroneously returns stats for offline cpus on Linux
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0
and cpu 1,2 are offline, then
'virsh nodecpustats 1' displays data for cpu12
whereas virsh nodecpustats 2 correctly throws the following error
"error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats"
This patch fixes the above mentioned problem
Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_fmt[8];
if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat,
if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", 3); } else { snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); }
while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
+ /* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid + */ + sscanf(buf, cpu_header_fmt, cpu_header_read); + if (STRNEQ(cpu_header, cpu_header_read)) + continue; + for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i];
What about this?
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat,
while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2);
- if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i;
if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } + virStringFreeList(buf_header); }
virReportInvalidArg(cpuNum,
This is definitely better and lesser amount of code.. Thanks
-- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Regards, Pradipta
[snip]
for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i];
What about this?
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat,
while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2);
- if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i;
if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } + virStringFreeList(buf_header); }
virReportInvalidArg(cpuNum,
This is definitely better and lesser amount of code..
I think the version with virStringSplit would need some fine tuning since in its current form it will not free the memory for the failure cases..Comments ? Also can some expert here provide some tips on whether in this particular circumstance it should be fine to allocate/realloc/free memory inside the while loop. Would be very helpful.. Thanks, Pradipta
Thanks
-- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/20/2013 05:47 PM, Pradipta Kumar Banerjee wrote:
[snip]
for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i];
What about this?
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat,
while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2);
- if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i;
if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } + virStringFreeList(buf_header); }
virReportInvalidArg(cpuNum,
This is definitely better and lesser amount of code..
I think the version with virStringSplit would need some fine tuning since in its current form it will not free the memory for the failure cases..Comments ? Good point. We should add virStringFreeList(buf_header) to cleanup.
Also can some expert here provide some tips on whether in this particular circumstance it should be fine to allocate/realloc/free memory inside the while loop. Would be very helpful.. I think putting allocate/free memory inside the while is OK for me. Because we must loop for searching the expected line,the times of loop depends on the content of /proc/stat. However, as you said, we better get some advise from the experts here.
Thanks, Pradipta
Thanks
-- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best Regards, Bing Bu Cao
On 12/19/2013 12:40 PM, Pradipta Kr. Banerjee wrote:
From: "Pradipta Kr. Banerjee" <bpradip@in.ibm.com>
virsh nodecpustats erroneously returns stats for offline cpus on Linux
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0
and cpu 1,2 are offline, then
'virsh nodecpustats 1' displays data for cpu12
whereas virsh nodecpustats 2 correctly throws the following error
"error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats"
This patch fixes the above mentioned problem
Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_fmt[8];
if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat,
if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", 3); } else { snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); }
while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
+ /* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid + */ + sscanf(buf, cpu_header_fmt, cpu_header_read); + if (STRNEQ(cpu_header, cpu_header_read)) + continue; + for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i]; I think we can use strsplit() to handle this bug easily.
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat, while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; + char **buf_header = virStringSplit(buf, " ", 2); - if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i; if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } + virStringFreeList(buf_header); } virReportInvalidArg(cpuNum,
-- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best Regards, Bing Bu Cao
ping. On 12/19/2013 10:10 AM, Pradipta Kr. Banerjee wrote:
From: "Pradipta Kr. Banerjee" <bpradip@in.ibm.com>
virsh nodecpustats erroneously returns stats for offline cpus on Linux
To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0
and cpu 1,2 are offline, then
'virsh nodecpustats 1' displays data for cpu12
whereas virsh nodecpustats 2 correctly throws the following error
"error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats"
This patch fixes the above mentioned problem
Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/nodeinfo.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; + char cpu_header_fmt[8];
if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat,
if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", 3); } else { snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); + snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); }
while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }
+ /* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid + */ + sscanf(buf, cpu_header_fmt, cpu_header_read); + if (STRNEQ(cpu_header, cpu_header_read)) + continue; + for (i = 0; i < *nparams; i++) { virNodeCPUStatsPtr param = ¶ms[i];
-- 1.8.3.1
-- Regards, Pradipta
participants (5)
-
Bing Bu Cao -
Bing Bu Cao -
Pradipta Kr. Banerjee -
Pradipta Kumar Banerjee
-
Pradipta Kumar Banerjee