[libvirt] [PATCH 1/3 V7] add nodeGetCPUmap() for getting available CPU IDs in a cpumap.
Eric Blake
eblake at redhat.com
Wed Mar 7 20:11:35 UTC 2012
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 at redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120307/b20cef62/attachment-0001.sig>
More information about the libvir-list
mailing list