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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

[I see you cc'd Eric@redhat.com; unfortunately, that doesn't forward to me - at least I saw it on-list :) ] 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.

+++ b/include/libvirt/libvirt.h.in @@ -3797,6 +3797,12 @@ int virConnectSetKeepAlive(virConnectPtr conn, int interval, unsigned int count);

+int virDomainGetCPUStats(virDomainPtr domain, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags); #ifdef __cplusplus

Phooey - I didn't notice this earlier, but we like new API to occur in context (all the virDomain calls in sequence), and not slammed at the bottom of the file (where we sunk the deprecated interfaces). I'm pushing this followup as trivial (it is purely code motion and doc comments).diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index f55fac3..0a7b324 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -1127,6 +1127,10 @@ char * virConnectGetURI (virConnectPtr conn); char * virConnectGetSysinfo (virConnectPtr conn, unsigned int flags); +int virConnectSetKeepAlive(virConnectPtr conn, + int interval, + unsigned int count); + /* * Capabilities of the connection / driver. @@ -1299,14 +1303,29 @@ char * virDomainScreenshot (virDomainPtr domain, unsigned int flags); /* - * Domain runtime information + * Domain runtime information, and collecting CPU statistics */ + int virDomainGetInfo (virDomainPtr domain, virDomainInfoPtr info); int virDomainGetState (virDomainPtr domain, int *state, int *reason, unsigned int flags); + +/** + * 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, + int start_cpu, + unsigned int ncpus, + unsigned int flags); + int virDomainGetControlInfo (virDomainPtr domain, virDomainControlInfoPtr info, unsigned int flags); @@ -3807,24 +3826,6 @@ typedef struct _virTypedParameter virMemoryParameter; */ typedef virMemoryParameter *virMemoryParameterPtr; -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, - int start_cpu, - unsigned int ncpus, - unsigned int flags); #ifdef __cplusplus } #endif -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Changed from V3: use new virDomainGetCPUStats() libvirt-API. use C code to construct the typed_param list array Acked-by: "Richard W.M. Jones" <rjones@redhat.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- libvirt/libvirt.ml | 8 ++++ libvirt/libvirt.mli | 11 +++++ libvirt/libvirt_c_oneoffs.c | 91 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 0 deletions(-) diff --git a/libvirt/libvirt.ml b/libvirt/libvirt.ml index fc29264..08c501b 100644 --- a/libvirt/libvirt.ml +++ b/libvirt/libvirt.ml @@ -343,6 +343,13 @@ struct | SchedFieldInt64 of int64 | SchedFieldUInt64 of int64 | SchedFieldFloat of float | SchedFieldBool of bool + type typed_param = string * typed_param_value + and typed_param_value = + | TypedFieldInt32 of int32 | TypedFieldUInt32 of int32 + | TypedFieldInt64 of int64 | TypedFieldUInt64 of int64 + | TypedFieldFloat of float | TypedFieldBool of bool + | TypedFieldString of string + type migrate_flag = Live type memory_flag = Virtual @@ -419,6 +426,7 @@ struct external set_vcpus : [>`W] t -> int -> unit = "ocaml_libvirt_domain_set_vcpus" external pin_vcpu : [>`W] t -> int -> string -> unit = "ocaml_libvirt_domain_pin_vcpu" external get_vcpus : [>`R] t -> int -> int -> int * vcpu_info array * string = "ocaml_libvirt_domain_get_vcpus" + external get_cpu_stats : [>`R] t -> int -> typed_param list array = "ocaml_libvirt_domain_get_cpu_stats" external get_max_vcpus : [>`R] t -> int = "ocaml_libvirt_domain_get_max_vcpus" external attach_device : [>`W] t -> xml -> unit = "ocaml_libvirt_domain_attach_device" external detach_device : [>`W] t -> xml -> unit = "ocaml_libvirt_domain_detach_device" diff --git a/libvirt/libvirt.mli b/libvirt/libvirt.mli index 7bda889..31f2ca2 100644 --- a/libvirt/libvirt.mli +++ b/libvirt/libvirt.mli @@ -439,6 +439,13 @@ sig | SchedFieldInt64 of int64 | SchedFieldUInt64 of int64 | SchedFieldFloat of float | SchedFieldBool of bool + type typed_param = string * typed_param_value + and typed_param_value = + | TypedFieldInt32 of int32 | TypedFieldUInt32 of int32 + | TypedFieldInt64 of int64 | TypedFieldUInt64 of int64 + | TypedFieldFloat of float | TypedFieldBool of bool + | TypedFieldString of string + type migrate_flag = Live type memory_flag = Virtual @@ -586,6 +593,10 @@ sig for a domain. See the libvirt documentation for details of the array and bitmap returned from this function. *) + val get_cpu_stats : [>`R] t -> int -> typed_param list array + (** [get_pcpu_stats dom nr_pcpu] returns the physic CPU stats + for a domain. See the libvirt documentation for details. + *) val get_max_vcpus : [>`R] t -> int (** Returns the maximum number of vCPUs supported for this domain. *) val attach_device : [>`W] t -> xml -> unit diff --git a/libvirt/libvirt_c_oneoffs.c b/libvirt/libvirt_c_oneoffs.c index d87dd21..a0c74be 100644 --- a/libvirt/libvirt_c_oneoffs.c +++ b/libvirt/libvirt_c_oneoffs.c @@ -604,6 +604,97 @@ ocaml_libvirt_domain_get_vcpus (value domv, value maxinfov, value maplenv) CAMLreturn (rv); } +CAMLprim value +ocaml_libvirt_domain_get_cpu_stats (value domv, value nr_pcpusv) +{ + CAMLparam2 (domv, nr_pcpusv); + CAMLlocal5 (cpustats, param_head, param_node, typed_param, typed_param_value); + virDomainPtr dom = Domain_val (domv); + virConnectPtr conn = Connect_domv (domv); + int nr_pcpus = Int_val (nr_pcpusv); + virTypedParameterPtr params; + int r, cpu, ncpus, nparams, i, j, pos; + + /* get percpu information */ + NONBLOCKING (nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)); + CHECK_ERROR (nparams < 0, conn, "virDomainGetCPUStats"); + + if ((params = malloc(sizeof(*params) * nparams * 128)) == NULL) + caml_failwith ((char *)__FUNCTION__); + + cpustats = caml_alloc (nr_pcpus, 0); /* cpustats: array of params(list of typed_param) */ + cpu = 0; + while (cpu < nr_pcpus) { + ncpus = nr_pcpus - cpu > 128 ? 128 : nr_pcpus - cpu; + + NONBLOCKING (r = virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0)); + CHECK_ERROR (r < 0, conn, "virDomainGetCPUStats"); + + for (i = 0; i < ncpus; i++) { + /* list of typed_param: single linked list of param_nodes */ + param_head = Val_emptylist; /* param_head: the head param_node of list of typed_param */ + + if (params[i * nparams].type == 0) { + Store_field(cpustats, cpu + i, param_head); + continue; + } + + for (j = nparams - 1; j >= 0; j--) { + pos = i * nparams + j; + if (params[pos].type == 0) + continue; + + param_node = caml_alloc(2, 0); /* param_node: typed_param, next param_node */ + Store_field(param_node, 1, param_head); + param_head = param_node; + + typed_param = caml_alloc(2, 0); /* typed_param: field name(string), typed_param_value */ + Store_field(param_node, 0, typed_param); + Store_field(typed_param, 0, caml_copy_string(params[pos].field)); + + /* typed_param_value: value with the corresponding type tag */ + switch(params[pos].type) { + case VIR_TYPED_PARAM_INT: + typed_param_value = caml_alloc (1, 0); + Store_field(typed_param_value, 0, caml_copy_int32(params[pos].value.i)); + break; + case VIR_TYPED_PARAM_UINT: + typed_param_value = caml_alloc (1, 1); + Store_field(typed_param_value, 0, caml_copy_int32(params[pos].value.ui)); + break; + case VIR_TYPED_PARAM_LLONG: + typed_param_value = caml_alloc (1, 2); + Store_field(typed_param_value, 0, caml_copy_int64(params[pos].value.l)); + break; + case VIR_TYPED_PARAM_ULLONG: + typed_param_value = caml_alloc (1, 3); + Store_field(typed_param_value, 0, caml_copy_int64(params[pos].value.ul)); + break; + case VIR_TYPED_PARAM_DOUBLE: + typed_param_value = caml_alloc (1, 4); + Store_field(typed_param_value, 0, caml_copy_double(params[pos].value.d)); + break; + case VIR_TYPED_PARAM_BOOLEAN: + typed_param_value = caml_alloc (1, 5); + Store_field(typed_param_value, 0, Int_val(!!(params[pos].value.b))); + break; + case VIR_TYPED_PARAM_STRING: + typed_param_value = caml_alloc (1, 6); + Store_field(typed_param_value, 0, caml_copy_string(params[pos].value.s)); + break; + default: + free(params); + caml_failwith ((char *)__FUNCTION__); + } + Store_field(typed_param, 1, typed_param_value); + } + Store_field(cpustats, cpu + i, param_head); + } + cpu += ncpus; + } + free(params); + CAMLreturn (cpustats); +} #ifdef HAVE_WEAK_SYMBOLS #ifdef HAVE_VIRDOMAINMIGRATE extern virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn,

Hi, Richard virt-top and ocaml site patches can work and are completed weeks ago since public API of libvirt is merged. Could you merged them? virt-top and ocaml site patches only depend on the public API of libvirt. but the testing of them may depend on the qemu-driver-site patch of libvirt, which is sent today. Thanks, Lai On 02/08/2012 04:59 PM, Lai Jiangshan wrote:

Old "virt-top -1" is not correct, its output is generated by guess: use average usage for pinned physical CPUs.

example(old "virt-top -1"):

PHYCPU %CPU rhel6 Windows 0 0.6 0.1= 0.5= 1 0.6 0.1= 0.5=# 2 0.6 0.1= 0.5= 3 0.6 0.1=# 0.5=

The output almost makes no sense(all the value are just average, not real).

This is new implement, it use cpuacct cgroup to gain *real* physical usages via cpuacct cgroup by virDomainGetCPUStats() API.

new result:

PHYCPU %CPU rhel6 Windows 0 1.3 0.3 1.0 1 2.3 0.3 2.0 2 2.2 0.5 1.7 3 2.5 0.4 2.1

PHYCPU %CPU rhel6 Windows 0 1.7 0.4 1.3 1 3.6 1.0 2.7 2 1.6 0.4 1.2 3 4.8 3.1 1.7

