On 22.01.2014 10:53, Ján Tomko wrote:

Check if cpu stats are read correctly from a sample /proc/stat collected from a 24 CPU machine. --- src/libvirt_linux.syms | 1 + src/nodeinfo.c | 6 +- tests/nodeinfodata/linux-cpustat-24cpu.out | 150 ++++++++++++++++++++++++++++ tests/nodeinfodata/linux-cpustat-24cpu.stat | 25 +++++ tests/nodeinfotest.c | 131 ++++++++++++++++++++++++ 5 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.out create mode 100644 tests/nodeinfodata/linux-cpustat-24cpu.stat

diff --git a/src/libvirt_linux.syms b/src/libvirt_linux.syms index 3500898..b3b2384 100644 --- a/src/libvirt_linux.syms +++ b/src/libvirt_linux.syms @@ -3,6 +3,7 @@ #

# nodeinfo.h +linuxNodeGetCPUStats; linuxNodeInfoCPUPopulate;

# util/virstatslinux.h diff --git a/src/nodeinfo.c b/src/nodeinfo.c index cba2fc1..9c236cd 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -193,6 +193,10 @@ freebsdNodeGetMemoryStats(virNodeMemoryStatsPtr params, int linuxNodeInfoCPUPopulate(FILE *cpuinfo, const char *sysfs_dir, virNodeInfoPtr nodeinfo); +int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams);

