On 03/07/2012 03:36 AM, Richard W.M. Jones wrote:
TBH I found the documentation for virDomainGetCPUStats to be very confusing indeed. I couldn't really tell if virt-top is calling the API correctly or not, so I simply used Fujitsu's code directly.
That's a shame about the documentation not being clear; anything we can do to improve it? There are basically 5 calling modes: * Typical use sequence is below. * * getting total stats: set start_cpu as -1, ncpus 1 * virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0) => nparams * params = calloc(nparams, sizeof (virTypedParameter)) * virDomainGetCPUStats(dom, params, nparams, -1, 1, 0) => total stats. * * getting per-cpu stats: * virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0) => ncpus * virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0) => nparams * params = calloc(ncpus * nparams, sizeof (virTypedParameter)) * virDomainGetCPUStats(dom, params, nparams, 0, ncpus, 0) => per-cpu stats * 3 of the calling modes (when params is NULL) are there to let you determine how large to size your arrays; the remaining two modes exist to query the total stats, and to query a slice of up to 128 per-cpu stats. The number of total stats can be different from the number of per-cpu stats (right now, it's 1 each for qemu, but I have a pending patch that I will post later today that adds two new total stats with no per-cpu counterpart). When querying total stats, start_cpu is -1 and ncpus is 1 (this is a hard-coded requirement), so the return value is the number of stats populated. When querying per-cpu stats, the single 'params' pointer is actually representing a 2D array. So if you allocate params with 3x4 slots, and call virDomainGetCPUStats(dom, params, 3, 0, 4, 0), but there are only 3 online CPUs and the result of the call is 2, then the result will be laid out as: params[0] = CPU0 stat 0 params[1] = CPU0 stat 1 params[2] = NULL params[3] = CPU1 stat 0 params[4] = CPU1 stat 1 params[5] = NULL params[6] = CPU2 stat 0 params[7] = CPU2 stat 1 params[8] = NULL params[9] = NULL params[10] = NULL params[11] = NULL Furthermore, if you have a beefy system with more than 128 cpus, you have to break the call into chunks of 128 at a time, using start_cpu at 0, 128, and so forth.
Do you have any comments on whether this is correct or not?
http://git.annexia.org/?p=ocaml-libvirt.git;a=blob;f=libvirt/libvirt_c_oneof...
546 547 /* get percpu information */ 548 NONBLOCKING (nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0));
This gets the number of total params, but this might be different than the number of per-cpu params. I think you want this to use virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0) or even the maximum of the two, if you plan on combining total and per-cpu usage into the same call.
549 CHECK_ERROR (nparams < 0, conn, "virDomainGetCPUStats"); 550 551 if ((params = malloc(sizeof(*params) * nparams * 128)) == NULL)
Here, you are blindly sizing params based on the maximum supported by the API. It might be more efficient to s/128/nr_pcpus/ or even MIN(128, nr_pcpus). It would also be possible to use virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0), which should be the same or less than nr_pcpus, if I'm correctly understanding how nr_pcpus was computed.
552 caml_failwith ("virDomainGetCPUStats: malloc"); 553 554 cpustats = caml_alloc (nr_pcpus, 0); /* cpustats: array of params(list of typed_param) */ 555 cpu = 0; 556 while (cpu < nr_pcpus) { 557 ncpus = nr_pcpus - cpu > 128 ? 128 : nr_pcpus - cpu;
Good, this looks like the correct way to divide things into slices of 128 cpus per read.
558 559 NONBLOCKING (r = virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0));
Here, nparams should be at least as large as the value from virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0), since there might be more per-cpu stats than there are total stats. If nparams is too small, you will be silently losing out on those extra stats.
560 CHECK_ERROR (r < 0, conn, "virDomainGetCPUStats"); 561 562 for (i = 0; i < ncpus; i++) { 563 /* list of typed_param: single linked list of param_nodes */ 564 param_head = Val_emptylist; /* param_head: the head param_node of list of typed_param */ 565 566 if (params[i * nparams].type == 0) { 567 Store_field(cpustats, cpu + i, param_head); 568 continue; 569 } 570 571 for (j = nparams - 1; j >= 0; j--) {
Here, I'd start the iteration on j = r - 1, since we already know r <= nparams, and that any slots beyond r are NULL.
572 pos = i * nparams + j; 573 if (params[pos].type == 0) 574 continue; 575 576 param_node = caml_alloc(2, 0); /* param_node: typed_param, next param_node */ 577 Store_field(param_node, 1, param_head); 578 param_head = param_node; 579 580 typed_param = caml_alloc(2, 0); /* typed_param: field name(string), typed_param_value */ 581 Store_field(param_node, 0, typed_param); ... Skipping this part; it looks reasonable (keeping in mind that this is my first time reviewing ocaml bindings).
610 case VIR_TYPED_PARAM_STRING: 611 typed_param_value = caml_alloc (1, 6); 612 v = caml_copy_string (params[pos].value.s); 613 free (params[pos].value.s); 614 break; 615 default: 616 free (params);
Memory leak - if there are any VIR_TYPED_PARAM_STRING embedded later in params than where you reached in your iteration, you leaked that memory.
617 caml_failwith ("virDomainGetCPUStats: " 618 "unknown parameter type returned"); 619 } 620 Store_field (typed_param_value, 0, v); 621 Store_field (typed_param, 1, typed_param_value); 622 } 623 Store_field (cpustats, cpu + i, param_head); 624 } 625 cpu += ncpus; 626 } 627 free(params);
Not very consistent on your style of space before (.
628 CAMLreturn (cpustats);
This only grabs the per-cpu stats; it doesn't grab any total stats. Does anyone need the ocaml bindings to be able to grab the total stats? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org