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: 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. 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. 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. Good, this looks like the correct way to divide things into slices of 128 cpus per read. 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. Here, I'd start the iteration on j = r - 1, since we already know r <= nparams, and that any slots beyond r are NULL. ... Skipping this part; it looks reasonable (keeping in mind that this is my first time reviewing ocaml bindings). 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. Not very consistent on your style of space before (. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org