[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3 V7] add nodeGetCPUmap() for getting available CPU IDs in a cpumap.



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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]