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_oneoffs.c;h=f827707a77e6478129370fce67e46ae745b9be9a;hb=HEAD#l534 > 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@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