Re: [libvirt] [PATCH V4 1/5] Add new public API virDomainGetCPUStats()

On 01/27/2012 11:20 PM, KAMEZAWA Hiroyuki wrote: > > add new API virDomainGetCPUStats() for getting cpu accounting information > per real cpus which is used by a domain. > > based on ideas by Lai Jiangshan and Eric Blake. > Proposed API is a bit morified to be able to return max cpu ID. s/morified/modified/ > (max cpu ID != cpu num.) Nice extension to my proposal. And you made it - I'm going to push this today, so your API is definitely in 0.9.10, even if we need a few touchups discovered during testing during the freeze.

For example, it's fine if you don't have the Python implementation done before the freeze; that's something I don't mind taking during the freeze week on the grounds that it is rounding out a feature that we have committed to, and that it won't be altering the API itself. But of course, sooner means more review time and testing :) I'm hoisting the hunk from 4/5 on libvirt.h.in, where you define the first stat, "cpu_time".

> +++ b/src/libvirt.c > @@ -18017,3 +18017,139 @@ error: > virDispatchError(dom->conn); > return -1; > } > + > +/** > + * virDomainGetCPUStats: > + * @domain: domain to query > + * @params: array to populate on output > + * @nparams: number of parameters per cpu > + * @start_cpu: which cpu to start with, or -1 for summary > + * @ncpus: how many cpus to query > + * @flags: unused for now This should use the common text we have elsewhere.

> + * > + * Get statistics relating to CPU usage attributable to a single > + * domain (in contrast to the statistics returned by > + * virNodeGetCPUStats() for all processes on the host). @dom > + * must be running (an inactive domain has no attributable cpu > + * usage). On input, @params must contain at least @nparams * @ncpus > + * entries, allocated by the caller. > + * > + * Now, @ncpus is limited to be <= 128. If you want to get > + * values in a host with more cpus, you need to call multiple times. > + * > + * @nparams are limited to be <= 16 but maximum params per cpu > + * provided by will be smaller than this. I'd combine these limits, and mention that they mainly apply to remote protocols.

> + * > + * > + * If @start_cpu is -1, then @ncpus must be 1, and the returned > + * results reflect the statistics attributable to the entire > + * domain (such as user and system time for the process as a > + * whole). Otherwise, @start_cpu represents which cpu to start > + * with, and @ncpus represents how many consecutive processors to > + * query, with statistics attributable per processor (such as > + * per-cpu usage). > + * > + * As a special case, if @params is NULL and @nparams is 0 and > + * @ncpus is 1, and the return value will be how many > + * statistics are available for the given @start_cpu. This number > + * may be different for @start_cpu of -1 than for any non-negative > + * value, but will be the same for all non-negative @start_cpu. > + * > + * And, if @param is NULL and @ncparam is 0 and @ncpus is 0, s/ncparam/nparams/ > + * Max cpu id of the node is returned. (considering cpu hotplug, > + * max cpu id may be different from the number of cpu on a host.) Yes, this is useful. > + * > + * For now, @flags is unused, and the statistics all relate to the > + * usage from the host perspective; the number of cpus available to > + * query can be determined by the cpus member of the result of > + * virNodeGetInfo(), but it means this information is not quite right.

> even if the domain has had vcpus pinned to only > + * use a subset of overall host cpus. It is possible that a future > + * version will support a flag that queries the cpu usage from the > + * guest's perspective, using the number of vcpus available to the > + * guest, found by virDomainGetVcpusFlags(). An individual guest > + * vcpu cannot be reliably mapped back to a specific host cpu unless > + * a single-processor vcpu pinning was used, but when @start_cpu is -1, > + * any difference in usage between a host and guest perspective would > + * serve as a measure of hypervisor overhead. > + * > + * Returns -1 on failure, or the number of statistics that were > + * populated per cpu on success (this will be less than the total > + * number of populated @params, unless @ncpus was 1; and may be > + * less than @nparams). The populated parameters start at each > + * stride of @nparams; any unpopulated parameters will be zeroed > + * on success. The caller is responsible for freeing any returned > + * string parameters. The return paragraph should be last.

> + * > + * Note: > + * Because cpu ids may be discontig, retuned @param array may contain > + * zero-filled entry in the middle. This repeats part of the return paragraph.

> + * All time related values are represented in ns, using UULONG. This should be documented by the various macros for well-defined stat names. > + * > + * Typical use sequence is below. > + * > + * getting total stats: set start_cpu as -1, ncpus 1 > + * virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0) => nparams > + * VIR_ALLOC_N(params, nparams) Example code is written for apps that aren't using libvirt internals, therefore it must use calloc instead of VIR_ALLOC_N. > + * virDomainGetCPUStats(dom, parasm, nparams, -1, 1, 0) => total stats. s/parasm/params/ > + * > + * getting percpu stats: > + * virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0) => max_cpu_id > + * virDomainGetCPUStats(dom, NULL, 0, 0, 1, 0) => nparams > + * VIR_ALLOC_N(params, max_cpu_id * nparams) > + * virDomainGetCPUStats(dom, params, nparams, 0, max_cpu_id, 0) => percpu stats This should be moved up right after the documentation of special casing.

> + * > + */ > + > +int virDomainGetCPUStats(virDomainPtr domain, > + virTypedParameterPtr params, > + unsigned int nparams, > + int start_cpu, > + unsigned int ncpus, > + unsigned int flags) > +{ > + virConnectPtr conn; > + > + VIR_DOMAIN_DEBUG(domain, "nparams=%d, start_cpu=%d, ncpu=%d, flags=%x", Missing params.

> + nparams, start_cpu, ncpus, flags); > + virResetLastError(); > + > + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { > + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); > + virDispatchError(NULL); > + return -1; > + } > + > + conn = domain->conn; > + /* start_cpu == -1 is a special case. ncpus must be 1 */ > + if ((start_cpu == -1) && (ncpus != 1)) { > + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); > + goto error; We can further check that start_cpu is -1 or non-negative. > + } > + /* if params == NULL, nparams must be 0 */ > + if ((params == NULL) && ((nparams != 0))) { > + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); > + goto error; We can further check that nparams is non-zero if params is non-NULL. We can further check that ncpus is non-zero unless params is NULL. Since we don't distinguish the error message, we could join these conditionals.

> + } > + > + /* remote protocol doesn't welcome big args in one shot */ > + if ((nparams > 16) || (ncpus > 128)) { > + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); > + goto error; > + } This restriction should only be forced by the remote protocol. See virDomainMemoryPeek for an example of a documented RPC limitation, but which is only enforced in the RPC code.

> +++ b/src/libvirt_public.syms > @@ -520,6 +520,7 @@ LIBVIRT_0.9.10 { > global: > virDomainShutdownFlags; > virStorageVolWipePattern; > + virDomainGetCPUStats; I like to keep this sorted.

Overall, ACK - you picked up on the review suggestions, and the API looks good enough now to commit to. Here's what I'm squashing before pushing, which means we now have the API in place before the freeze! I'm not sure if I will finish reviewing the rest of the series today, seeing as it is my weekend, but we'll certainly get it all in before the release.