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

On 01/27/2012 11:20 PM, KAMEZAWA Hiroyuki wrote: s/morified/modified/ 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". This should use the common text we have elsewhere. I'd combine these limits, and mention that they mainly apply to remote protocols. s/ncparam/nparams/ Yes, this is useful. but it means this information is not quite right. The return paragraph should be last. This repeats part of the return paragraph. This should be documented by the various macros for well-defined stat names. Example code is written for apps that aren't using libvirt internals, therefore it must use calloc instead of VIR_ALLOC_N. s/parasm/params/ This should be moved up right after the documentation of special casing. Missing params. We can further check that start_cpu is -1 or non-negative. 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. 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. 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. diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 10a8862..f55fac3 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -3811,6 +3811,14 @@ int virConnectSetKeepAlive(virConnectPtr conn, int interval, unsigned int count); +/* Collecting CPU statistics */ + +/** + * VIR_DOMAIN_CPU_STATS_CPUTIME: + * cpu usage in nanoseconds, as a ullong + */ +#define VIR_DOMAIN_CPU_STATS_CPUTIME "cpu_time" + int virDomainGetCPUStats(virDomainPtr domain, virTypedParameterPtr params, unsigned int nparams, diff --git i/src/libvirt.c w/src/libvirt.c index 6ff93cc..7efea67 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18159,7 +18159,7 @@ error: * @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 + * @flags: extra flags; not used yet, so callers should always pass 0 * * Get statistics relating to CPU usage attributable to a single * domain (in contrast to the statistics returned by @@ -18168,13 +18168,6 @@ error: * 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. - * - * * 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 @@ -18183,57 +18176,52 @@ error: * query, with statistics attributable per processor (such as * per-cpu usage). * - * As a special case, if @params is NULL and @nparams is 0 and + * The remote driver imposes a limit of 128 @ncpus and 16 @nparams; + * the number of parameters per cpu should not exceed 16, but if you + * have a host with more than 128 CPUs, your program should split + * the request into multiple calls. + * + * As special cases, 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, - * 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.) + * Likewise, if @params is NULL and @nparams is 0 and @ncpus is 0, + * the number of cpus available to query is returned. From the + * host perspective, this would typically match the cpus member + * of virNodeGetInfo(), but might be less due to host cpu hotplug. * * 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(), even if the domain has had vcpus pinned to only - * use a subset of overall host cpus. It is possible that a future + * usage from the host perspective. 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. - * - * Note: - * Because cpu ids may be discontig, retuned @param array may contain - * zero-filled entry in the middle. - * All time related values are represented in ns, using UULONG. + * guest's perspective, where the maximum cpu to query would be + * related to virDomainGetVcpusFlags() rather than virNodeGetInfo(). + * 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. * * 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) - * virDomainGetCPUStats(dom, parasm, nparams, -1, 1, 0) => total stats. + * params = calloc(nparams, sizeof (virTypedParameter)) + * virDomainGetCPUStats(dom, params, nparams, -1, 1, 0) => total stats. * - * getting percpu stats: - * virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0) => max_cpu_id + * getting per-cpu stats: + * virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0) => ncpus * 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 + * params = calloc(ncpus * nparams, sizeof (virTypedParameter)) + * virDomainGetCPUStats(dom, params, nparams, 0, ncpus, 0) => per-cpu stats * + * 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, which means the results may be discontiguous; + * any unpopulated parameters will be zeroed on success. The caller + * is responsible for freeing any returned string parameters. */ - int virDomainGetCPUStats(virDomainPtr domain, virTypedParameterPtr params, unsigned int nparams, @@ -18243,8 +18231,9 @@ int virDomainGetCPUStats(virDomainPtr domain, { virConnectPtr conn; - VIR_DOMAIN_DEBUG(domain, "nparams=%d, start_cpu=%d, ncpu=%d, flags=%x", - nparams, start_cpu, ncpus, flags); + VIR_DOMAIN_DEBUG(domain, + "params=%p, nparams=%d, start_cpu=%d, ncpus=%u, flags=%x", + params, nparams, start_cpu, ncpus, flags); virResetLastError(); if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -18254,13 +18243,16 @@ int virDomainGetCPUStats(virDomainPtr domain, } 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; - } - /* if params == NULL, nparams must be 0 */ - if ((params == NULL) && ((nparams != 0))) { + /* Special cases: + * start_cpu must be non-negative, or else -1 + * if start_cpu is -1, ncpus must be 1 + * params == NULL must match nparams == 0 + * ncpus must be non-zero unless params == NULL + */ + if (start_cpu < -1 || + (start_cpu == -1 && ncpus != 1) || + ((params == NULL) != (nparams == 0)) || + (ncpus == 0 && params != NULL)) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org