Note: average flag(=) is dropped, there is not average value in here. Note: running flag(#) is dropped, because if the value is not empty, it means the guest was once running in the physical CPU in this period between updates.

Changed from V3: use new virDomainGetCPUStats() libvirt-API. add a new function find_usages_from_stats() to gain cpu usages.

Acked-by: "Richard W.M. Jones" <rjones@redhat.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- virt-top/virt_top.ml | 80 +++++++++++++++++++------------------------------ 1 files changed, 31 insertions(+), 49 deletions(-)

diff --git a/virt-top/virt_top.ml b/virt-top/virt_top.ml index ef5ac67..f7bd072 100644 --- a/virt-top/virt_top.ml +++ b/virt-top/virt_top.ml @@ -446,14 +446,14 @@ let collect, clear_pcpu_display_data = let last_info = Hashtbl.create 13 in let last_time = ref (Unix.gettimeofday ()) in

- (* Save vcpuinfo structures across redraws too (only for pCPU display). *) - let last_vcpu_info = Hashtbl.create 13 in + (* Save pcpu_usages structures across redraws too (only for pCPU display). *) + let last_pcpu_usages = Hashtbl.create 13 in

let clear_pcpu_display_data () = - (* Clear out vcpu_info used by PCPUDisplay display_mode + (* Clear out pcpu_info used by PCPUDisplay display_mode * when we switch back to TaskDisplay mode. *) - Hashtbl.clear last_vcpu_info + Hashtbl.clear last_pcpu_usages in

let collect (conn, _, _, _, _, node_info, _, _) = @@ -652,22 +652,28 @@ let collect, clear_pcpu_display_data = (try let domid = rd.rd_domid in let maplen = C.cpumaplen nr_pcpus in + let cpu_stats = D.get_cpu_stats rd.rd_dom nr_pcpus in + let rec find_usages_from_stats = (function + | ("cpu_time", D.TypedFieldUInt64(usages)) :: _ -> usages + | (_, _) :: params -> find_usages_from_stats params + | [] -> 0L) in + let pcpu_usages = Array.map find_usages_from_stats cpu_stats in let maxinfo = rd.rd_info.D.nr_virt_cpu in let nr_vcpus, vcpu_infos, cpumaps = D.get_vcpus rd.rd_dom maxinfo maplen in

- (* Got previous vcpu_infos for this domain? *) - let prev_vcpu_infos = - try Some (Hashtbl.find last_vcpu_info domid) + (* Got previous pcpu_usages for this domain? *) + let prev_pcpu_usages = + try Some (Hashtbl.find last_pcpu_usages domid) with Not_found -> None in - (* Update last_vcpu_info. *) - Hashtbl.replace last_vcpu_info domid vcpu_infos; - - (match prev_vcpu_infos with - | Some prev_vcpu_infos - when Array.length prev_vcpu_infos = Array.length vcpu_infos -> - Some (domid, name, nr_vcpus, vcpu_infos, prev_vcpu_infos, - cpumaps, maplen) + (* Update last_pcpu_usages. *) + Hashtbl.replace last_pcpu_usages domid pcpu_usages; + + (match prev_pcpu_usages with + | Some prev_pcpu_usages + when Array.length prev_pcpu_usages = Array.length pcpu_usages -> + Some (domid, name, nr_vcpus, vcpu_infos, pcpu_usages, + prev_pcpu_usages, cpumaps, maplen) | _ -> None (* ignore missing / unequal length prev_vcpu_infos *) ); with @@ -680,37 +686,15 @@ let collect, clear_pcpu_display_data = (* Rearrange the data into a matrix. Major axis (down) is * pCPUs. Minor axis (right) is domains. At each node we store: * cpu_time (on this pCPU only, nanosecs), - * average? (if set, then cpu_time is an average because the - * vCPU is pinned to more than one pCPU) - * running? (if set, we were instantaneously running on this pCPU) *) - let empty_node = (0L, false, false) in - let pcpus = Array.make_matrix nr_pcpus nr_doms empty_node in + let pcpus = Array.make_matrix nr_pcpus nr_doms 0L in

List.iteri ( - fun di (domid, name, nr_vcpus, vcpu_infos, prev_vcpu_infos, - cpumaps, maplen) -> + fun di (domid, name, nr_vcpus, vcpu_infos, pcpu_usages, + prev_pcpu_usages, cpumaps, maplen) -> (* Which pCPUs can this dom run on? *) - for v = 0 to nr_vcpus-1 do - let pcpu = vcpu_infos.(v).D.cpu in (* instantaneous pCPU *) - let nr_poss_pcpus = ref 0 in (* how many pcpus can it run on? *) - for p = 0 to nr_pcpus-1 do - (* vcpu v can reside on pcpu p *) - if C.cpu_usable cpumaps maplen v p then - incr nr_poss_pcpus - done; - let nr_poss_pcpus = Int64.of_int !nr_poss_pcpus in - for p = 0 to nr_pcpus-1 do - (* vcpu v can reside on pcpu p *) - if C.cpu_usable cpumaps maplen v p then - let vcpu_time_on_pcpu = - vcpu_infos.(v).D.vcpu_time - -^ prev_vcpu_infos.(v).D.vcpu_time in - let vcpu_time_on_pcpu = - vcpu_time_on_pcpu /^ nr_poss_pcpus in - pcpus.(p).(di) <- - (vcpu_time_on_pcpu, nr_poss_pcpus > 1L, p = pcpu) - done + for p = 0 to Array.length pcpu_usages - 1 do + pcpus.(p).(di) <- (pcpu_usages.(p) -^ prev_pcpu_usages.(p)) done ) doms;

@@ -719,7 +703,7 @@ let collect, clear_pcpu_display_data = fun row -> let cpu_time = ref 0L in for di = 0 to Array.length row-1 do - let t, _, _ = row.(di) in + let t = row.(di) in cpu_time := !cpu_time +^ t done; Int64.to_float !cpu_time @@ -938,7 +922,7 @@ let redraw = let dom_names = String.concat "" ( List.map ( - fun (_, name, _, _, _, _, _) -> + fun (_, name, _, _, _, _, _, _) -> let len = String.length name in let width = max (len+1) 7 in pad width name @@ -957,8 +941,8 @@ let redraw = addch ' ';

List.iteri ( - fun di (domid, name, _, _, _, _, _) -> - let t, is_average, is_running = pcpus.(p).(di) in + fun di (domid, name, _, _, _, _, _, _) -> + let t = pcpus.(p).(di) in let len = String.length name in let width = max (len+1) 7 in let str = @@ -966,9 +950,7 @@ let redraw = else ( let t = Int64.to_float t in let percent = 100. *. t /. total_cpu_per_pcpu in - sprintf "%s%c%c " (Show.percent percent) - (if is_average then '=' else ' ') - (if is_running then '#' else ' ') + sprintf "%s " (Show.percent percent) ) in addstr (pad width str); ()

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> add virBitmapParseCommaSeparetedFormat() for parsing bitmap in comma separeted ascii format. This format of bitmap is used in Linux sysfs and cpuset. Changelog: - fixed typos. - fixed string scan routine. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/libvirt_private.syms | 2 + src/nodeinfo.c | 51 ++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 4 +++ src/util/bitmap.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/bitmap.h | 6 +++- 5 files changed, 127 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6ad36c..14b144f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -17,6 +17,7 @@ virBitmapFree; virBitmapGetBit; virBitmapSetBit; virBitmapString; +virBitmapParseCommaSeparatedFormat; # buf.h @@ -797,6 +798,7 @@ nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; nodeGetMemoryStats; +nodeGetCPUmap; # nwfilter_conf.h diff --git a/src/nodeinfo.c b/src/nodeinfo.c index e0b66f7..b0ebb88 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -47,6 +47,7 @@ #include "count-one-bits.h" #include "intprops.h" #include "virfile.h" +#include "bitmap.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -569,6 +570,33 @@ int linuxNodeGetMemoryStats(FILE *meminfo, cleanup: return ret; } + +/* + * Linux maintains cpu bit map. For example, if cpuid=5's flag is not set + * and max cpu is 7. The map file shows 0-4,6-7. This functin parses + * it and returns bitmap. + */ +static virBitmapPtr linuxParseCPUmap(int *max_cpuid, const char *path) +{ + char str[1024]; + FILE *fp; + virBitmapPtr map = NULL; + + fp = fopen(path, "r"); + if (!fp) { + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + if (fgets(str, sizeof(str), fp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + map = virBitmapParseCommaSeparatedFormat(str, max_cpuid); + +cleanup: + VIR_FORCE_FCLOSE(fp); + return map; +} #endif int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -712,6 +740,29 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif } +virBitmapPtr nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED, + int *max_id, + const char *mapname) +{ +#ifdef __linux__ + char *path; + virBitmapPtr map; + + if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) { + virReportOOMError(); + return NULL; + } + + map = linuxParseCPUmap(max_id, path); + VIR_FREE(path); + return map; +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node cpumap not implemented on this platform")); + return -1; +#endif +} + #if HAVE_NUMACTL # if LIBNUMA_API_VERSION <= 1 # define NUMA_MAX_N_CPUS 4096 diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 4766152..7f26b77 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -26,6 +26,7 @@ # include "libvirt/libvirt.h" # include "capabilities.h" +# include "bitmap.h" int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); int nodeCapsInitNUMA(virCapsPtr caps); @@ -46,4 +47,7 @@ int nodeGetCellsFreeMemory(virConnectPtr conn, int maxCells); unsigned long long nodeGetFreeMemory(virConnectPtr conn); +virBitmapPtr nodeGetCPUmap(virConnectPtr conn, + int *max_id, + const char *mapname); #endif /* __VIR_NODEINFO_H__*/ diff --git a/src/util/bitmap.c b/src/util/bitmap.c index 8c326c4..6dead3b 100644 --- a/src/util/bitmap.c +++ b/src/util/bitmap.c @@ -33,6 +33,11 @@ #include "bitmap.h" #include "memory.h" #include "buf.h" +#include "c-ctype.h" +#include "util.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE struct _virBitmap { @@ -180,3 +185,63 @@ char *virBitmapString(virBitmapPtr bitmap) return virBufferContentAndReset(&buf); } + +/** + * virBitmapParseCommaSeparatedFormat: + * + * When bitmap is printed in ascii format, especially in Linux, + * comma-separated format is sometimes used. For example, a bitmap 11111011 is + * represetned as 0-4,6-7. This function parses comma-separated format + * and returns virBitmap. Found max bit is returned, too. + * + * This functions stops if characters other than digits, ',', '-' are + * found. This function will be useful for parsing linux's bitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) +{ + char *pos = buf; + virBitmapPtr map = NULL; + int val, x; + + /* at first, find the highest number */ + val = 0; + while ((*pos != '\n') && (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + } else if ((*pos == ',') || (*pos == '-')) { + ++pos; + } else + break; + } + *max_bit = val; + + map = virBitmapAlloc(val + 1); + if (map == NULL) { /* we may return NULL at failure of parsing */ + virReportOOMError(); + return NULL; + } + + pos = buf; + while ((*pos != '\n') && (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + if (virBitmapSetBit(map, val) < 0) + goto failed; + } else if (*pos == ',') { + ++pos; + } else if (*pos == '-') { + x = val; + pos++; + virStrToLong_i(pos, &pos, 10, &val); + for (;x <= val; ++x) { + if (virBitmapSetBit(map, x) < 0) + goto failed; + } + } else + break; + } + return map; +failed: + virBitmapFree(map); + return NULL; +} diff --git a/src/util/bitmap.h b/src/util/bitmap.h index ef62cca..e3d3e2b 100644 --- a/src/util/bitmap.h +++ b/src/util/bitmap.h @@ -61,5 +61,9 @@ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) char *virBitmapString(virBitmapPtr bitmap) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; - +/* + * parese comma-separeted bitmap format and allocate virBitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) + ATTRIBUTE_RETURN_CHECK; #endif -- 1.7.4.4

On 02/09/2012 03:43 AM, Lai Jiangshan wrote:

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

* Now, only "cpu_time" is supported. * cpuacct cgroup is used for providing percpu cputime information.

* include/libvirt/libvirt.h.in - defines VIR_DOMAIN_CPU_STATS_CPUTIME * src/qemu/qemu.conf - take care of cpuacct cgroup. * src/qemu/qemu_conf.c - take care of cpuacct cgroup. * src/qemu/qemu_driver.c - added an interface * src/util/cgroup.c/h - added interface for getting percpu cputime

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/qemu/qemu.conf | 3 +- src/qemu/qemu_conf.c | 3 +- src/qemu/qemu_driver.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 6 ++ src/util/cgroup.h | 1 + 5 files changed, 168 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 95428c1..db07b8a 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -166,6 +166,7 @@ # - 'memory' - use for memory tunables # - 'blkio' - use for block devices I/O tunables # - 'cpuset' - use for CPUs and memory nodes +# - cpuacct - use for CPUs statistics.

Needs consistent quoting with lines above.

+ +/* qemuDomainGetCPUStats() with start_cpu == -1 */ +static int qemuDomainGetTotalcpuStats(virDomainPtr domain ATTRIBUTE_UNUSED, + virCgroupPtr group, + virTypedParameterPtr params, + int nparams, + unsigned int flags)

Formatting nit: these days, we are preferring to start new function names on column 1, with the return type on the previous line, as in: static int qemuDomainGetTotalCpuStats(...) Why are you passing an unused parameter through to a static function? The only time that should happen is in a callback interface, but you are directly calling this function, so I'd prune the parameter instead.

+{ + unsigned long long cpu_time; + int param_idx = 0; + int ret; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);

No need to re-check flags; we already checked it up front in the primary entry point. At which point, you probably don't need the flags parameter in this function.

+ + if (nparams == 0) /* return supprted number of params */

s/supprted/supported/

+ return 1; + /* entry 0 is cputime */ + ret = virCgroupGetCpuacctUsage(group, &cpu_time); + if (ret < 0) { + virReportSystemError(-ret, "%s", _("unable to get cpu account")); + return -1; + } + + virTypedParameterAssign(¶ms[param_idx], VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + return param_idx + 1;

Why are you using the 'param_idx' variable, which is always 0? I'd simplify this to just 'return 1;', and worry about things if we ever add additional parameters later.

+} + +static int qemuDomainGetPercpuStats(virDomainPtr domain, + virCgroupPtr group, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + virBitmapPtr map = NULL; + int rv = -1; + int i, max_id; + char *pos; + char *buf = NULL; + virTypedParameterPtr ent; + int param_idx; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);

Again, why check flags here? It should have already been done in the caller.

+ /* return the number of supported params ? */ + if (nparams == 0 && ncpus != 0) + return 1; /* only cpu_time is supported */ + + /* return percpu cputime in index 0 */ + param_idx = 0; + /* to parse account file, we need "present" cpu map */ + map = nodeGetCPUmap(domain->conn, &max_id, "present");

I'm wondering if you can just use virDomainCpuSetParse to get a cpumap, and then use macros like VIR_CPU_USABLE in parsing that map, rather than going through virBitmap.

+ if (!map) + return rv; + + if (ncpus == 0) { /* returns max cpu ID */ + rv = max_id; + goto cleanup; + } + /* we get percpu cputime accoutning info. */

s/accoutning/accounting/

+ if (virCgroupGetCpuacctPercpuUsage(group, &buf)) + goto cleanup; + pos = buf; + + if (max_id > start_cpu + ncpus - 1) + max_id = start_cpu + ncpus - 1; + + for (i = 0; i <= max_id; i++) { + bool exist; + unsigned long long cpu_time; + + if (virBitmapGetBit(map, i, &exist) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, _("bitmap parse error")); + goto cleanup; + } + if (!exist) + continue; + if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cpuacct parse error")); + goto cleanup; + } + if (i < start_cpu) + continue; + ent = ¶ms[ (i - start_cpu) * nparams + param_idx]; + virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + } + rv = param_idx + 1;

This part looks right, even if we aren't incrementing param_idx until some future date when we add a second parameter.

+cleanup: + VIR_FREE(buf); + virBitmapFree(map); + return rv; +} + + +static int +qemuDomainGetCPUStats(virDomainPtr domain, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + struct qemud_driver *driver = domain->conn->privateData; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + bool isActive; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + qemuDriverLock(driver); + + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (vm == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), domain->uuid); + goto cleanup; + } + + isActive = virDomainObjIsActive(vm); + if (!isActive) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUACCT)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup CPUACCT controller is not mounted")); + goto cleanup; + } + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), vm->def->name); + goto cleanup; + } + + if (nparams == 0 && ncpus == 0) /* returns max cpu id */ + ret = qemuDomainGetPercpuStats(domain, group, NULL, 0, 0, 0, flags); + else if (start_cpu == -1) /* get total */ + ret = qemuDomainGetTotalcpuStats(domain, group, params, nparams, flags); + else + ret = qemuDomainGetPercpuStats(domain, group, params, nparams, + start_cpu, ncpus ,flags);

Formatting nit: s/ ,/, /

+cleanup: + virCgroupFree(&group); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = "QEMU", @@ -12193,6 +12349,7 @@ static virDriver qemuDriver = { .domainGetDiskErrors = qemuDomainGetDiskErrors, /* 0.9.10 */ .domainSetMetadata = qemuDomainSetMetadata, /* 0.9.10 */ .domainGetMetadata = qemuDomainGetMetadata, /* 0.9.10 */ + .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.10 */

We've now missed 0.9.10, so this should be 0.9.11. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/09/2012 03:43 AM, Lai Jiangshan wrote:

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Total: cpu_time 15.8 CPU0: cpu_time 8.6 CPU1: cpu_time 3.5 CPU2: cpu_time 2.4 CPU3: cpu_time 1.3

We should align the output so all '.' are in the same column. Given that the kernel gives us more than just tenths of a second, should we be exposing more granularity to the user?

/* + * "cputime" command

Wrong comment, since you installed it under "cpu-accts".

+ */ +static const vshCmdInfo info_cpu_accts[] = {

But I still don't like that name, either; since we're wrapping virDomainGetCPUStats, I think a better name would be "cpu-stats".

+ {"help", N_("show domain cpu accounting information")},

Here, I'd use "show domain cpu usage statistics",

+ {"desc", N_("Returns information about the domain's cpu accounting")},

and for the longer text, maybe "Display information about the domain's CPU usage, including per-CPU accounting"

+ {NULL, NULL}, +}; + +static const vshCmdOptDef opts_cpu_accts[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {NULL, 0, 0, NULL},

Should we expose the ability to pass in '--all' or an optional '--start=n --count=n' to let the user limit how much input they want? Given your above example, virsh cpu-stats dom => Stats for all CPUs, and a total virsh cpu-stats dom --all => Only the total stats virsh cpu-stats dom --start=2 => Only the per-cpu stats, for CPU 2 to the end virsh cpu-stats dom --start=2 --count=1 => Only the per-cpu stats of just CPU 2

+}; + +static bool +cmdCPUAccts(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + virTypedParameterPtr params = NULL; + bool ret = true; + int i, j, pos, cpu, nparams; + int max_id; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + /* get max cpu id on the node */ + max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0); + if (max_id < 0) { + vshPrint(ctl, "max id %d\n", max_id);

If max_id is less than 0, then it will be -1 and the command failed; we should print out the libvirt error at that point.

+ goto cleanup; + } + /* get supported num of parameter for total statistics */ + nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0); + if (nparams < 0) + goto cleanup; + + if (VIR_ALLOC_N(params, nparams)) { + virReportOOMError(); + goto cleanup; + } + /* passing start_cpu == -1 gives us domain's total status */ + nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, 0); + if (nparams < 0) + goto cleanup; + + vshPrint(ctl, "Total:\n"); + for (i = 0; i < nparams; i++) { + vshPrint(ctl, "\t%-15s ", params[i].field); + switch (params[i].type) { + case VIR_TYPED_PARAM_ULLONG: + /* Now all UULONG param is used for time accounting in ns */

That may not be true in the future. I'd rather see this specifically do a STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME) before reformatting the output...

+ vshPrint(ctl, "%.1lf\n", + params[i].value.ul / 1000000000.0); + break; + default: + vshError(ctl, "%s", _("API mismatch?")); + goto cleanup;

...and the default, rather than erroring out, should instead use vshGetTypedParamValue and output the name/value pair for all statistics that were unknown by this version of virsh but which may have been added in the server.

+ } + } + VIR_FREE(params);

Memory leak if you don't use virTypedParameterArrayClear first. Just because we don't pass strings back now doesn't mean we won't do it in the future.

+ /* get percpu information */ + nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0); + if (nparams < 0) + goto cleanup; + + cpu = 0; + while (cpu <= max_id) { + int ncpus = 128; + + if (cpu + ncpus - 1 > max_id) /* id starts from 0. */ + ncpus = max_id + 1 - cpu; + + if (VIR_ALLOC_N(params, nparams * ncpus)) {

Why are we re-allocating this each time through the loop? It should be enough to allocate it once before the loop, and reuse it thereafter.

+ virReportOOMError(); + goto cleanup; + } + + if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0) + goto cleanup; + + for (i = 0; i < ncpus; i++) { + if (params[i * nparams].type == 0) + continue;

This shouldn't be possible (if it is true, we have a bug in our server).

+ vshPrint(ctl, "CPU%d:\n", cpu + i); + + for (j = 0; j < nparams; j++) { + pos = i * nparams + j; + if (params[pos].type == 0) + continue;

This shouldn't be possible (if it is true, we have a bug in our server).

+ vshPrint(ctl, "\t%-15s ", params[pos].field); + switch(params[j].type) { + case VIR_TYPED_PARAM_ULLONG: + vshPrint(ctl, "%.1lf\n", + params[pos].value.ul / 1000000000.0); + break; + default: + vshError(ctl, "%s", _("API mismatch?")); + goto cleanup;

Same story about formatting - we want to format all name/value pairs, even those that were added in the server after this version of virsh was compiled; and we can only do the conversion from nanoseconds to fractional seconds if the field name is known.

+ } + } + } + cpu += ncpus; + /* Note: If we handle string type, it should be freed */ + VIR_FREE(params);

Your comment was right - you are missing a call to virTypedParameterArrayClear. And even if you hoist the array allocation outside of the loop, you still need an array clear in the loop between successive iterations.

+ } +cleanup: + VIR_FREE(params); + virDomainFree(dom); + return ret; +} + +/* * "inject-nmi" command */ static const vshCmdInfo info_inject_nmi[] = { @@ -16441,6 +16556,7 @@ static const vshCmdDef domManagementCmds[] = { #endif {"cpu-baseline", cmdCPUBaseline, opts_cpu_baseline, info_cpu_baseline, 0}, {"cpu-compare", cmdCPUCompare, opts_cpu_compare, info_cpu_compare, 0}, + {"cpu-accts", cmdCPUAccts, opts_cpu_accts, info_cpu_accts, 0}, {"create", cmdCreate, opts_create, info_create, 0}, {"define", cmdDefine, opts_define, info_define, 0}, {"desc", cmdDesc, opts_desc, info_desc, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index bd79b4c..2b2f70b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -762,6 +762,11 @@ Provide the maximum number of virtual CPUs supported for a guest VM on this connection. If provided, the I<type> parameter must be a valid type attribute for the <domain> element of XML.

+=item B<cpu-accts> I<domain> + +Provide cpu accounting information of a domain. The domain should +be running. + =item B<migrate> [I<--live>] [I<--direct>] [I<--p2p> [I<--tunnelled>]] [I<--persistent>] [I<--undefinesource>] [I<--suspend>] [I<--copy-storage-all>] [I<--copy-storage-inc>] [I<--change-protection>] [I<--verbose>]

-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> * Now, only "cpu_time" is supported. * cpuacct cgroup is used for providing percpu cputime information. * include/libvirt/libvirt.h.in - defines VIR_DOMAIN_CPU_STATS_CPUTIME * src/qemu/qemu.conf - take care of cpuacct cgroup. * src/qemu/qemu_conf.c - take care of cpuacct cgroup. * src/qemu/qemu_driver.c - added an interface * src/util/cgroup.c/h - added interface for getting percpu cputime Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/qemu/qemu.conf | 3 +- src/qemu/qemu_conf.c | 3 +- src/qemu/qemu_driver.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 6 ++ src/util/cgroup.h | 1 + 5 files changed, 158 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 95428c1..cb87728 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -166,6 +166,7 @@ # - 'memory' - use for memory tunables # - 'blkio' - use for block devices I/O tunables # - 'cpuset' - use for CPUs and memory nodes +# - 'cpuacct' - use for CPUs statistics. # # NB, even if configured here, they won't be used unless # the administrator has mounted cgroups, e.g.: @@ -177,7 +178,7 @@ # can be mounted in different locations. libvirt will detect # where they are located. # -# cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuset" ] +# cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuset", "cpuacct" ] # This is the basic set of devices allowed / required by # all virtual machines. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e95c7a5..a709cbf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -318,7 +318,8 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, (1 << VIR_CGROUP_CONTROLLER_DEVICES) | (1 << VIR_CGROUP_CONTROLLER_MEMORY) | (1 << VIR_CGROUP_CONTROLLER_BLKIO) | - (1 << VIR_CGROUP_CONTROLLER_CPUSET); + (1 << VIR_CGROUP_CONTROLLER_CPUSET) | + (1 << VIR_CGROUP_CONTROLLER_CPUACCT); } for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { if (driver->cgroupControllers & (1 << i)) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e2e73b7..e7871ab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12078,6 +12078,152 @@ cleanup: return ret; } + +/* qemuDomainGetCPUStats() with start_cpu == -1 */ +static int +qemuDomainGetTotalcpuStats(virCgroupPtr group, + virTypedParameterPtr params, + int nparams) +{ + unsigned long long cpu_time; + int param_idx = 0; + int ret; + + if (nparams == 0) /* return supported number of params */ + return 1; + /* entry 0 is cputime */ + ret = virCgroupGetCpuacctUsage(group, &cpu_time); + if (ret < 0) { + virReportSystemError(-ret, "%s", _("unable to get cpu account")); + return -1; + } + + virTypedParameterAssign(¶ms[param_idx], VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + return 1; +} + +static int qemuDomainGetPercpuStats(virDomainPtr domain, + virCgroupPtr group, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus) +{ + const char *map = NULL; + int rv = -1; + int i, max_id; + char *pos; + char *buf = NULL; + virTypedParameterPtr ent; + int param_idx; + + /* return the number of supported params ? */ + if (nparams == 0 && ncpus != 0) + return 1; /* only cpu_time is supported */ + + /* return percpu cputime in index 0 */ + param_idx = 0; + /* to parse account file, we need "present" cpu map */ + map = nodeGetCPUmap(domain->conn, &max_id, "present"); + if (!map) + return rv; + + if (ncpus == 0) { /* returns max cpu ID */ + rv = max_id; + goto cleanup; + } + /* we get percpu cputime accounting info. */ + if (virCgroupGetCpuacctPercpuUsage(group, &buf)) + goto cleanup; + pos = buf; + + if (max_id > start_cpu + ncpus - 1) + max_id = start_cpu + ncpus - 1; + + for (i = 0; i <= max_id; i++) { + unsigned long long cpu_time; + + if (!map[i]) + continue; + if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cpuacct parse error")); + goto cleanup; + } + if (i < start_cpu) + continue; + ent = ¶ms[ (i - start_cpu) * nparams + param_idx]; + virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + } + rv = param_idx + 1; +cleanup: + VIR_FREE(buf); + VIR_FREE(map); + return rv; +} + + +static int +qemuDomainGetCPUStats(virDomainPtr domain, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + struct qemud_driver *driver = domain->conn->privateData; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + bool isActive; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + qemuDriverLock(driver); + + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (vm == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), domain->uuid); + goto cleanup; + } + + isActive = virDomainObjIsActive(vm); + if (!isActive) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUACCT)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup CPUACCT controller is not mounted")); + goto cleanup; + } + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), vm->def->name); + goto cleanup; + } + + if (nparams == 0 && ncpus == 0) /* returns max cpu id */ + ret = qemuDomainGetPercpuStats(domain, group, NULL, 0, 0, 0); + else if (start_cpu == -1) /* get total */ + ret = qemuDomainGetTotalcpuStats(group, params, nparams); + else + ret = qemuDomainGetPercpuStats(domain, group, params, nparams, + start_cpu, ncpus); +cleanup: + virCgroupFree(&group); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = "QEMU", @@ -12235,6 +12381,7 @@ static virDriver qemuDriver = { .domainGetDiskErrors = qemuDomainGetDiskErrors, /* 0.9.10 */ .domainSetMetadata = qemuDomainSetMetadata, /* 0.9.10 */ .domainGetMetadata = qemuDomainGetMetadata, /* 0.9.10 */ + .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ }; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 15b870d..2d34c50 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1555,6 +1555,12 @@ int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage) "cpuacct.usage", usage); } +int virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage) +{ + return virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPUACCT, + "cpuacct.usage_percpu", usage); +} + int virCgroupSetFreezerState(virCgroupPtr group, const char *state) { return virCgroupSetValueStr(group, diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 8d75735..b4e0f37 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -115,6 +115,7 @@ int virCgroupSetCpuCfsQuota(virCgroupPtr group, long long cfs_quota); int virCgroupGetCpuCfsQuota(virCgroupPtr group, long long *cfs_quota); int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage); +int virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage); int virCgroupSetFreezerState(virCgroupPtr group, const char *state); int virCgroupGetFreezerState(virCgroupPtr group, char **state); -- 1.7.4.4

Hi, Eric Did you received these new patches? Thanks, Lai On 02/16/2012 03:15 PM, Lai Jiangshan wrote:

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Changelog: - fixed typos. - fixed string scan routine.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/nodeinfo.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 3 + 3 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c22dec..6e99243 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -792,6 +792,7 @@ virNodeDeviceObjUnlock;

# nodeinfo.h nodeCapsInitNUMA; +nodeGetCPUmap; nodeGetCPUStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index e0b66f7..fc1aaea 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -31,6 +31,7 @@ #include <dirent.h> #include <sys/utsname.h> #include <sched.h> +#include <conf/domain_conf.h>

#if HAVE_NUMACTL # define NUMA_VERSION1_COMPATIBILITY 1 @@ -569,6 +570,73 @@ int linuxNodeGetMemoryStats(FILE *meminfo, cleanup: return ret; } + +/* + * Linux maintains cpu bit map. For example, if cpuid=5's flag is not set + * and max cpu is 7. The map file shows 0-4,6-7. This function parses + * it and returns cpumap. + */ +static const char * +linuxParseCPUmap(int *max_cpuid, const char *path) +{ + FILE *fp; + char *map = NULL; + char *str = NULL; + size_t size = 128, pos = 0; + int max_id, i; + + fp = fopen(path, "r"); + if (!fp) { + virReportSystemError(errno, _("cannot open %s"), path); + goto error; + } + + if (VIR_ALLOC_N(str, size) < 0) { + virReportOOMError(); + goto error; + } + for (;;) { + pos += fread(str + pos, 1, size - pos, fp); + if (pos < size) + break; + + size = size << 1; + if (VIR_REALLOC_N(str, size) < 0) { + virReportOOMError(); + goto error; + } + } + if (pos == 0) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto error; + } + str[pos - 1] = 0; + + if (VIR_ALLOC_N(map, VIR_DOMAIN_CPUMASK_LEN) < 0) { + virReportOOMError(); + goto error; + } + if (virDomainCpuSetParse(str, 0, map, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + goto error; + } + + for (i = 0; i < VIR_DOMAIN_CPUMASK_LEN; i++) { + if (map[i]) { + max_id = i; + } + } + *max_cpuid = max_id; + + VIR_FORCE_FCLOSE(fp); + return map; + +error: + VIR_FORCE_FCLOSE(fp); + VIR_FREE(str); + VIR_FREE(map); + return NULL; +} #endif

int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -712,6 +780,30 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif }

+const char * +nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED, + int *max_id ATTRIBUTE_UNUSED, + const char *mapname ATTRIBUTE_UNUSED) +{ +#ifdef __linux__ + char *path; + const char *cpumap; + + if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) { + virReportOOMError(); + return NULL; + } + + cpumap = linuxParseCPUmap(max_id, path); + VIR_FREE(path); + return cpumap; +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node cpumap not implemented on this platform")); + return -1; +#endif +} + #if HAVE_NUMACTL # if LIBNUMA_API_VERSION <= 1 # define NUMA_MAX_N_CPUS 4096 diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 4766152..852e19d 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -46,4 +46,7 @@ int nodeGetCellsFreeMemory(virConnectPtr conn, int maxCells); unsigned long long nodeGetFreeMemory(virConnectPtr conn);

+const char *nodeGetCPUmap(virConnectPtr conn, + int *max_id, + const char *mapname); #endif /* __VIR_NODEINFO_H__*/

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> * Now, only "cpu_time" is supported. * cpuacct cgroup is used for providing percpu cputime information. * include/libvirt/libvirt.h.in - defines VIR_DOMAIN_CPU_STATS_CPUTIME * src/qemu/qemu.conf - take care of cpuacct cgroup. * src/qemu/qemu_conf.c - take care of cpuacct cgroup. * src/qemu/qemu_driver.c - added an interface * src/util/cgroup.c/h - added interface for getting percpu cputime Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/qemu/qemu.conf | 3 +- src/qemu/qemu_conf.c | 3 +- src/qemu/qemu_driver.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 6 ++ src/util/cgroup.h | 1 + 5 files changed, 157 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 95428c1..cb87728 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -166,6 +166,7 @@ # - 'memory' - use for memory tunables # - 'blkio' - use for block devices I/O tunables # - 'cpuset' - use for CPUs and memory nodes +# - 'cpuacct' - use for CPUs statistics. # # NB, even if configured here, they won't be used unless # the administrator has mounted cgroups, e.g.: @@ -177,7 +178,7 @@ # can be mounted in different locations. libvirt will detect # where they are located. # -# cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuset" ] +# cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuset", "cpuacct" ] # This is the basic set of devices allowed / required by # all virtual machines. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e95c7a5..a709cbf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -318,7 +318,8 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, (1 << VIR_CGROUP_CONTROLLER_DEVICES) | (1 << VIR_CGROUP_CONTROLLER_MEMORY) | (1 << VIR_CGROUP_CONTROLLER_BLKIO) | - (1 << VIR_CGROUP_CONTROLLER_CPUSET); + (1 << VIR_CGROUP_CONTROLLER_CPUSET) | + (1 << VIR_CGROUP_CONTROLLER_CPUACCT); } for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { if (driver->cgroupControllers & (1 << i)) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c6bdd29..d6b56ae 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12087,6 +12087,151 @@ cleanup: return ret; } +/* qemuDomainGetCPUStats() with start_cpu == -1 */ +static int +qemuDomainGetTotalcpuStats(virCgroupPtr group, + virTypedParameterPtr params, + int nparams) +{ + unsigned long long cpu_time; + int param_idx = 0; + int ret; + + if (nparams == 0) /* return supported number of params */ + return 1; + /* entry 0 is cputime */ + ret = virCgroupGetCpuacctUsage(group, &cpu_time); + if (ret < 0) { + virReportSystemError(-ret, "%s", _("unable to get cpu account")); + return -1; + } + + virTypedParameterAssign(¶ms[param_idx], VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + return 1; +} + +static int qemuDomainGetPercpuStats(virDomainPtr domain, + virCgroupPtr group, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus) +{ + const char *map = NULL; + int rv = -1; + int i, max_id; + char *pos; + char *buf = NULL; + virTypedParameterPtr ent; + int param_idx; + + /* return the number of supported params ? */ + if (nparams == 0 && ncpus != 0) + return 1; /* only cpu_time is supported */ + + /* return percpu cputime in index 0 */ + param_idx = 0; + /* to parse account file, we need "present" cpu map */ + map = nodeGetCPUmap(domain->conn, &max_id, "present"); + if (!map) + return rv; + + if (ncpus == 0) { /* returns max cpu ID */ + rv = max_id; + goto cleanup; + } + /* we get percpu cputime accounting info. */ + if (virCgroupGetCpuacctPercpuUsage(group, &buf)) + goto cleanup; + pos = buf; + + if (max_id > start_cpu + ncpus - 1) + max_id = start_cpu + ncpus - 1; + + for (i = 0; i <= max_id; i++) { + unsigned long long cpu_time; + + if (!map[i]) + continue; + if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cpuacct parse error")); + goto cleanup; + } + if (i < start_cpu) + continue; + ent = ¶ms[ (i - start_cpu) * nparams + param_idx]; + virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, cpu_time); + } + rv = param_idx + 1; +cleanup: + VIR_FREE(buf); + VIR_FREE(map); + return rv; +} + + +static int +qemuDomainGetCPUStats(virDomainPtr domain, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + struct qemud_driver *driver = domain->conn->privateData; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + bool isActive; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + qemuDriverLock(driver); + + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (vm == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), domain->uuid); + goto cleanup; + } + + isActive = virDomainObjIsActive(vm); + if (!isActive) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUACCT)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup CPUACCT controller is not mounted")); + goto cleanup; + } + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), vm->def->name); + goto cleanup; + } + + if (nparams == 0 && ncpus == 0) /* returns max cpu id */ + ret = qemuDomainGetPercpuStats(domain, group, NULL, 0, 0, 0); + else if (start_cpu == -1) /* get total */ + ret = qemuDomainGetTotalcpuStats(group, params, nparams); + else + ret = qemuDomainGetPercpuStats(domain, group, params, nparams, + start_cpu, ncpus); +cleanup: + virCgroupFree(&group); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + static int qemuDomainPMSuspendForDuration(virDomainPtr dom, unsigned int target, @@ -12387,6 +12532,7 @@ static virDriver qemuDriver = { .domainGetMetadata = qemuDomainGetMetadata, /* 0.9.10 */ .domainPMSuspendForDuration = qemuDomainPMSuspendForDuration, /* 0.9.11 */ .domainPMWakeup = qemuDomainPMWakeup, /* 0.9.11 */ + .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ }; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 15b870d..2d34c50 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1555,6 +1555,12 @@ int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage) "cpuacct.usage", usage); } +int virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage) +{ + return virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPUACCT, + "cpuacct.usage_percpu", usage); +} + int virCgroupSetFreezerState(virCgroupPtr group, const char *state) { return virCgroupSetValueStr(group, diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 8d75735..b4e0f37 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -115,6 +115,7 @@ int virCgroupSetCpuCfsQuota(virCgroupPtr group, long long cfs_quota); int virCgroupGetCpuCfsQuota(virCgroupPtr group, long long *cfs_quota); int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage); +int virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage); int virCgroupSetFreezerState(virCgroupPtr group, const char *state); int virCgroupGetFreezerState(virCgroupPtr group, char **state); -- 1.7.4.4

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Total: cpu_time 14.312 CPU0: cpu_time 3.253 CPU1: cpu_time 1.923 CPU2: cpu_time 7.424 CPU3: cpu_time 1.712 Changed from V5: add --all, --start, --count option Changed from V: rebase Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- tools/virsh.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 8 +++ 2 files changed, 162 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index dc362dc..a431826 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5537,6 +5537,159 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) } /* + * "cpu-stats" command + */ +static const vshCmdInfo info_cpu_stats[] = { + {"help", N_("show domain cpu statistics")}, + {"desc", N_("Display statistics about the domain's CPUs, including per-CPU statistics.")}, + {NULL, NULL}, +}; + +static const vshCmdOptDef opts_cpu_stats[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"all", VSH_OT_BOOL, 0, N_("Show total statistics only")}, + {"start", VSH_OT_INT, 0, N_("Show statistics from this CPU")}, + {"count", VSH_OT_INT, 0, N_("Number of shown CPUs at most")}, + {NULL, 0, 0, NULL}, +}; + +static bool +cmdCPUStats(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + virTypedParameterPtr params = NULL; + int i, j, pos, max_id, cpu = -1, show_count = -1, nparams; + bool show_all = false, show_per_cpu = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + show_all = vshCommandOptBool(cmd, "all"); + if (vshCommandOptInt(cmd, "start", &cpu) > 0) + show_per_cpu = true; + if (vshCommandOptInt(cmd, "count", &show_count) > 0) + show_per_cpu = true; + + /* default show per_cpu and total */ + if (!show_all && !show_per_cpu) { + show_all = true; + show_per_cpu = true; + } + + if (!show_all) + goto do_show_per_cpu; + + /* get supported num of parameter for total statistics */ + if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0) + goto failed_stats; + if (VIR_ALLOC_N(params, nparams)) + goto failed_params; + + /* passing start_cpu == -1 gives us domain's total status */ + if ((nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, 0)) < 0) + goto failed_stats; + + vshPrint(ctl, "Total:\n"); + for (i = 0; i < nparams; i++) { + vshPrint(ctl, "\t%-10s ", params[i].field); + if (STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) { + if (params[i].type == VIR_TYPED_PARAM_ULLONG) { + vshPrint(ctl, "%12.3lf\n", + params[i].value.ul / 1000000000.0); + } else { + const char *s = vshGetTypedParamValue(ctl, ¶ms[i]); + vshPrint(ctl, _("%s(ABI changed? ullong is expected)\n"), s); + VIR_FREE(s); + } + } else { + const char *s = vshGetTypedParamValue(ctl, ¶ms[i]); + vshPrint(ctl, _("%s\n"), s); + VIR_FREE(s); + } + } + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); + + if (!show_per_cpu) /* show all stats only */ + goto cleanup; + +do_show_per_cpu: + /* check cpu, show_count, and ignore wrong argument */ + if (cpu < 0) + cpu = 0; + if (show_count < 0) + show_count = INT_MAX; + + /* get max cpu id on the node */ + if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0)) < 0) + goto failed_stats; + /* get percpu information */ + if ((nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0)) < 0) + goto failed_stats; + + if (VIR_ALLOC_N(params, nparams * 128)) + goto failed_params; + + while (cpu <= max_id) { + int ncpus = 128; + + if (cpu + ncpus - 1 > max_id) /* id starts from 0. */ + ncpus = max_id + 1 - cpu; + + if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0) + goto failed_stats; + + for (i = 0; i < ncpus; i++) { + if (params[i * nparams].type == 0) /* this cpu is not in the map */ + continue; + vshPrint(ctl, "CPU%d:\n", cpu + i); + + for (j = 0; j < nparams; j++) { + pos = i * nparams + j; + vshPrint(ctl, "\t%-10s ", params[pos].field); + if (STREQ(params[pos].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) { + if (params[j].type == VIR_TYPED_PARAM_ULLONG) { + vshPrint(ctl, "%12.3lf\n", + params[pos].value.ul / 1000000000.0); + } else { + const char *s = vshGetTypedParamValue(ctl, ¶ms[pos]); + vshPrint(ctl, _("%s(ABI changed? ullong is expected)\n"), s); + VIR_FREE(s); + } + } else { + const char *s = vshGetTypedParamValue(ctl, ¶ms[pos]); + vshPrint(ctl, _("%s\n"), s); + VIR_FREE(s); + } + } + + if (--show_count <= 0) /* mark done to exit the outmost loop */ + cpu = max_id; + } + cpu += ncpus; + virTypedParameterArrayClear(params, nparams * ncpus); + } + VIR_FREE(params); +cleanup: + virDomainFree(dom); + return true; + +failed_params: + virReportOOMError(); + virDomainFree(dom); + return false; + +failed_stats: + vshError(ctl, _("Failed to virDomainGetCPUStats()\n")); + VIR_FREE(params); + virDomainFree(dom); + return false; +} + +/* * "inject-nmi" command */ static const vshCmdInfo info_inject_nmi[] = { @@ -16910,6 +17063,7 @@ static const vshCmdDef domManagementCmds[] = { #endif {"cpu-baseline", cmdCPUBaseline, opts_cpu_baseline, info_cpu_baseline, 0}, {"cpu-compare", cmdCPUCompare, opts_cpu_compare, info_cpu_compare, 0}, + {"cpu-stats", cmdCPUStats, opts_cpu_stats, info_cpu_stats, 0}, {"create", cmdCreate, opts_create, info_create, 0}, {"define", cmdDefine, opts_define, info_define, 0}, {"desc", cmdDesc, opts_desc, info_desc, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 8f6a2d6..b23868d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -790,6 +790,14 @@ Provide the maximum number of virtual CPUs supported for a guest VM on this connection. If provided, the I<type> parameter must be a valid type attribute for the <domain> element of XML. +=item B<cpu-stats> I<domain> [I<--all>] [I<start>] [I<count>] + +Provide cpu statistics information of a domain. The domain should +be running. Default it shows stats for all CPUs, and a total. Use +I<--all> for only the total stats, I<start> for only the per-cpu +stats of the CPUs from I<start>, I<count> for only I<count> CPUs' +stats. + =item B<migrate> [I<--live>] [I<--direct>] [I<--p2p> [I<--tunnelled>]] [I<--persistent>] [I<--undefinesource>] [I<--suspend>] [I<--copy-storage-all>] [I<--copy-storage-inc>] [I<--change-protection>] [I<--unsafe>] [I<--verbose>] -- 1.7.4.4

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/libvirt.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 8035add..a55d823 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18541,7 +18541,7 @@ int virDomainGetCPUStats(virDomainPtr domain, (start_cpu == -1 && ncpus != 1) || ((params == NULL) != (nparams == 0)) || (ncpus == 0 && params != NULL) || - ncpus < UINT_MAX / nparams) { + (nparams && ncpus > UINT_MAX / nparams)) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } -- 1.7.4.4

On 02/09/2012 03:43 AM, Lai Jiangshan wrote:

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

add virBitmapParseCommaSeparetedFormat() for parsing bitmap in

s/Separeted/Separated/

comma separeted ascii format.

and again

This format of bitmap is used in Linux sysfs and cpuset.

Changelog: - fixed typos. - fixed string scan routine.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/libvirt_private.syms | 2 + src/nodeinfo.c | 51 ++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 4 +++ src/util/bitmap.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/bitmap.h | 6 +++- 5 files changed, 127 insertions(+), 1 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6ad36c..14b144f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -17,6 +17,7 @@ virBitmapFree; virBitmapGetBit; virBitmapSetBit; virBitmapString; +virBitmapParseCommaSeparatedFormat;

Sorting.

# buf.h @@ -797,6 +798,7 @@ nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; nodeGetMemoryStats; +nodeGetCPUmap;

# nwfilter_conf.h diff --git a/src/nodeinfo.c b/src/nodeinfo.c index e0b66f7..b0ebb88 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -47,6 +47,7 @@ #include "count-one-bits.h" #include "intprops.h" #include "virfile.h" +#include "bitmap.h"

#define VIR_FROM_THIS VIR_FROM_NONE @@ -569,6 +570,33 @@ int linuxNodeGetMemoryStats(FILE *meminfo, cleanup: return ret; } + +/* + * Linux maintains cpu bit map. For example, if cpuid=5's flag is not set + * and max cpu is 7. The map file shows 0-4,6-7. This functin parses

s/functin/function/

+ * it and returns bitmap. + */ +static virBitmapPtr linuxParseCPUmap(int *max_cpuid, const char *path)

How is this function different from virDomainCpuSetParse in domain_conf.c? That function parses out a cpuset rather than a bitmap, but the syntax string being parsed looks to be a superset of what you are parsing here. Maybe it's more efficient to write a generic conversion from cpumap to bitset, and reuse the existing parser. I'll have to see how you actually use the parsed bitset later in the series to see which approach might be best.

+{ + char str[1024]; + FILE *fp; + virBitmapPtr map = NULL; + + fp = fopen(path, "r"); + if (!fp) { + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + if (fgets(str, sizeof(str), fp) == NULL) {

Are we sure that is long enough? Suppose I have a machine with 512 cores - I can come up with a cpumap that exceeds 1024 bytes.

+ virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + map = virBitmapParseCommaSeparatedFormat(str, max_cpuid); + +cleanup: + VIR_FORCE_FCLOSE(fp); + return map; +} #endif

int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -712,6 +740,29 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif }

+virBitmapPtr nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED, + int *max_id,

max_id has to be marked unused, to avoid gcc warnings on non-Linux compilation.

+ const char *mapname)

same for mapname.

+{ +#ifdef __linux__ + char *path; + virBitmapPtr map; + + if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) { + virReportOOMError(); + return NULL; + } + + map = linuxParseCPUmap(max_id, path); + VIR_FREE(path); + return map; +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node cpumap not implemented on this platform")); + return -1; +#endif +} +

+ +/** + * virBitmapParseCommaSeparatedFormat: + * + * When bitmap is printed in ascii format, especially in Linux, + * comma-separated format is sometimes used. For example, a bitmap 11111011 is + * represetned as 0-4,6-7. This function parses comma-separated format

s/represetned/represented/

+ * and returns virBitmap. Found max bit is returned, too. + * + * This functions stops if characters other than digits, ',', '-' are + * found. This function will be useful for parsing linux's bitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit)

const char *buf, since we aren't modifying it.

+{ + char *pos = buf;

const char *pos, since buf is const.

+ virBitmapPtr map = NULL; + int val, x; + + /* at first, find the highest number */ + val = 0; + while ((*pos != '\n') && (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + } else if ((*pos == ',') || (*pos == '-')) { + ++pos; + } else + break; + } + *max_bit = val;

Yeah, I'm definitely starting to think that reusing our existing cpumap parser, then a loop that converts bytes of the cpumap into longs of a virBitmap, might be a more reusable prospect.

- +/* + * parese comma-separeted bitmap format and allocate virBitmap.

s/parese/parse/; s/separeted/separated/

+ */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) + ATTRIBUTE_RETURN_CHECK;

ATTRIBUTE_NONNULL(1), and decide whether max_bit can be NULL (if so, fix the code to allow it; if not, add the attribute) If we go with my reuse suggestion, this would be: virBitmapPtr virBitmapCreateFromCpumap(char *cpumap, int maplen, int *max_bit) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/27/2012 11:21 PM, KAMEZAWA Hiroyuki wrote:

remote protocol driver for virDomainGetCPUStats()

Note: Unlike other users of virTypedParameter with RPC, this interface can return zero-filled entries because the interface assumes 2 dimentional array. Then, the function has its own serialize/deserialize

s/dimentional/dimensional/

routine.

daemon/remote.c | 146 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 117 ++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 22 ++++++ src/remote_protocol-structs | 15 ++++ --- daemon/remote.c | 147 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 117 +++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 22 ++++++- src/remote_protocol-structs | 15 +++

Odd that the diffstat listed twice. I'm going to reorder my review, to start with the .x file.

diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 0f354bb..205aeeb 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -208,6 +208,12 @@ const REMOTE_DOMAIN_SEND_KEY_MAX = 16; */ const REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX = 16;

+/* + * Upper limit on list of (real) cpu parameters + * nparams(<=16) * ncpus(<=128) <= 2048. + */ +const REMOTE_DOMAIN_GET_CPU_STATS_MAX = 2048;

We should enforce the two limits independently, which means we also need two smaller constants (it's a shame that rpcgen doesn't allow products when computing a constant, but we can document how we arrive at various numbers). We can reuse VIR_NODE_CPU_STATS_MAX for one of the two limits.

+ /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

@@ -1149,6 +1155,19 @@ struct remote_domain_get_block_io_tune_ret { int nparams; };

+struct remote_domain_get_cpu_stats_args { + remote_nonnull_domain dom; + unsigned int nparams; + int start_cpu; + unsigned int ncpus; + unsigned int flags; +}; + +struct remote_domain_get_cpu_stats_ret { + remote_typed_param params<REMOTE_DOMAIN_GET_CPU_STATS_MAX>; + int nparams; +};

Looks good.

+ /* Network calls: */

struct remote_num_of_networks_ret { @@ -2667,7 +2686,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */ - REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259 /* autogen autogen */ + REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */ + REMOTE_PROC_DOMAIN_GET_CPU_STATS = 260 /* skipgen skipgen */

Trivial conflict resolution.

+static int +remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_get_cpu_stats_args *args, + remote_domain_get_cpu_stats_ret *ret) +{ + virDomainPtr dom = NULL; + struct daemonClientPrivate *priv; + virTypedParameterPtr params = NULL; + remote_typed_param *info = NULL; + int cpu, idx, src, dest; + int rv = -1; + unsigned int return_size = 0; + int percpu_len = 0; + + priv = virNetServerClientGetPrivateData(client); + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + /* + * nparams * ncpus should be smaller than + * REMOTE_DOMAIN_GET_CPU_STATS_MAX but nparams + * and ncpus are limited by API. check it. + */ + if (args->nparams > 16) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + if (args->ncpus > 128) {

Use the constants from the .x file, rather than open-coding things. Oops, I just realized that I mentioned that libvirt.c should not have a length check, but then I forgot to delete it. Meanwhile, even if libvirt.c does not have a length check, it still needs to have a multiplication overflow check.

+ /* + * Here, we return values based on the real param size returned by + * a driver rather than the passed one. RPC Client stub should decode + * it to fit callar's expectation.

s/callar/caller/

+ * + * If cpu ID is sparse, The whole array for some cpu IDs will be + * zero-cleared. This doesn't meet to remoteSerializeTypedParameters() + * and we do serialization by ourselves.

We can still use remoteSerializeTypedParameters; we just have to teach it to skip zero entries like it already can skip string entries. Oh, I just realized that to support string entries, we have to support a flags of VIR_TYPED_PARAM_STRING_OKAY.

+success: + rv = 0; + ret->params.params_len = return_size; + ret->params.params_val = info; + ret->nparams = percpu_len; +cleanup: + if (rv < 0) { + virNetMessageSaveError(rerr); + if (info) {

Cleanup is a lot simpler if we let the existing serialization skip zero entries.

+++ b/src/remote/remote_driver.c @@ -2305,6 +2305,122 @@ done: return rv; }

+static int remoteDomainGetCPUStats(virDomainPtr domain, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + struct private_data *priv = domain->conn->privateData; + remote_domain_get_cpu_stats_args args; + remote_domain_get_cpu_stats_ret ret; + remote_typed_param *info; + int rv = -1; + int cpu, idx, src, dest; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.nparams = nparams; + args.start_cpu = start_cpu; + args.ncpus = ncpus; + args.flags = flags; + + memset(&ret, 0, sizeof(ret));

This side needs to validate incoming parameters before sending over the wire. Hmm, remoteNodeGetCPUStats needs to do likewise.

+ + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_CPU_STATS, + (xdrproc_t) xdr_remote_domain_get_cpu_stats_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_get_cpu_stats_ret, + (char *) &ret) == -1) + goto done; + + if (nparams == 0) { + rv = ret.nparams; + goto cleanup; + } + /* + * the returned arrray's size is not same to nparams * ncpus. And + * if cpu ID is not contiguous, all-zero entries can be found. + */ + memset(params, 0, sizeof(virTypedParameter) * nparams * ncpus);

I prefer sizeof(expr) over sizeof(type), in case expr ever changes types.

+ + /* Here, ret.nparams is always smaller than nparams */ + info = ret.params.params_val; + + for (cpu = 0; cpu < ncpus; ++cpu) { + for (idx = 0; idx < ret.nparams; ++idx) { + src = cpu * ret.nparams + idx; + dest = cpu * nparams + idx; + + if (info[src].value.type == 0) /* skip zeroed ones */ + continue;

Again, open coding things is not nice. If we write the server to never send zero entries (which is a good thing - less data over the wire), then the client needs to re-create zero entries by calling deserialization in a loop - deserialize ret.nparams entries at every nparams slot.

+ + rv = ret.nparams; +cleanup: + if (rv < 0) { + int max = nparams * ncpus; + int i; + + for (i = 0; i < max; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + VIR_FREE(params[i].value.s);

Potential free of bogus memory if anything can reach cleanup with rv < 0 but before we sanitized the contents of params. Here's what I'm squashing in: diff --git i/daemon/remote.c w/daemon/remote.c index 15857a8..cb8423a 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -704,8 +704,11 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, } for (i = 0, j = 0; i < nparams; ++i) { - if (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && - params[i].type == VIR_TYPED_PARAM_STRING) { + /* virDomainGetCPUStats can return a sparse array; also, we + * can't pass back strings to older clients. */ + if (!params[i].type || + (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && + params[i].type == VIR_TYPED_PARAM_STRING)) { --*ret_params_len; continue; } @@ -3534,10 +3537,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, virDomainPtr dom = NULL; struct daemonClientPrivate *priv; virTypedParameterPtr params = NULL; - remote_typed_param *info = NULL; - int cpu, idx, src, dest; int rv = -1; - unsigned int return_size = 0; int percpu_len = 0; priv = virNetServerClientGetPrivateData(client); @@ -3546,128 +3546,53 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - /* - * nparams * ncpus should be smaller than - * REMOTE_DOMAIN_GET_CPU_STATS_MAX but nparams - * and ncpus are limited by API. check it. - */ - if (args->nparams > 16) { + if (args->nparams > REMOTE_NODE_CPU_STATS_MAX) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (args->ncpus > 128) { + if (args->ncpus > REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("ncpus too large")); goto cleanup; } - if (args->nparams > 0) { - if (VIR_ALLOC_N(params, args->ncpus * args->nparams) < 0) - goto no_memory; + if (args->nparams > 0 && + VIR_ALLOC_N(params, args->ncpus * args->nparams) < 0) { + virReportOOMError(); + goto cleanup; } if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; percpu_len = virDomainGetCPUStats(dom, params, args->nparams, - args->start_cpu, args->ncpus, args->flags); + args->start_cpu, args->ncpus, + args->flags); if (percpu_len < 0) goto cleanup; - /* If nparams == 0, the function returns a sigle value */ + /* If nparams == 0, the function returns a single value */ if (args->nparams == 0) goto success; - /* - * Here, we return values based on the real param size returned by - * a driver rather than the passed one. RPC Client stub should decode - * it to fit callar's expectation. - * - * If cpu ID is sparse, The whole array for some cpu IDs will be - * zero-cleared. This doesn't meet to remoteSerializeTypedParameters() - * and we do serialization by ourselves. - */ - return_size = percpu_len * args->ncpus; - if (VIR_ALLOC_N(info, return_size) < 0) - goto no_memory; + if (remoteSerializeTypedParameters(params, args->nparams * args->ncpus, + &ret->params.params_val, + &ret->params.params_len, + args->flags) < 0) + goto cleanup; + + percpu_len = ret->params.params_len / args->ncpus; - for (cpu = 0; cpu < args->ncpus; ++cpu) { - for (idx = 0; idx < percpu_len; ++idx) { - src = cpu * args->nparams + idx; - dest = cpu * percpu_len + idx; - - /* If CPU ID is discontiguous, this can happen */ - if (params[src].type == 0) - continue; - - info[dest].field = strdup(params[src].field); - if (info[dest].field == NULL) - goto no_memory; - - info[dest].value.type = params[src].type; - - switch (params[src].type) { - case VIR_TYPED_PARAM_INT: - info[dest].value.remote_typed_param_value_u.i = - params[src].value.i; - break; - case VIR_TYPED_PARAM_UINT: - info[dest].value.remote_typed_param_value_u.ui = - params[src].value.ui; - break; - case VIR_TYPED_PARAM_LLONG: - info[dest].value.remote_typed_param_value_u.l = - params[src].value.l; - break; - case VIR_TYPED_PARAM_ULLONG: - info[dest].value.remote_typed_param_value_u.ul = - params[src].value.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - info[dest].value.remote_typed_param_value_u.d = - params[src].value.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - info[dest].value.remote_typed_param_value_u.b = - params[src].value.b; - break; - case VIR_TYPED_PARAM_STRING: - info[dest].value.remote_typed_param_value_u.s = - strdup(params[src].value.s); - if (info[dest].value.remote_typed_param_value_u.s == NULL) - goto no_memory; - break; - default: - virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"), - params[src].type); - goto cleanup; - } - } - } success: rv = 0; - ret->params.params_len = return_size; - ret->params.params_val = info; ret->nparams = percpu_len; + cleanup: - if (rv < 0) { + if (rv < 0) virNetMessageSaveError(rerr); - if (info) { - int i; - for (i = 0; i < return_size; i++) { - VIR_FREE(info[i].field); - if (info[i].value.type == VIR_TYPED_PARAM_STRING) - VIR_FREE(info[i].value.remote_typed_param_value_u.s); - } - VIR_FREE(info); - } - } virTypedParameterArrayClear(params, args->ncpus * args->nparams); VIR_FREE(params); if (dom) virDomainFree(dom); return rv; -no_memory: - virReportOOMError(); - goto cleanup; } /*----- Helpers. -----*/ diff --git i/src/libvirt.c w/src/libvirt.c index 7060f5a..e702a34 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18162,7 +18162,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: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virTypedParameterFlags * * Get statistics relating to CPU usage attributable to a single * domain (in contrast to the statistics returned by @@ -18251,20 +18251,19 @@ int virDomainGetCPUStats(virDomainPtr domain, * if start_cpu is -1, ncpus must be 1 * params == NULL must match nparams == 0 * ncpus must be non-zero unless params == NULL + * nparams * ncpus must not overflow (RPC may restrict it even more) */ 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; - } - - /* remote protocol doesn't welcome big args in one shot */ - if ((nparams > 16) || (ncpus > 128)) { + (ncpus == 0 && params != NULL) || + ncpus < UINT_MAX / nparams) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; if (conn->driver->domainGetCPUStats) { int ret; diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index a3ca63e..2312cf9 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -2,7 +2,7 @@ * remote_driver.c: driver to provide access to libvirtd running * on a remote machine * - * Copyright (C) 2007-2011 Red Hat, Inc. + * Copyright (C) 2007-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -2315,12 +2315,24 @@ static int remoteDomainGetCPUStats(virDomainPtr domain, struct private_data *priv = domain->conn->privateData; remote_domain_get_cpu_stats_args args; remote_domain_get_cpu_stats_ret ret; - remote_typed_param *info; int rv = -1; - int cpu, idx, src, dest; + int cpu; remoteDriverLock(priv); + if (nparams > REMOTE_NODE_CPU_STATS_MAX) { + remoteError(VIR_ERR_RPC, + _("nparams count exceeds maximum: %u > %u"), + nparams, REMOTE_NODE_CPU_STATS_MAX); + goto done; + } + if (ncpus > REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX) { + remoteError(VIR_ERR_RPC, + _("ncpus count exceeds maximum: %u > %u"), + ncpus, REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX); + goto done; + } + make_nonnull_domain(&args.dom, domain); args.nparams = nparams; args.start_cpu = start_cpu; @@ -2336,67 +2348,38 @@ static int remoteDomainGetCPUStats(virDomainPtr domain, (char *) &ret) == -1) goto done; + /* Check the length of the returned list carefully. */ + if (ret.params.params_len > nparams * ncpus || + (ret.params.params_len && + ret.nparams * ncpus != ret.params.params_len)) { + remoteError(VIR_ERR_RPC, "%s", + _("remoteDomainGetCPUStats: " + "returned number of stats exceeds limit")); + memset(params, 0, sizeof(*params) * nparams * ncpus); + goto cleanup; + } + + /* Handle the case when the caller does not know the number of stats + * and is asking for the number of stats supported + */ if (nparams == 0) { rv = ret.nparams; goto cleanup; } - /* - * the returned arrray's size is not same to nparams * ncpus. And - * if cpu ID is not contiguous, all-zero entries can be found. + + /* The remote side did not send back any zero entries, so we have + * to expand things back into a possibly sparse array. */ - memset(params, 0, sizeof(virTypedParameter) * nparams * ncpus); - - /* Here, ret.nparams is always smaller than nparams */ - info = ret.params.params_val; - - for (cpu = 0; cpu < ncpus; ++cpu) { - for (idx = 0; idx < ret.nparams; ++idx) { - src = cpu * ret.nparams + idx; - dest = cpu * nparams + idx; - - if (info[src].value.type == 0) /* skip zeroed ones */ - continue; - - params[dest].type = info[src].value.type; - strcpy(params[dest].field, info[src].field); - - switch (params[dest].type) { - case VIR_TYPED_PARAM_INT: - params[dest].value.i = - info[src].value.remote_typed_param_value_u.i; - break; - case VIR_TYPED_PARAM_UINT: - params[dest].value.ui = - info[src].value.remote_typed_param_value_u.ui; - break; - case VIR_TYPED_PARAM_LLONG: - params[dest].value.l = - info[src].value.remote_typed_param_value_u.l; - break; - case VIR_TYPED_PARAM_ULLONG: - params[dest].value.ul = - info[src].value.remote_typed_param_value_u.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - params[dest].value.d = - info[src].value.remote_typed_param_value_u.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - params[dest].value.b = - info[src].value.remote_typed_param_value_u.b; - break; - case VIR_TYPED_PARAM_STRING: - params[dest].value.s = - strdup(info[src].value.remote_typed_param_value_u.s); - if (params[dest].value.s == NULL) - goto out_of_memory; - break; - default: - remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"), - params[dest].type); - goto cleanup; - } - } + memset(params, 0, sizeof(*params) * nparams * ncpus); + for (cpu = 0; cpu < ncpus; cpu++) { + int tmp = nparams; + remote_typed_param *stride = &ret.params.params_val[cpu * ret.nparams]; + + if (remoteDeserializeTypedParameters(stride, ret.nparams, + REMOTE_NODE_CPU_STATS_MAX, + ¶ms[cpu * nparams], + &tmp) < 0) + goto cleanup; } rv = ret.nparams; @@ -2415,9 +2398,6 @@ cleanup: done: remoteDriverUnlock(priv); return rv; -out_of_memory: - virReportOOMError(); - goto cleanup; } diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index 0bd399c..b58925a 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -209,8 +209,13 @@ const REMOTE_DOMAIN_SEND_KEY_MAX = 16; const REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX = 16; /* - * Upper limit on list of (real) cpu parameters - * nparams(<=16) * ncpus(<=128) <= 2048. + * Upper limit on cpus involved in per-cpu stats + */ +const REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX = 128; + +/* + * Upper limit on list of per-cpu stats: + * REMOTE_NODE_CPU_STATS_MAX * REMOTE_DOMAIN_GET_CPU_STATS_MAX */ const REMOTE_DOMAIN_GET_CPU_STATS_MAX = 2048; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

add nodeGetCPUmap() for getting available CPU IDs in a bitmap. add virBitmapParseCommaSeparetedFormat() for parsing bitmap in comma separeted ascii format. This format of bitmap is used in Linux sysfs and cpuset. * cpuacct's percpu usage information is provided based on /sys/devices/system/cpu/present cpu map. So, this kind of call is required. This routine itself may be useful for other purpose. libvirt_private.syms | 2 + nodeinfo.c | 51 ++++++++++++++++++++++++++++++++++++++++ nodeinfo.h | 4 +++ util/bitmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++ util/bitmap.h | 6 +++- 5 files changed, 127 insertions(+), 1 deletion(-) --- src/libvirt_private.syms | 2 + src/nodeinfo.c | 51 ++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 4 +++ src/util/bitmap.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/bitmap.h | 6 +++- 5 files changed, 127 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 915a43f..fd44322 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -17,6 +17,7 @@ virBitmapFree; virBitmapGetBit; virBitmapSetBit; virBitmapString; +virBitmapParseCommaSeparatedFormat; # buf.h @@ -800,6 +801,7 @@ nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; nodeGetMemoryStats; +nodeGetCPUmap; # nwfilter_conf.h diff --git a/src/nodeinfo.c b/src/nodeinfo.c index e0b66f7..92bada7 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -47,6 +47,7 @@ #include "count-one-bits.h" #include "intprops.h" #include "virfile.h" +#include "bitmap.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -569,6 +570,33 @@ int linuxNodeGetMemoryStats(FILE *meminfo, cleanup: return ret; } + +/* + * Linux maintains cpu bit map. For example, if cpuid=5's flag is not set + * and max cpu is 7. The map file shows 0-4,6-7. This functin parse + * it and returns bitmap. + */ +static virBitmapPtr linuxParseCPUmap(int *max_cpuid, const char *path) +{ + char str[1024]; + FILE *fp; + virBitmapPtr map = NULL; + + fp = fopen(path, "r"); + if (!fp) { + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + if (fgets(str, sizeof(str), fp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + map = virBitmapParseCommaSeparatedFormat(str, max_cpuid); + +cleanup: + VIR_FORCE_FCLOSE(fp); + return map; +} #endif int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -712,6 +740,29 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif } +virBitmapPtr nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED, + int *max_id, + const char *mapname) +{ +#ifdef __linux__ + char *path; + virBitmapPtr map; + + if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) { + virReportOOMError(); + return NULL; + } + + map = linuxParseCPUmap(max_id, path); + VIR_FREE(path); + return map; +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node cpumap not implemented on this platform")); + return -1; +#endif +} + #if HAVE_NUMACTL # if LIBNUMA_API_VERSION <= 1 # define NUMA_MAX_N_CPUS 4096 diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 4766152..7f26b77 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -26,6 +26,7 @@ # include "libvirt/libvirt.h" # include "capabilities.h" +# include "bitmap.h" int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); int nodeCapsInitNUMA(virCapsPtr caps); @@ -46,4 +47,7 @@ int nodeGetCellsFreeMemory(virConnectPtr conn, int maxCells); unsigned long long nodeGetFreeMemory(virConnectPtr conn); +virBitmapPtr nodeGetCPUmap(virConnectPtr conn, + int *max_id, + const char *mapname); #endif /* __VIR_NODEINFO_H__*/ diff --git a/src/util/bitmap.c b/src/util/bitmap.c index 8c326c4..c7e1a81 100644 --- a/src/util/bitmap.c +++ b/src/util/bitmap.c @@ -33,6 +33,11 @@ #include "bitmap.h" #include "memory.h" #include "buf.h" +#include "c-ctype.h" +#include "util.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE struct _virBitmap { @@ -180,3 +185,63 @@ char *virBitmapString(virBitmapPtr bitmap) return virBufferContentAndReset(&buf); } + +/** + * virBitmapParseCommaSeparatedFormat: + * + * When bitmap is printed in ascii format, expecially in Linux, + * comma-separated format is sometimes used. For example, a bitmap 11111011 is + * represetned as 0-4,6-7. This function parse comma-separated format + * and returns virBitmap. Found max bit is returend, too. + * + * This functions stops if characters other than digits, ',', '-' are + * found. This function will be useful for parsing linux's bitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) +{ + char *pos = buf; + virBitmapPtr map = NULL; + int val, x; + + /* at first, find the highest number */ + val = 0; + while ((*pos != '\n') || (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + } else if ((*pos == ',') || (*pos == '-')) { + ++pos; + } else + break; + } + *max_bit = val; + + map = virBitmapAlloc(val + 1); + if (map == NULL) { /* we may return NULL at failure of parsing */ + virReportOOMError(); + return NULL; + } + + pos = buf; + while ((*pos != '\n') || (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + if (virBitmapSetBit(map, val) < 0) + goto failed; + } else if (*pos == ',') { + ++pos; + } else if (*pos == '-') { + x = val; + pos++; + virStrToLong_i(pos, &pos, 10, &val); + for (;x <= val; ++x) { + if (virBitmapSetBit(map, x) < 0) + goto failed; + } + } else + break; + } + return map; +failed: + virBitmapFree(map); + return NULL; +} diff --git a/src/util/bitmap.h b/src/util/bitmap.h index ef62cca..e3d3e2b 100644 --- a/src/util/bitmap.h +++ b/src/util/bitmap.h @@ -61,5 +61,9 @@ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) char *virBitmapString(virBitmapPtr bitmap) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; - +/* + * parese comma-separeted bitmap format and allocate virBitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) + ATTRIBUTE_RETURN_CHECK; #endif -- 1.7.4.1

On Tue, 31 Jan 2012 17:58:17 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:

...

+ +/** + * virBitmapParseCommaSeparatedFormat: + * + * When bitmap is printed in ascii format, expecially in Linux,

s/expecially/especially/

...

+ * comma-separated format is sometimes used. For example, a bitmap 11111011 is + * represetned as 0-4,6-7. This function parse comma-separated format

s/parse/parses/

...

+ * and returns virBitmap. Found max bit is returend, too.

s/returend/returned/

will fix.

...

+ * + * This functions stops if characters other than digits, ',', '-' are + * found. This function will be useful for parsing linux's bitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) +{ + char *pos = buf; + virBitmapPtr map = NULL; + int val, x; + + /* at first, find the highest number */ + val = 0; + while ((*pos != '\n') || (*pos != 0)) {

(*pos != '\n') && (*pos != 0)

Ah, yes. Fixed one here. ==

From 6267799ae373aa259b7da6350c2bf8056eee4ed3 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 27 Jan 2012 19:38:24 +0900 Subject: [PATCH] add nodeGetCPUmap() for getting available CPU IDs in a bitmap. add virBitmapParseCommaSeparetedFormat() for parsing bitmap in comma separeted ascii format. This format of bitmap is used in Linux sysfs and cpuset.

Changelog: - fixed typos. - fixed string scan routine. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> libvirt_private.syms | 2 + nodeinfo.c | 51 ++++++++++++++++++++++++++++++++++++++++ nodeinfo.h | 4 +++ util/bitmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++ util/bitmap.h | 6 +++- 5 files changed, 127 insertions(+), 1 deletion(-) --- src/libvirt_private.syms | 2 + src/nodeinfo.c | 51 ++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 4 +++ src/util/bitmap.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/bitmap.h | 6 +++- 5 files changed, 127 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e300f06..f6fc5d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -17,6 +17,7 @@ virBitmapFree; virBitmapGetBit; virBitmapSetBit; virBitmapString; +virBitmapParseCommaSeparatedFormat; # buf.h @@ -801,6 +802,7 @@ nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; nodeGetMemoryStats; +nodeGetCPUmap; # nwfilter_conf.h diff --git a/src/nodeinfo.c b/src/nodeinfo.c index e0b66f7..b0ebb88 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -47,6 +47,7 @@ #include "count-one-bits.h" #include "intprops.h" #include "virfile.h" +#include "bitmap.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -569,6 +570,33 @@ int linuxNodeGetMemoryStats(FILE *meminfo, cleanup: return ret; } + +/* + * Linux maintains cpu bit map. For example, if cpuid=5's flag is not set + * and max cpu is 7. The map file shows 0-4,6-7. This functin parses + * it and returns bitmap. + */ +static virBitmapPtr linuxParseCPUmap(int *max_cpuid, const char *path) +{ + char str[1024]; + FILE *fp; + virBitmapPtr map = NULL; + + fp = fopen(path, "r"); + if (!fp) { + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + if (fgets(str, sizeof(str), fp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + map = virBitmapParseCommaSeparatedFormat(str, max_cpuid); + +cleanup: + VIR_FORCE_FCLOSE(fp); + return map; +} #endif int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -712,6 +740,29 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, #endif } +virBitmapPtr nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED, + int *max_id, + const char *mapname) +{ +#ifdef __linux__ + char *path; + virBitmapPtr map; + + if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) { + virReportOOMError(); + return NULL; + } + + map = linuxParseCPUmap(max_id, path); + VIR_FREE(path); + return map; +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node cpumap not implemented on this platform")); + return -1; +#endif +} + #if HAVE_NUMACTL # if LIBNUMA_API_VERSION <= 1 # define NUMA_MAX_N_CPUS 4096 diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 4766152..7f26b77 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -26,6 +26,7 @@ # include "libvirt/libvirt.h" # include "capabilities.h" +# include "bitmap.h" int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); int nodeCapsInitNUMA(virCapsPtr caps); @@ -46,4 +47,7 @@ int nodeGetCellsFreeMemory(virConnectPtr conn, int maxCells); unsigned long long nodeGetFreeMemory(virConnectPtr conn); +virBitmapPtr nodeGetCPUmap(virConnectPtr conn, + int *max_id, + const char *mapname); #endif /* __VIR_NODEINFO_H__*/ diff --git a/src/util/bitmap.c b/src/util/bitmap.c index 8c326c4..6dead3b 100644 --- a/src/util/bitmap.c +++ b/src/util/bitmap.c @@ -33,6 +33,11 @@ #include "bitmap.h" #include "memory.h" #include "buf.h" +#include "c-ctype.h" +#include "util.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE struct _virBitmap { @@ -180,3 +185,63 @@ char *virBitmapString(virBitmapPtr bitmap) return virBufferContentAndReset(&buf); } + +/** + * virBitmapParseCommaSeparatedFormat: + * + * When bitmap is printed in ascii format, especially in Linux, + * comma-separated format is sometimes used. For example, a bitmap 11111011 is + * represetned as 0-4,6-7. This function parses comma-separated format + * and returns virBitmap. Found max bit is returned, too. + * + * This functions stops if characters other than digits, ',', '-' are + * found. This function will be useful for parsing linux's bitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) +{ + char *pos = buf; + virBitmapPtr map = NULL; + int val, x; + + /* at first, find the highest number */ + val = 0; + while ((*pos != '\n') && (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + } else if ((*pos == ',') || (*pos == '-')) { + ++pos; + } else + break; + } + *max_bit = val; + + map = virBitmapAlloc(val + 1); + if (map == NULL) { /* we may return NULL at failure of parsing */ + virReportOOMError(); + return NULL; + } + + pos = buf; + while ((*pos != '\n') && (*pos != 0)) { + if (c_isdigit(*pos)) { + virStrToLong_i(pos, &pos, 10, &val); + if (virBitmapSetBit(map, val) < 0) + goto failed; + } else if (*pos == ',') { + ++pos; + } else if (*pos == '-') { + x = val; + pos++; + virStrToLong_i(pos, &pos, 10, &val); + for (;x <= val; ++x) { + if (virBitmapSetBit(map, x) < 0) + goto failed; + } + } else + break; + } + return map; +failed: + virBitmapFree(map); + return NULL; +} diff --git a/src/util/bitmap.h b/src/util/bitmap.h index ef62cca..e3d3e2b 100644 --- a/src/util/bitmap.h +++ b/src/util/bitmap.h @@ -61,5 +61,9 @@ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) char *virBitmapString(virBitmapPtr bitmap) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; - +/* + * parese comma-separeted bitmap format and allocate virBitmap. + */ +virBitmapPtr virBitmapParseCommaSeparatedFormat(char *buf, int *max_bit) + ATTRIBUTE_RETURN_CHECK; #endif -- 1.7.4.1

cpu-accts command shows cpu accounting information of a domain. [root@bluextal ~]# virsh cpu-accts GuestX1 Total: cpu_time 165.4 CPU0: cpu_time 0.0 CPU1: cpu_time 0.0 CPU2: cpu_time 0.0 CPU3: cpu_time 0.0 CPU4: cpu_time 78.9 CPU5: cpu_time 0.0 CPU6: cpu_time 0.0 CPU7: cpu_time 86.5 This guest using 165secs of cpu in total. CPU4/7 is used mainly. --- tools/virsh.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++ 2 files changed, 121 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 74655c2..9746500 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5106,6 +5106,121 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) } /* + * "cputime" command + */ +static const vshCmdInfo info_cpu_accts[] = { + {"help", N_("show domain cpu accounting information")}, + {"desc", N_("Returns information about the domain's cpu accounting")}, + {NULL, NULL}, +}; + +static const vshCmdOptDef opts_cpu_accts[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {NULL, 0, 0, NULL}, +}; + +static bool +cmdCPUAccts(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + virTypedParameterPtr params = NULL; + bool ret = true; + int i, j, pos, cpu, nparams; + int max_id; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + /* get max cpu id on the node */ + max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, 0); + if (max_id < 0) { + vshPrint(ctl, "max id %d\n", max_id); + goto cleanup; + } + /* get supported num of parameter for total statistics */ + nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0); + if (nparams < 0) + goto cleanup; + + if (VIR_ALLOC_N(params, nparams)) { + virReportOOMError(); + goto cleanup; + } + /* passing start_cpu == -1 gives us domain's total status */ + nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, 0); + if (nparams < 0) + goto cleanup; + + vshPrint(ctl, "Total:\n"); + for (i = 0; i < nparams; i++) { + vshPrint(ctl, "\t%-15s ", params[i].field); + switch (params[i].type) { + case VIR_TYPED_PARAM_ULLONG: + /* Now all UULONG param is used for time accounting in ns */ + vshPrint(ctl, "%.1lf\n", + params[i].value.ul / 1000000000.0); + break; + default: + vshError(ctl, "%s", _("API mismatch?")); + goto cleanup; + } + } + VIR_FREE(params); + /* get percpu information */ + nparams = virDomainGetCPUStats(dom, NULL, 0, -1, 1, 0); + if (nparams < 0) + goto cleanup; + + cpu = 0; + while (cpu <= max_id) { + int ncpus = 128; + + if (cpu + ncpus - 1 > max_id) /* id starts from 0. */ + ncpus = max_id + 1 - cpu; + + if (VIR_ALLOC_N(params, nparams * ncpus)) { + virReportOOMError(); + goto cleanup; + } + + if (virDomainGetCPUStats(dom, params, nparams, cpu, ncpus, 0) < 0) + goto cleanup; + + for (i = 0; i < ncpus; i++) { + if (params[i * nparams].type == 0) + continue; + vshPrint(ctl, "CPU%d:\n", cpu + i); + + for (j = 0; j < nparams; j++) { + pos = i * nparams + j; + if (params[pos].type == 0) + continue; + vshPrint(ctl, "\t%-15s ", params[pos].field); + switch(params[j].type) { + case VIR_TYPED_PARAM_ULLONG: + vshPrint(ctl, "%.1lf\n", + params[pos].value.ul / 1000000000.0); + break; + default: + vshError(ctl, "%s", _("API mismatch?")); + goto cleanup; + } + } + } + cpu += ncpus; + /* Note: If we handle string type, it should be freed */ + VIR_FREE(params); + } +cleanup: + VIR_FREE(params); + virDomainFree(dom); + return ret; +} + +/* * "inject-nmi" command */ static const vshCmdInfo info_inject_nmi[] = { @@ -16014,6 +16129,7 @@ static const vshCmdDef domManagementCmds[] = { #endif {"cpu-baseline", cmdCPUBaseline, opts_cpu_baseline, info_cpu_baseline, 0}, {"cpu-compare", cmdCPUCompare, opts_cpu_compare, info_cpu_compare, 0}, + {"cpu-accts", cmdCPUAccts, opts_cpu_accts, info_cpu_accts, 0}, {"create", cmdCreate, opts_create, info_create, 0}, {"define", cmdDefine, opts_define, info_define, 0}, {"destroy", cmdDestroy, opts_destroy, info_destroy, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 8599f66..df65935 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -721,6 +721,11 @@ Provide the maximum number of virtual CPUs supported for a guest VM on this connection. If provided, the I<type> parameter must be a valid type attribute for the <domain> element of XML. +=item B<cpu-accts> I<domain> + +Provide cpu accounting information of a domain. The domain should +be running. + =item B<migrate> [I<--live>] [I<--direct>] [I<--p2p> [I<--tunnelled>]] [I<--persistent>] [I<--undefinesource>] [I<--suspend>] [I<--copy-storage-all>] [I<--copy-storage-inc>] [I<--change-protection>] [I<--verbose>] -- 1.7.4.1