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

[KAMEZAWA Hiroyuki]

On Sat, 28 Jan 2012 07:19:42 -0700
Eric Blake <eblake at redhat.com> 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