On Sat, 28 Jan 2012 07:19:42 -0700
Eric Blake <eblake@xxxxxxxxxx> wrote:
> 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.
Thank you. good news :)
> 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".
>
Thanks.
> > +++ 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.
>
Sure.
> > + *
> > + * 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.
>
Ok, it will be better.
> > + *
> > + *
> > + * 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.
>
We'll check other users of calculation as max_cpu = sockets*cores*threads.
I wonder it may be better that NodeGetInfo() returns max_cpu "ID", online
cpu map etc..but I can't change the interface _virNodeInfo, 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.
>
Okay.
> > + *
> > + * 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.
>
Sorry.
> > + * 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.
>
That sounds better.
> > + *
> > + */
> > +
> > +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.
>
Ah, yes.
> > + 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.
>
I see.
> > + }
> > +
> > + /* 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.
>
Hmm, ok.
> > +++ 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.
>
Thank you for pushing and lots of fixes.
Regards,
-Kame
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list