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.
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list