/* Return the positive decimal contents of the given * DIR/cpu%u/FILE, or -1 on error. If DEFAULT_VALUE is non-negative @@ -681,7 +685,7 @@ cleanup:

# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))

-static int +int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, virNodeCPUStatsPtr params, diff --git a/tests/nodeinfodata/linux-cpustat-24cpu.out b/tests/nodeinfodata/linux-cpustat-24cpu.out new file mode 100644 index 0000000..0a1a5bc --- /dev/null +++ b/tests/nodeinfodata/linux-cpustat-24cpu.out @@ -0,0 +1,150 @@ +cpu: +kernel: 8751170 +user: 14128079 +idle: 1816344522 +iowait: 81323 + +cpu0: +kernel: 447603 +user: 749021 +idle: 75399242 +iowait: 5295 + +cpu1: +kernel: 167215 +user: 337326 +idle: 76178612 +iowait: 1121 + +cpu2: +kernel: 308930 +user: 666889 +idle: 75649696 +iowait: 4298 + +cpu3: +kernel: 227674 +user: 328464 +idle: 76131634 +iowait: 1219 + +cpu4: +kernel: 299514 +user: 583915 +idle: 75746383 +iowait: 3997 + +cpu5: +kernel: 112287 +user: 231867 +idle: 76336319 +iowait: 798 + +cpu6: +kernel: 546590 +user: 896252 +idle: 75132665 +iowait: 7210 + +cpu7: +kernel: 177715 +user: 342337 +idle: 76154889 +iowait: 1933 + +cpu8: +kernel: 452773 +user: 772479 +idle: 75359327 +iowait: 5845 + +cpu9: +kernel: 1050230 +user: 1079258 +idle: 74532776 +iowait: 3340 + +cpu10: +kernel: 535495 +user: 847295 +idle: 75202362 +iowait: 4038 + +cpu11: +kernel: 171635 +user: 323891 +idle: 76181622 +iowait: 993 + +cpu12: +kernel: 331031 +user: 683257 +idle: 75587176 +iowait: 5174 + +cpu13: +kernel: 112686 +user: 230633 +idle: 76345295 +iowait: 1367 + +cpu14: +kernel: 251393 +user: 547599 +idle: 75824554 +iowait: 5195 + +cpu15: +kernel: 199044 +user: 260673 +idle: 76230586 +iowait: 1379 + +cpu16: +kernel: 244158 +user: 463357 +idle: 75923993 +iowait: 6211 + +cpu17: +kernel: 88571 +user: 189253 +idle: 76411610 +iowait: 1388 + +cpu18: +kernel: 546539 +user: 875655 +idle: 75096896 +iowait: 5756 + +cpu19: +kernel: 186366 +user: 348768 +idle: 76137323 +iowait: 1299 + +cpu20: +kernel: 449460 +user: 765202 +idle: 75348938 +iowait: 4389 + +cpu21: +kernel: 1045076 +user: 1116075 +idle: 74500557 +iowait: 2411 + +cpu22: +kernel: 534125 +user: 847779 +idle: 75178185 +iowait: 5632 + +cpu23: +kernel: 265029 +user: 640815 +idle: 75753872 +iowait: 1026 +

There shouldn't be an empty line at EOF.

diff --git a/tests/nodeinfodata/linux-cpustat-24cpu.stat b/tests/nodeinfodata/linux-cpustat-24cpu.stat new file mode 100644 index 0000000..bc9d449 --- /dev/null +++ b/tests/nodeinfodata/linux-cpustat-24cpu.stat @@ -0,0 +1,25 @@ +cpu 14126233 1846 7764352 1816344522 81323 395581 591237 0 5880634 0 +cpu0 748997 24 320851 75399242 5295 22050 104702 0 331814 0 +cpu1 337325 1 140909 76178612 1121 8962 17344 0 166726 0 +cpu2 666860 29 269302 75649696 4298 18473 21155 0 272094 0 +cpu3 328387 77 211400 76131634 1219 9701 6573 0 115551 0 +cpu4 583896 19 265185 75746383 3997 17525 16804 0 253387 0 +cpu5 231867 0 100660 76336319 798 6856 4771 0 118465 0 +cpu6 896023 229 472933 75132665 7210 25811 47846 0 410328 0 +cpu7 342336 1 159567 76154889 1933 8675 9473 0 204523 0 +cpu8 772415 64 382065 75359327 5845 22810 47898 0 347169 0 +cpu9 1078771 487 1007467 74532776 3340 28419 14344 0 150374 0 +cpu10 847174 121 461786 75202362 4038 25206 48503 0 370309 0 +cpu11 323890 1 153521 76181622 993 9462 8652 0 199566 0 +cpu12 683237 20 290483 75587176 5174 19287 21261 0 293663 0 +cpu13 230633 0 100001 76345295 1367 7171 5514 0 103907 0 +cpu14 547593 6 220118 75824554 5195 14963 16312 0 207464 0 +cpu15 260648 25 185128 76230586 1379 8448 5468 0 76655 0 +cpu16 463328 29 214199 75923993 6211 14403 15556 0 184943 0 +cpu17 189247 6 79317 76411610 1388 5455 3799 0 85456 0 +cpu18 875552 103 470237 75096896 5756 25159 51143 0 408446 0 +cpu19 348767 1 167550 76137323 1299 8813 10003 0 208604 0 +cpu20 765169 33 380697 75348938 4389 21782 46981 0 353323 0 +cpu21 1115675 400 1003579 74500557 2411 28146 13351 0 162678 0 +cpu22 847629 150 463239 75178185 5632 24933 45953 0 376150 0 +cpu23 640804 11 244148 75753872 1026 13061 7820 0 479032 0 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 74f6d4d..6101551 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -31,6 +31,13 @@ extern int linuxNodeInfoCPUPopulate(FILE *cpuinfo, char *sysfs_dir, virNodeInfoPtr nodeinfo);

+extern int +linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virNodeCPUStatsPtr params, + int *nparams); + + static int linuxTestCompareFiles(const char *cpuinfofile, char *sysfs_dir, @@ -83,6 +90,93 @@ fail: return ret; }

+# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) + +static int +linuxCPUStatsToBuf(virBufferPtr buf, + int cpu, + virNodeCPUStatsPtr param, + size_t nparams) +{ + size_t i = 0; + + if (cpu < 0) + virBufferAddLit(buf, "cpu:\n"); + else + virBufferAsprintf(buf, "cpu%d:\n", cpu); + + for (i = 0; i < nparams; i++) + virBufferAsprintf(buf, "%s: %llu\n", param[i].field, + param[i].value / TICK_TO_NSEC); + + virBufferAddChar(buf, '\n'); + return 0; +} + +static int +linuxCPUStatsCompareFiles(const char *cpustatfile, + size_t ncpus, + const char *outfile) +{ + int ret = -1; + char *actualData = NULL; + char *expectData = NULL; + FILE *cpustat = NULL; + virNodeCPUStatsPtr params = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + size_t i; + int nparams = 0; + + if (virtTestLoadFile(outfile, &expectData) < 0) + goto fail; + + if (!(cpustat = fopen(cpustatfile, "r"))) { + virReportSystemError(errno, "failed to open '%s': ", cpustatfile); + goto fail; + } + + if (linuxNodeGetCPUStats(NULL, 0, NULL, &nparams) < 0) + goto fail; + + if (VIR_ALLOC_N(params, nparams) < 0) + goto fail; + + if (linuxNodeGetCPUStats(cpustat, VIR_NODE_CPU_STATS_ALL_CPUS, params, + &nparams) < 0) + goto fail; + + if (linuxCPUStatsToBuf(&buf, VIR_NODE_CPU_STATS_ALL_CPUS, + params, nparams) < 0) + goto fail; + + for (i = 0; i < ncpus; i++) { + if (linuxNodeGetCPUStats(cpustat, i, params, &nparams) < 0) + goto fail; + if (linuxCPUStatsToBuf(&buf, i, params, nparams) < 0) + goto fail; + } + + if (!(actualData = virBufferContentAndReset(&buf))) { + virReportOOMError(); + goto fail; + } + + if (STRNEQ(actualData, expectData)) { + virtTestDifference(stderr, expectData, actualData); + goto fail; + } + + ret = 0; + +fail: + virBufferFreeAndReset(&buf); + VIR_FORCE_FCLOSE(cpustat); + VIR_FREE(expectData); + VIR_FREE(actualData); + VIR_FREE(params); + return ret; +} +

static int linuxTestNodeInfo(const void *data) @@ -118,6 +212,34 @@ cleanup: return result; }

+struct nodeCPUStatsData { + const char *name; + int ncpus; +}; + +static int +linuxTestNodeCPUStats(const void *data) +{ + const struct nodeCPUStatsData *testData = data; + int result = -1; + char *cpustatfile = NULL; + char *outfile = NULL; + + if (virAsprintf(&cpustatfile, "%s/nodeinfodata/linux-cpustat-%s.stat", + abs_srcdir, testData->name) < 0 || + virAsprintf(&outfile, "%s/nodeinfodata/linux-cpustat-%s.out", + abs_srcdir, testData->name) < 0) + goto fail; + + result = linuxCPUStatsCompareFiles(cpustatfile, + testData->ncpus, + outfile); +fail: + VIR_FREE(cpustatfile); + VIR_FREE(outfile); + return result; +} +

static int mymain(void) @@ -145,6 +267,15 @@ mymain(void) if (virtTestRun(nodeData[i], linuxTestNodeInfo, nodeData[i]) != 0) ret = -1;

+# define DO_TEST_CPU_STATS(name, ncpus) \ + do { \ + static struct nodeCPUStatsData data = { name, ncpus }; \ + if (virtTestRun("CPU stats " name, linuxTestNodeCPUStats, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_CPU_STATS("24cpu", 24); + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; }

My overall status is - I like it, although with the nitpick that Dan pointed out I can't give you my ACK. BTW: It should be fairly simple to extend the suite to test the regression that was fixed just recently: commit 94f82053590795a67bd5fb38026c86fb5329e7b7 Author: Bing Bu Cao <mars@linux.vnet.ibm.com> AuthorDate: Thu Jan 16 16:18:09 2014 +0800 Commit: Michal Privoznik <mprivozn@redhat.com> CommitDate: Tue Jan 21 17:24:03 2014 +0100 linuxNodeGetCPUStats: Correctly handle cpu prefix To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function simply uses STRPREFIX() to match the cpuid with the one read from /proc/stat. However, as the file is read line by line it may happen, that some CPUs share the same prefix. So if user requested stats for the first CPU, which is offline, then there's no cpu1 in the stats file so the one that we match is cpu10. Which is obviously wrong. Fortunately, the IDs are terminated by a space, so we can utilize that. Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> Can you look at it too, please? Michal

On 22.01.2014 10:53, Ján Tomko wrote:

Split out the repetitive code. --- src/nodeinfo.c | 77 +++++++++++++++++++++++----------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 9c236cd..585da49 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -683,6 +683,20 @@ cleanup: return ret; }

+static int +virNodeCPUStatsAssign(virNodeCPUStatsPtr param, + const char *name, + unsigned long long value) +{ + if (virStrcpyStatic(param->field, name) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel cpu time too long for destination"));

When touching this, this error message is a bit hard to read; I've to read it multiple time to get the meaning. I suggest changing that to: kernel cpu time field is too long for the destination

+ return -1; + } + param->value = value; + return 0; +} + # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))

int @@ -721,8 +735,6 @@ linuxNodeGetCPUStats(FILE *procstat, char *buf = line;

if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ - size_t i; - if (sscanf(buf, "%*s %llu %llu %llu %llu %llu" // user ~ iowait "%llu %llu %llu %llu %llu", // irq ~ guest_nice @@ -731,51 +743,22 @@ linuxNodeGetCPUStats(FILE *procstat, continue; }

- for (i = 0; i < *nparams; i++) { - virNodeCPUStatsPtr param = ¶ms[i]; - - switch (i) { - case 0: /* fill kernel cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_KERNEL) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = (sys + irq + softirq) * TICK_TO_NSEC; - break; - - case 1: /* fill user cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_USER) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = (usr + ni) * TICK_TO_NSEC; - break; - - case 2: /* fill idle cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_IDLE) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = idle * TICK_TO_NSEC; - break; - - case 3: /* fill iowait cpu time here */ - if (virStrcpyStatic(param->field, VIR_NODE_CPU_STATS_IOWAIT) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field kernel cpu time too long for destination")); - goto cleanup; - } - param->value = iowait * TICK_TO_NSEC; - break; - - default: - break; - /* should not hit here */ - } - } + if (virNodeCPUStatsAssign(¶ms[0], VIR_NODE_CPU_STATS_KERNEL, + (sys + irq + softirq) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[1], VIR_NODE_CPU_STATS_USER, + (usr + ni) * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[2], VIR_NODE_CPU_STATS_IDLE, + idle * TICK_TO_NSEC) < 0) + goto cleanup; + + if (virNodeCPUStatsAssign(¶ms[3], VIR_NODE_CPU_STATS_IOWAIT, + iowait * TICK_TO_NSEC) < 0) + goto cleanup; + ret = 0; goto cleanup; }

ACK Michal