On Tue, Jun 07, 2011 at 09:58:47AM +0900, Minoru Usui wrote:

virNodeGetCPUStats: Expose new API

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- include/libvirt/libvirt.h.in | 75 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 5 +++ 2 files changed, 79 insertions(+), 1 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 8058229..43e59a7 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -194,7 +194,6 @@ typedef struct _virStream virStream; */ typedef virStream *virStreamPtr;

- /** * VIR_SECURITY_LABEL_BUFLEN: * @@ -280,6 +279,66 @@ struct _virNodeInfo { unsigned int threads;/* number of threads per core */ };

+/** + * VIR_CPU_STATS_FIELD_LENGTH: + * + * Macro providing the field length of virNodeCPUStats + */ +#define VIR_CPU_STATS_FIELD_LENGTH 80 + +/** + * VIR_CPU_STATS_ALL_CPUS: + * + * Macro for the total CPU time/utilization + */ +#define VIR_CPU_STATS_ALL_CPUS -1 + +/** + * VIR_CPU_STATS_KERNEL: + * + * Macro for the cumulative CPU time which spends by kernel, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_STATS_KERNEL "kernel" + +/** + * The cumulative CPU time which spends by user processes, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_STATS_USER "user" + +/** + * The cumulative idle CPU time, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_STATS_IDLE "idle" + +/** + * The cumulative I/O wait CPU time, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_STATS_IOWAIT "iowait" + +/** + * The CPU utilization. + * The usage value is in percent and 100% represents all CPUs on + * the server. + */ +#define VIR_CPU_STATS_UTILIZATION "utilization" + +/** + * virCPUStats: + * + * a virNodeCPUStats is a structure filled by virNodeGetCPUStats() + * and providing the information for the cpu stats of the node. + */ +typedef struct _virCPUStats virCPUStats; + +struct _virCPUStats { + char field[VIR_CPU_STATS_FIELD_LENGTH]; + unsigned long long value; +}; +

/* Common data types shared among interfaces with name/type/value lists. */

@@ -544,6 +603,14 @@ int virDomainMigrateSetMaxSpeed(virDomainPtr domain, typedef virNodeInfo *virNodeInfoPtr;

/** + * virCPUStatsPtr: + * + * a virCPUStatsPtr is a pointer to a virCPUStats structure. + */ + +typedef virCPUStats *virCPUStatsPtr; + +/** * virConnectFlags * * Flags when opening a connection to a hypervisor @@ -677,6 +744,12 @@ int virNodeGetInfo (virConnectPtr conn, virNodeInfoPtr info); char * virConnectGetCapabilities (virConnectPtr conn);

+int virNodeGetCPUStats (virConnectPtr conn, + int cpuNum, + virCPUStatsPtr params, + int *nparams, + unsigned int flags); + unsigned long long virNodeGetFreeMemory (virConnectPtr conn);

int virNodeGetSecurityModel (virConnectPtr conn, diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 4d4299a..271b8e3 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -450,4 +450,9 @@ LIBVIRT_0.9.2 { virInterfaceChangeRollback; } LIBVIRT_0.9.0;

+LIBVIRT_0.9.3 { + global: + virNodeGetCPUStats; +} LIBVIRT_0.9.2; + # .... define new API here using predicted next version number ....

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/14/2011 03:04 AM, Daniel P. Berrange wrote:

On Tue, Jun 07, 2011 at 10:00:23AM +0900, Minoru Usui wrote:

...

virNodeGetCPUTime: Implement public API

...

+ * @cpuNum: number of node cpu. (VIR_CPU_STATS_ALL_CPUS means total cpu + * statistics)

Trailing whitespace; tsk-tsk. 'make syntax-check' caught it.

...

+ * Here is the sample code snippet: + * + * if ((virNodeGetCPUStats(conn, cpuNum, NULL, &nparams, 0) == 0) && + * (nparams != 0)) { + * params = vshMalloc(ctl, sizeof(virCPUStats) * nparams); + * memset(params, 0, sizeof(virCPUStats) * nparams); + * if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, 0)) { + * vshError(ctl, "%s", _("Unable to get node cpu stats")); + * goto error; + * } + * }

The example code should stick to using standard libc APIs for demonstration purposes, because applications developing against libvirt don't have use of vshMalloc or vshError. Just use normal malloc() and fprintf(stderr) here instead.

Copy and paste from a previous example, so I've fixed that, too.

...

+ * + * This function doesn't requires privileged access to the hypervisor.

s/requires/require/

ACK if the comment is changed.

Pushed with this squashed in (along with 2/12 unchanged): diff --git i/src/libvirt.c w/src/libvirt.c index ccb00cd..ace0120 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -3008,16 +3008,15 @@ error: * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API * again. * - * Here is the sample code snippet: + * Here is a sample code snippet: * * if ((virDomainGetMemoryParameters(dom, NULL, &nparams, 0) == 0) && * (nparams != 0)) { - * params = vshMalloc(ctl, sizeof(*params) * nparams); + * if ((params = malloc(sizeof(*params) * nparams)) == NULL) + * goto error; * memset(params, 0, sizeof(*params) * nparams); - * if (virDomainGetMemoryParameters(dom, params, &nparams, 0)) { - * vshError(ctl, "%s", _("Unable to get memory parameters")); + * if (virDomainGetMemoryParameters(dom, params, &nparams, 0)) * goto error; - * } * } * * This function requires privileged access to the hypervisor. This function @@ -5288,7 +5287,7 @@ error: /** * virNodeGetCPUStats: * @conn: pointer to the hypervisor connection. - * @cpuNum: number of node cpu. (VIR_CPU_STATS_ALL_CPUS means total cpu + * @cpuNum: number of node cpu. (VIR_CPU_STATS_ALL_CPUS means total cpu * statistics) * @params: pointer to node cpu time parameter objects * @nparams: number of node cpu time parameter (this value should be same or @@ -5307,19 +5306,18 @@ error: * array, i.e. (sizeof(@virCPUStats) * @nparams) bytes and call * the API again. * - * Here is the sample code snippet: + * Here is a sample code snippet: * * if ((virNodeGetCPUStats(conn, cpuNum, NULL, &nparams, 0) == 0) && * (nparams != 0)) { - * params = vshMalloc(ctl, sizeof(virCPUStats) * nparams); + * if ((params = malloc(sizeof(virCPUStats) * nparams)) == NULL) + * goto error; * memset(params, 0, sizeof(virCPUStats) * nparams); - * if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, 0)) { - * vshError(ctl, "%s", _("Unable to get node cpu stats")); + * if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, 0)) * goto error; - * } * } * - * This function doesn't requires privileged access to the hypervisor. + * This function doesn't require privileged access to the hypervisor. * This function expects the caller to allocate the @params. * * CPU time Statistics: -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:01:12AM +0900, Minoru Usui wrote:

virNodeGetCPUStats: Implement remote protocol

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- daemon/remote.c | 77 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 67 ++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 22 +++++++++++- 3 files changed, 165 insertions(+), 1 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 2a32ee8..5b947af 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1473,6 +1473,83 @@ cleanup: return rv; }

+static int +remoteDispatchNodeGetCPUStats (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_node_get_cpu_stats_args *args, + remote_node_get_cpu_stats_ret *ret) +{ + virCPUStatsPtr params = NULL; + int i; + int cpuNum = args->cpuNum; + int nparams = args->nparams; + unsigned int flags; + int rv = -1; + + if (!conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + flags = args->flags; + + if (nparams > REMOTE_NODE_CPU_STATS_MAX) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + if (VIR_ALLOC_N(params, nparams) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virNodeGetCPUStats(conn, cpuNum, params, &nparams, flags) < 0) + goto cleanup; + + /* In this case, we need to send back the number of stats + * supported + */ + if (args->nparams == 0) { + ret->nparams = nparams; + goto success; + } + + /* Serialise the memory parameters. */ + ret->params.params_len = nparams; + if (VIR_ALLOC_N(ret->params.params_val, nparams) < 0) + goto no_memory; + + for (i = 0; i < nparams; ++i) { + /* remoteDispatchClientRequest will free this: */ + ret->params.params_val[i].field = strdup(params[i].field); + if (ret->params.params_val[i].field == NULL) + goto no_memory; + + ret->params.params_val[i].value = params[i].value; + } + +success: + rv = 0; + +cleanup: + if (rv < 0) { + remoteDispatchError(rerr); + if (ret->params.params_val) { + for (i = 0; i < nparams; i++) + VIR_FREE(ret->params.params_val[i].field); + VIR_FREE(ret->params.params_val); + } + } + VIR_FREE(params); + return rv; + +no_memory: + virReportOOMError(); + goto cleanup; +} + /*-------------------------------------------------------------*/

static int diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 14c3d24..c57c407 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1697,6 +1697,72 @@ done: }

static int +remoteNodeGetCPUStats (virConnectPtr conn, + int cpuNum, + virCPUStatsPtr params, int *nparams, + unsigned int flags) +{ + int rv = -1; + remote_node_get_cpu_stats_args args; + remote_node_get_cpu_stats_ret ret; + int i = -1; + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + args.nparams = *nparams; + args.cpuNum = cpuNum; + args.flags = flags; + + memset (&ret, 0, sizeof ret); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_STATS, + (xdrproc_t) xdr_remote_node_get_cpu_stats_args, + (char *) &args, + (xdrproc_t) xdr_remote_node_get_cpu_stats_ret, + (char *) &ret) == -1) + goto done; + + /* Check the length of the returned list carefully. */ + if (ret.params.params_len > REMOTE_NODE_CPU_STATS_MAX || + ret.params.params_len > *nparams) { + remoteError(VIR_ERR_RPC, "%s", + _("remoteNodeGetCPUStats: " + "returned number of stats exceeds limit")); + 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) { + *nparams = ret.nparams; + rv = 0; + goto cleanup; + } + + *nparams = ret.params.params_len; + + /* Deserialise the result. */ + for (i = 0; i < *nparams; ++i) { + if (virStrcpyStatic(params[i].field, ret.params.params_val[i].field) == NULL) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("Stats %s too big for destination"), + ret.params.params_val[i].field); + goto cleanup; + } + params[i].value = ret.params.params_val[i].value; + } + + rv = 0; + +cleanup: + xdr_free ((xdrproc_t) xdr_remote_node_get_cpu_stats_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, int startCell, @@ -6295,6 +6361,7 @@ static virDriver remote_driver = { .domainBlockPeek = remoteDomainBlockPeek, /* 0.4.2 */ .domainMemoryPeek = remoteDomainMemoryPeek, /* 0.4.2 */ .domainGetBlockInfo = remoteDomainGetBlockInfo, /* 0.8.1 */ + .nodeGetCPUStats = remoteNodeGetCPUStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = remoteNodeGetCellsFreeMemory, /* 0.3.3 */ .nodeGetFreeMemory = remoteNodeGetFreeMemory, /* 0.3.3 */ .domainEventRegister = remoteDomainEventRegister, /* 0.5.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index c9b8cff..59a9d62 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -134,6 +134,9 @@ const REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX = 16; /* Upper limit on list of memory parameters. */ const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16;

+/* Upper limit on list of node cpu stats. */ +const REMOTE_NODE_CPU_STATS_MAX = 16; + /* Upper limit on number of NUMA cells */ const REMOTE_NODE_MAX_CELLS = 1024;

@@ -319,6 +322,11 @@ struct remote_typed_param { remote_typed_param_value value; };

+struct remote_node_get_cpu_stats { + remote_nonnull_string field; + unsigned hyper value; +}; + /*----- Calls. -----*/

/* For each call we may have a 'remote_CALL_args' and 'remote_CALL_ret' @@ -403,6 +411,17 @@ struct remote_get_capabilities_ret { remote_nonnull_string capabilities; };

+struct remote_node_get_cpu_stats_args { + int cpuNum; + int nparams; + unsigned int flags; +}; + +struct remote_node_get_cpu_stats_ret { + remote_node_get_cpu_stats params<REMOTE_NODE_CPU_STATS_MAX>; + int nparams; +}; + struct remote_node_get_cells_free_memory_args { int startCell; int maxcells; @@ -2297,7 +2316,8 @@ enum remote_procedure { REMOTE_PROC_INTERFACE_CHANGE_COMMIT = 221, /* autogen autogen */ REMOTE_PROC_INTERFACE_CHANGE_ROLLBACK = 222, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_SCHEDULER_PARAMETERS_FLAGS = 223, /* skipgen autogen */ - REMOTE_PROC_DOMAIN_EVENT_CONTROL_ERROR = 224 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_EVENT_CONTROL_ERROR = 224, /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_CPU_STATS = 225 /* skipgen skipgen */

/* * Notice how the entries are grouped in sets of 10 ?

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/14/2011 03:06 AM, Daniel P. Berrange wrote:

On Tue, Jun 07, 2011 at 10:02:13AM +0900, Minoru Usui wrote:

...

virNodeGetCPUStats: Implement virsh support

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- tools/virsh.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 7 +++ 2 files changed, 141 insertions(+), 0 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 5679a2d..2fbc91a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3682,6 +3682,139 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) }

/* + * "nodecpustats" command + */ +static const vshCmdInfo info_nodecpustats[] = { + {"help", N_("Prints cpu stats of the node.")}, + {"desc", N_("Returns cpu stats of the node.(nsec)")},

Not sure that the (nsec) string aids understanding here, so I nuked it.

...

+ struct cpu_stats { + unsigned long long user; + unsigned long long sys; + unsigned long long idle; + unsigned long long iowait; + unsigned long long util; + } cpu_stats[2];

...

+ + memset(cpu_stats, 0, sizeof(struct cpu_stats) * 2);

2 is a magic number here, easily avoided.

...

+ + if (strcmp(params[j].field, VIR_CPU_STATS_KERNEL) == 0) + cpu_stats[i].sys = value;

'make syntax-check' called you on this; use STREQ instead.

...

+ + if (strcmp(params[j].field, VIR_CPU_STATS_USER) == 0) + cpu_stats[i].user = value;

For a (minor) efficiency boost, we can use 'else if' instead of 'if'.

...

+ + if (flag_utilization || (flag_percent == false)) + break; + + i++; + sleep(1); + } while(i < 2); + + if (flag_percent == false) {

Comparison against bool is discouraged in HACKING.

...

@@ -11293,6 +11426,7 @@ static const vshCmdDef hostAndHypervisorCmds[] = { {"freecell", cmdFreecell, opts_freecell, info_freecell, 0}, {"hostname", cmdHostname, NULL, info_hostname, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, + {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats},

Sort these lines, and for consistency, keep the trailing 0.

...

{"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command, 0}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 6ca3002..ade4eab 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -237,6 +237,13 @@ Print the XML representation of the hypervisor sysinfo, if available. Returns basic information about the node, like number and type of CPU, and size of the physical memory.

+=item B<nodecpustats> optional I<--cpu> I<--percent>

The usage is [--cpu] <cpu>; I've tweaked this slightly.

...

+ +Returns cpu stats of the node. +If I<--cpu> is specified, this will prints specified cpu statistics only. +If I<--percent> is specified, this will prints percentage of each kind of cpu +statistics during 1 second. + =item B<capabilities>

Print an XML document describing the capabilities of the hypervisor

ACK

Here's what I squashed in before pushing: diff --git i/tools/virsh.c w/tools/virsh.c index 6071aaa..1e2385c 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -3718,7 +3718,7 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) */ static const vshCmdInfo info_nodecpustats[] = { {"help", N_("Prints cpu stats of the node.")}, - {"desc", N_("Returns cpu stats of the node.(nsec)")}, + {"desc", N_("Returns cpu stats of the node.")}, {NULL, NULL} }; @@ -3766,7 +3766,7 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; } - memset(cpu_stats, 0, sizeof(struct cpu_stats) * 2); + memset(cpu_stats, 0, sizeof(cpu_stats)); params = vshCalloc(ctl, nparams, sizeof(*params)); i = 0; @@ -3779,33 +3779,29 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) for (j = 0; j < nparams; j++) { unsigned long long value = params[j].value; - if (strcmp(params[j].field, VIR_CPU_STATS_KERNEL) == 0) + if (STREQ(params[j].field, VIR_CPU_STATS_KERNEL)) { cpu_stats[i].sys = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_USER) == 0) + } else if (STREQ(params[j].field, VIR_CPU_STATS_USER)) { cpu_stats[i].user = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_IDLE) == 0) + } else if (STREQ(params[j].field, VIR_CPU_STATS_IDLE)) { cpu_stats[i].idle = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_IOWAIT) == 0) + } else if (STREQ(params[j].field, VIR_CPU_STATS_IOWAIT)) { cpu_stats[i].iowait = value; - - if (strcmp(params[j].field, VIR_CPU_STATS_UTILIZATION) == 0) { + } else if (STREQ(params[j].field, VIR_CPU_STATS_UTILIZATION)) { cpu_stats[i].util = value; flag_utilization = true; } } - if (flag_utilization || (flag_percent == false)) + if (flag_utilization || !flag_percent) break; i++; sleep(1); } while(i < 2); - if (flag_percent == false) { - if (flag_utilization == false) { + if (!flag_percent) { + if (!flag_utilization) { vshPrint(ctl, "%-15s %20llu\n", _("user :"), cpu_stats[0].user); vshPrint(ctl, "%-15s %20llu\n", _("system:"), cpu_stats[0].sys); vshPrint(ctl, "%-15s %20llu\n", _("idle :"), cpu_stats[0].idle); @@ -11526,8 +11522,8 @@ static const vshCmdDef hostAndHypervisorCmds[] = { VSH_CMD_FLAG_NOCONNECT}, {"freecell", cmdFreecell, opts_freecell, info_freecell, 0}, {"hostname", cmdHostname, NULL, info_hostname, 0}, + {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, - {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats}, {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command, 0}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0}, diff --git i/tools/virsh.pod w/tools/virsh.pod index b2dbbb9..b4c580a 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -239,10 +239,10 @@ and size of the physical memory. The output corresponds to virNodeInfo structure. Specifically, the "CPU socket(s)" field means number of CPU sockets per NUMA cell. -=item B<nodecpustats> optional I<--cpu> I<--percent> +=item B<nodecpustats> optional I<cpu> I<--percent> Returns cpu stats of the node. -If I<--cpu> is specified, this will prints specified cpu statistics only. +If I<cpu> is specified, this will prints specified cpu statistics only. If I<--percent> is specified, this will prints percentage of each kind of cpu statistics during 1 second. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:02:55AM +0900, Minoru Usui wrote:

virNodeGetCPUStats: Implement linux support

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 1 + src/nodeinfo.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 6 ++- src/qemu/qemu_driver.c | 1 + src/uml/uml_driver.c | 1 + 6 files changed, 147 insertions(+), 1 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e6ab870..a8c77f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -731,6 +731,7 @@ virNodeDeviceObjUnlock;

# nodeinfo.h nodeCapsInitNUMA; +nodeGetCPUStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8eb87a2..3286154 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2779,6 +2779,7 @@ static virDriver lxcDriver = { .domainSetSchedulerParameters = lxcSetSchedulerParameters, /* 0.5.0 */ .domainSetSchedulerParametersFlags = lxcSetSchedulerParametersFlags, /* 0.9.2 */ .domainInterfaceStats = lxcDomainInterfaceStats, /* 0.7.3 */ + .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.6.5 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.6.5 */ .domainEventRegister = lxcDomainEventRegister, /* 0.7.0 */ diff --git a/src/nodeinfo.c b/src/nodeinfo.c index f55c83e..39afd0e 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -57,12 +57,20 @@ #ifdef __linux__ # define CPUINFO_PATH "/proc/cpuinfo" # define CPU_SYS_PATH "/sys/devices/system/cpu" +# define PROCSTAT_PATH "/proc/stat" + +# define LINUX_NB_CPU_STATS 4

/* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, virNodeInfoPtr nodeinfo, bool need_hyperthreads);

+int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virCPUStatsPtr params, + int *nparams); + /* Return the positive decimal contents of the given * CPU_SYS_PATH/cpu%u/FILE, or -1 on error. If MISSING_OK and the * file could not be found, return 1 instead of an error; this is @@ -378,6 +386,108 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, return 0; }

+#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) +#define CPU_HEADER_LEN 8 + +int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virCPUStatsPtr params, + int *nparams) +{ + int ret = -1; + char line[1024]; + unsigned long long usr, ni, sys, idle, iowait; + unsigned long long irq, softirq, steal, guest, guest_nice; + char cpu_header[CPU_HEADER_LEN]; + + if ((*nparams) == 0) { + /* Current number of cpu stats supported by linux */ + *nparams = LINUX_NB_CPU_STATS; + ret = 0; + goto cleanup; + } + + if ((*nparams) != LINUX_NB_CPU_STATS) { + nodeReportError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto cleanup; + } + + if (cpuNum == VIR_CPU_STATS_ALL_CPUS) { + strcpy(cpu_header, "cpu"); + } else { + sprintf(cpu_header, "cpu%d", cpuNum); + }

cpu_header is declared to be 8 bytes in size, which only allows for integers upto 4 digits long, before we get a buffer overflow here. gnulib has some macro which can be used to declare a string buffer large enough to hold an integer, but I can't remember what the macro is called right now. Hopefully Eric will remind us shortly....

+ + while (fgets(line, sizeof(line), procstat) != NULL) { + char *buf = line; + + if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ + int i; + + if (sscanf(buf, + "%*s %llu %llu %llu %llu %llu" // user ~ iowait + "%llu %llu %llu %llu %llu", // irq ~ guest_nice + &usr, &ni, &sys, &idle, &iowait, + &irq, &softirq, &steal, &guest, &guest_nice) < 4) { + continue; + } + + for (i = 0; i < *nparams; i++) { + virCPUStatsPtr param = ¶ms[i]; + + switch (i) { + case 0: /* fill kernel cpu time here */ + if (virStrcpyStatic(param->field, VIR_CPU_STATS_KERNEL)== NULL) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel cpu time too long for destination")); + goto cleanup; + } + param->value = (sys + irq + softirq) * TICK_TO_NSEC; + break; + + case 1: /* fill user cpu time here */ + if (virStrcpyStatic(param->field, VIR_CPU_STATS_USER) == NULL) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel cpu time too long for destination")); + goto cleanup; + } + param->value = (usr + ni) * TICK_TO_NSEC; + break; + + case 2: /* fill idle cpu time here */ + if (virStrcpyStatic(param->field, VIR_CPU_STATS_IDLE) == NULL) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel cpu time too long for destination")); + goto cleanup; + } + param->value = idle * TICK_TO_NSEC; + break; + + case 3: /* fill iowait cpu time here */ + if (virStrcpyStatic(param->field, VIR_CPU_STATS_IOWAIT) == NULL) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field kernel cpu time too long for destination")); + goto cleanup; + } + param->value = iowait * TICK_TO_NSEC; + break; + + default: + break; + /* should not hit here */ + } + } + ret = 0; + goto cleanup; + } + } + + nodeReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid cpu number")); + +cleanup: + return ret; +} #endif

int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { @@ -416,6 +526,34 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { #endif }

+int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cpuNum, + virCPUStatsPtr params, + int *nparams, + unsigned int flags ATTRIBUTE_UNUSED) +{ + +#ifdef __linux__ + { + int ret; + FILE *procstat = fopen(PROCSTAT_PATH, "r"); + if (!procstat) { + virReportSystemError(errno, + _("cannot open %s"), PROCSTAT_PATH); + return -1; + } + ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams); + VIR_FORCE_FCLOSE(procstat); + + return ret; + } +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node CPU stats 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 88bac6c..361e3e5 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -30,7 +30,11 @@ int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); int nodeCapsInitNUMA(virCapsPtr caps);

- +int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, + int cpuNum, + virCPUStatsPtr params, + int *nparams, + unsigned int flags ATTRIBUTE_UNUSED); int nodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, int startCell, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5632d62..108a37f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8044,6 +8044,7 @@ static virDriver qemuDriver = { .domainBlockPeek = qemudDomainBlockPeek, /* 0.4.4 */ .domainMemoryPeek = qemudDomainMemoryPeek, /* 0.4.4 */ .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */ + .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.4.4 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.4.4 */ .domainEventRegister = qemuDomainEventRegister, /* 0.5.0 */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 536cd8c..471277d 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2218,6 +2218,7 @@ static virDriver umlDriver = { .domainGetAutostart = umlDomainGetAutostart, /* 0.5.0 */ .domainSetAutostart = umlDomainSetAutostart, /* 0.5.0 */ .domainBlockPeek = umlDomainBlockPeek, /* 0.5.0 */ + .nodeGetCPUStats = nodeGetCPUStats, /* 0.9.3 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.5.0 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.5.0 */ .isEncrypted = umlIsEncrypted, /* 0.7.3 */

ACK if the buffer overflow problem is solved Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/14/2011 03:40 AM, Daniel P. Berrange wrote:

On Tue, Jun 14, 2011 at 10:15:36AM +0100, Daniel P. Berrange wrote:

...

On Tue, Jun 07, 2011 at 10:02:55AM +0900, Minoru Usui wrote:

...

virNodeGetCPUStats: Implement linux support

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> +#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) +#define CPU_HEADER_LEN 8 + +int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virCPUStatsPtr params, + int *nparams) +{ + int ret = -1; + char line[1024]; + unsigned long long usr, ni, sys, idle, iowait; + unsigned long long irq, softirq, steal, guest, guest_nice; + char cpu_header[CPU_HEADER_LEN];

...

...

+ } else { + sprintf(cpu_header, "cpu%d", cpuNum); + }

cpu_header is declared to be 8 bytes in size, which only allows for integers upto 4 digits long, before we get a buffer overflow here.

gnulib has some macro which can be used to declare a string buffer large enough to hold an integer, but I can't remember what the macro is called right now. Hopefully Eric will remind us shortly....

Jim Meyering reminded me that it is INT_BUFSIZE_BOUND. So eg you want todo

char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum) + 1];

to allow enough space for 'cpu' + any cpuNum value formatted as a string + the trailing NULL.

Actually, INT_BUFSIZE_BOUND already includes space for the trailing NUL, so the +1 is not needed.

ACK if the buffer overflow problem is solved

Also, 'make syntax-check' complained about cppi indentation, and the fact that we've blacklisted sprintf (use snprintf instead). Plus you had some weird indentation. Here's what I squashed before pushing: diff --git i/src/nodeinfo.c w/src/nodeinfo.c index 235a68a..bdf8f8a 100644 --- i/src/nodeinfo.c +++ w/src/nodeinfo.c @@ -66,10 +66,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, virNodeInfoPtr nodeinfo, bool need_hyperthreads); -int linuxNodeGetCPUStats(FILE *procstat, - int cpuNum, - virCPUStatsPtr params, - int *nparams); +static int linuxNodeGetCPUStats(FILE *procstat, + int cpuNum, + virCPUStatsPtr params, + int *nparams); /* Return the positive decimal contents of the given * CPU_SYS_PATH/cpu%u/FILE, or -1 on error. If MISSING_OK and the @@ -384,8 +384,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, return 0; } -#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) -#define CPU_HEADER_LEN 8 +# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, @@ -396,7 +395,7 @@ int linuxNodeGetCPUStats(FILE *procstat, char line[1024]; unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; - char cpu_header[CPU_HEADER_LEN]; + char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -414,7 +413,7 @@ int linuxNodeGetCPUStats(FILE *procstat, if (cpuNum == VIR_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, "cpu"); } else { - sprintf(cpu_header, "cpu%d", cpuNum); + snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum); } while (fgets(line, sizeof(line), procstat) != NULL) { @@ -436,7 +435,7 @@ int linuxNodeGetCPUStats(FILE *procstat, switch (i) { case 0: /* fill kernel cpu time here */ - if (virStrcpyStatic(param->field, VIR_CPU_STATS_KERNEL)== NULL) { + if (virStrcpyStatic(param->field, VIR_CPU_STATS_KERNEL) == NULL) { nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Field kernel cpu time too long for destination")); goto cleanup; @@ -528,22 +527,23 @@ int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, int cpuNum, virCPUStatsPtr params, int *nparams, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(0, -1); #ifdef __linux__ { - int ret; - FILE *procstat = fopen(PROCSTAT_PATH, "r"); - if (!procstat) { - virReportSystemError(errno, - _("cannot open %s"), PROCSTAT_PATH); - return -1; - } - ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams); - VIR_FORCE_FCLOSE(procstat); + int ret; + FILE *procstat = fopen(PROCSTAT_PATH, "r"); + if (!procstat) { + virReportSystemError(errno, + _("cannot open %s"), PROCSTAT_PATH); + return -1; + } + ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams); + VIR_FORCE_FCLOSE(procstat); - return ret; + return ret; } #else nodeReportError(VIR_ERR_NO_SUPPORT, "%s", -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:03:36AM +0900, Minoru Usui wrote:

virNodeGetMemoryStats: Expose new API

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- include/libvirt/libvirt.h.in | 78 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 2 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 43e59a7..885db25 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -339,6 +339,70 @@ struct _virCPUStats { unsigned long long value; };

+/** + * VIR_MEMORY_STATS_FIELD_LENGTH: + * + * Macro providing the field length of virMemoryStats + */ +#define VIR_MEMORY_STATS_FIELD_LENGTH 80 + +/** + * VIR_MEMORY_STATS_ALL_CELLS: + * + * Macro for the total memory of all cells. + */ +#define VIR_MEMORY_STATS_ALL_CELLS -1 + +/** + * VIR_MEMORY_STATS_TOTAL: + * + * Macro for the total memory of specified cell: + * it represents the maximum memory. + */ + +#define VIR_MEMORY_STATS_TOTAL "total" + +/** + * VIR_MEMORY_STATS_FREE: + * + * Macro for the free memory of specified cell: + * On linux, it includes buffer and cached memory, in case of + * VIR_MEMORY_STATS_ALL_CELLS. + */ + +#define VIR_MEMORY_STATS_FREE "free" + +/** + * VIR_MEMORY_STATS_BUFFERS: + * + * Macro for the buffer memory: On linux, it only returns in case of + * VIR_MEMORY_STATS_ALL_CELLS. + */ + +#define VIR_MEMORY_STATS_BUFFERS "buffers" + +/** + * VIR_MEMORY_STATS_CACHED: + * + * Macro for the cached memory: On linux, it only returns in case of + * VIR_MEMORY_STATS_ALL_CELLS. + */ + +#define VIR_MEMORY_STATS_CACHED "cached" + +/** + * virMemoryStats: + * + * a virMemoryStats is a structure filled by virNodeGetMemoryStats() + * and providing the information of the memory of the Node. + */ + +typedef struct _virMemoryStats virMemoryStats; + +struct _virMemoryStats { + char field[VIR_MEMORY_STATS_FIELD_LENGTH]; + unsigned long long value; +};

/* Common data types shared among interfaces with name/type/value lists. */

@@ -611,6 +675,14 @@ typedef virNodeInfo *virNodeInfoPtr; typedef virCPUStats *virCPUStatsPtr;

/** + * virMemoryStatsPtr: + * + * a virMemoryStatsPtr is a pointer to a virMemoryStats structure. + */ + +typedef virMemoryStats *virMemoryStatsPtr; + +/** * virConnectFlags * * Flags when opening a connection to a hypervisor @@ -750,6 +822,12 @@ int virNodeGetCPUStats (virConnectPtr conn, int *nparams, unsigned int flags);

+int virNodeGetMemoryStats (virConnectPtr conn, + int cellNum, + virCPUStatsPtr params, + int *nparams, + unsigned int flags); + unsigned long long virNodeGetFreeMemory (virConnectPtr conn);

int virNodeGetSecurityModel (virConnectPtr conn, diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 271b8e3..3aadc6f 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -453,6 +453,7 @@ LIBVIRT_0.9.2 { LIBVIRT_0.9.3 { global: virNodeGetCPUStats; + virNodeGetMemoryStats; } LIBVIRT_0.9.2;

Indentation went wrong here, but ACK if that is fixed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Tue, Jun 07, 2011 at 10:04:54AM +0900, Minoru Usui wrote:

virNodeGetMemoryStats: Implement public API

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- include/libvirt/libvirt.h.in | 2 +- src/libvirt.c | 87 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 885db25..b7772ba 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -824,7 +824,7 @@ int virNodeGetCPUStats (virConnectPtr conn,

int virNodeGetMemoryStats (virConnectPtr conn, int cellNum, - virCPUStatsPtr params, + virMemoryStatsPtr params, int *nparams, unsigned int flags);

Opps, I think this chunk should be in your first patch.

diff --git a/src/libvirt.c b/src/libvirt.c index 45d9be7..976eaa0 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5429,6 +5429,93 @@ error: }

/** + * virNodeGetMemoryStats: + * @conn: pointer to the hypervisor connection. + * @cellNum: number of node cell. (VIR_MEMORY_STATS_ALL_CELLS means total cell + * statistics) + * @params: pointer to node memory stats objects + * @nparams: number of node memory stats (this value should be same or + * less than the number of stats supported) + * @flags: currently unused, for future extension. always pass 0. + * + * This function provides memory stats of the node. + * If you want to get total cpu statistics of the node, you must specify + * VIR_MEMORY_STATS_ALL_CELLS to @cellNum. + * The @params array will be filled with the values equal to the number of + * stats suggested by @nparams + * + * As the value of @nparams is dynamic, call the API setting @nparams to 0 and + * @params as NULL, the API returns the number of parameters supported by the + * HV by updating @nparams on SUCCESS. The caller should then allocate @params + * array, i.e. (sizeof(@virMemoryStats) * @nparams) bytes and call + * the API again. + * + * Here is the sample code snippet: + * + * if ((virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, 0) == 0) && + * (nparams != 0)) { + * params = vshMalloc(ctl, sizeof(virMemoryStats) * nparams); + * memset(params, cellNum, 0, sizeof(virMemoryStats) * nparams); + * if (virNodeGetMemoryStats(conn, params, &nparams, 0)) { + * vshError(ctl, "%s", _("Unable to get node memory stats")); + * goto error; + * } + * }

Same comment as the earlier patch - use malloc() and fprintf(stderr) here.

+ * + * This function doesn't requires privileged access to the hypervisor. + * This function expects the caller to allocate the @params. + * + * Memory Stats: + * + * VIR_MEMORY_STATS_TOTAL: + * The total memory usage.(KB) + * VIR_MEMORY_STATS_FREE: + * The free memory usage.(KB) + * On linux, this usage includes buffers and cached. + * VIR_MEMORY_STATS_BUFFERS: + * The buffers memory usage.(KB) + * VIR_MEMORY_STATS_CACHED: + * The cached memory usage.(KB) + * + * Returns -1 in case of error, 0 in case of success. + */ +int virNodeGetMemoryStats (virConnectPtr conn, + int cellNum, + virMemoryStatsPtr params, + int *nparams, unsigned int flags) +{ + VIR_DEBUG("conn=%p, cellNum=%d, params=%p, nparams=%d, flags=%u", + conn, cellNum, params, nparams ? *nparams : -1, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if ((nparams == NULL) || (*nparams < 0) || + ((cellNum < 0) && (cellNum != VIR_MEMORY_STATS_ALL_CELLS))) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->nodeGetMemoryStats) { + int ret; + ret = conn->driver->nodeGetMemoryStats (conn, cellNum, params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** * virNodeGetFreeMemory: * @conn: pointer to the hypervisor connection *

ACK, if those 2 points above are addressed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/14/2011 03:18 AM, Daniel P. Berrange wrote:

On Tue, Jun 07, 2011 at 10:05:40AM +0900, Minoru Usui wrote:

...

virNodeGetMemoryStats: Implement remote protocol

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- daemon/remote.c | 77 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 65 +++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 22 +++++++++++- 3 files changed, 163 insertions(+), 1 deletions(-)

...

@@ -2317,7 +2336,8 @@ enum remote_procedure { REMOTE_PROC_INTERFACE_CHANGE_ROLLBACK = 222, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_SCHEDULER_PARAMETERS_FLAGS = 223, /* skipgen autogen */ REMOTE_PROC_DOMAIN_EVENT_CONTROL_ERROR = 224, /* skipgen skipgen */ - REMOTE_PROC_NODE_GET_CPU_STATS = 225 /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_CPU_STATS = 225, /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_MEMORY_STATS = 226 /* skipgen skipgen */

Another merge conflict, and again missing remote_protocol-structs changes.

...

/* * Notice how the entries are grouped in sets of 10 ?

ACK

Pushed with this squashed in. diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index ea7bdd2..ef5f266 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -75,6 +75,10 @@ struct remote_node_get_cpu_stats { remote_nonnull_string field; uint64_t value; }; +struct remote_node_get_memory_stats { + remote_nonnull_string field; + uint64_t value; +}; struct remote_open_args { remote_string name; int flags; @@ -137,6 +141,18 @@ struct remote_node_get_cpu_stats_ret { } params; int nparams; }; +struct remote_node_get_memory_stats_args { + int nparams; + int cellNum; + u_int flags; +}; +struct remote_node_get_memory_stats_ret { + struct { + u_int params_len; + remote_node_get_memory_stats * params_val; + } params; + int nparams; +}; struct remote_node_get_cells_free_memory_args { int startCell; int maxcells; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jun 07, 2011 at 10:09:15AM +0900, Minoru Usui wrote:

virNodeGetMemoryStats: Implement virsh support

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- tools/virsh.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++ 2 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 2fbc91a..c419be9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3815,6 +3815,67 @@ cmdNodeCPUStats(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) }

/* + * "nodememstats" command + */ +static const vshCmdInfo info_nodememstats[] = { + {"help", N_("Prints memory stats of the node.")}, + {"desc", N_("Returns memory stats of the node.(KB)")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_node_memstats[] = { + {"cell", VSH_OT_INT, 0, N_("prints specified cell statistics only.")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdNodememstats(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) +{ + int nparams = 0; + unsigned int i = 0; + int cellNum = VIR_MEMORY_STATS_ALL_CELLS; + virMemoryStatsPtr params = NULL; + bool ret = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (vshCommandOptInt(cmd, "cell", &cellNum) < 0) { + vshError(ctl, "%s", _("Invalid value of cellNum")); + return false; + } + + /* get the number of memory parameters */ + if (virNodeGetMemoryStats(ctl->conn, cellNum, NULL, &nparams, 0) != 0) { + vshError(ctl, "%s", + _("Unable to get number of memory stats")); + goto cleanup; + } + + if (nparams == 0) { + /* nothing to output */ + ret = true; + goto cleanup; + } + + /* now go get all the memory parameters */ + params = vshCalloc(ctl, nparams, sizeof(*params)); + if (virNodeGetMemoryStats(ctl->conn, cellNum, params, &nparams, 0) != 0) { + vshError(ctl, "%s", _("Unable to get memory stats")); + goto cleanup; + } + + for (i = 0; i < nparams; i++) + vshPrint(ctl, "%-7s: %20llu kB\n", params[i].field, params[i].value); + + ret = true; + + cleanup: + VIR_FREE(params); + return ret; +} + +/* * "capabilities" command */ static const vshCmdInfo info_capabilities[] = { @@ -11427,6 +11488,7 @@ static const vshCmdDef hostAndHypervisorCmds[] = { {"hostname", cmdHostname, NULL, info_hostname, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, {"nodecpustats", cmdNodeCPUStats, opts_node_cpustats, info_nodecpustats}, + {"nodememstats", cmdNodememstats, opts_node_memstats, info_nodememstats, 0}, {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command, 0}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index ade4eab..1556765 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -244,6 +244,11 @@ If I<--cpu> is specified, this will prints specified cpu statistics only. If I<--percent> is specified, this will prints percentage of each kind of cpu statistics during 1 second.

+=item B<nodememstats> optional I<--cell> + +Returns memory stats of the node. +If I<--cell> is specified, this will prints specified cell statistics only. + =item B<capabilities>

Print an XML document describing the capabilities of the hypervisor

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/14/2011 03:21 AM, Daniel P. Berrange wrote:

On Tue, Jun 07, 2011 at 10:11:17AM +0900, Minoru Usui wrote:

...

virNodeGetMemoryStats: Implement linux support

Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 1 + src/nodeinfo.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 5 ++ src/qemu/qemu_driver.c | 1 + src/uml/uml_driver.c | 1 + 6 files changed, 170 insertions(+), 0 deletions(-)

+ + sprintf(meminfo_path, "%s/node%d/meminfo", NODE_SYS_PATH, cellNum); + }

Instead of using a static buffer, it is preferrable to use virAsprintf() here to construct meminfo_path.

ACK with this squashed in (whitespace ignored, the real diff was a bit bigger but had more indentation changed), so I pushed: diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms index 57243f1..7a05539 100644 --- i/src/libvirt_private.syms +++ w/src/libvirt_private.syms @@ -734,10 +734,10 @@ virNodeDeviceObjUnlock; # nodeinfo.h nodeCapsInitNUMA; nodeGetCPUStats; -nodeGetMemoryStats; nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; +nodeGetMemoryStats; # nwfilter_conf.h diff --git i/src/nodeinfo.c w/src/nodeinfo.c index 809fc44..cb2f805 100644 --- i/src/nodeinfo.c +++ w/src/nodeinfo.c @@ -559,9 +563,8 @@ int linuxNodeGetMemoryStats(FILE *meminfo, buf = p; } - if (sscanf(buf, "%s %lu kB", meminfo_hdr, &val) < 2) { + if (sscanf(buf, "%s %lu kB", meminfo_hdr, &val) < 2) continue; - } for (j = 0; field_conv[j].meminfo_hdr != NULL; j++) { struct field_conv *convp = &field_conv[j]; @@ -579,10 +582,9 @@ int linuxNodeGetMemoryStats(FILE *meminfo, break; } } - if (found >= nr_param) { + if (found >= nr_param) break; } - } if (found == 0) { nodeReportError(VIR_ERR_INTERNAL_ERROR, @@ -666,16 +668,22 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, int cellNum, virMemoryStatsPtr params, int *nparams, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(0, -1); + #ifdef __linux__ { int ret; - char meminfo_path[1024]; + char *meminfo_path = NULL; FILE *meminfo; if (cellNum == VIR_MEMORY_STATS_ALL_CELLS) { - sprintf(meminfo_path, MEMINFO_PATH); + meminfo_path = strdup(MEMINFO_PATH); + if (!meminfo_path) { + virReportOOMError(); + return -1; + } } else { if (numa_available() < 0) { nodeReportError(VIR_ERR_NO_SUPPORT, @@ -684,21 +692,28 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, } if (cellNum > numa_max_node()) { - nodeReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid cell number")); + nodeReportError(VIR_ERR_INVALID_ARG, "%s", + _("Invalid cell number")); return -1; } - sprintf(meminfo_path, "%s/node%d/meminfo", NODE_SYS_PATH, cellNum); + if (virAsprintf(&meminfo_path, "%s/node%d/meminfo", + NODE_SYS_PATH, cellNum) < 0) { + virReportOOMError(); + return -1; + } } meminfo = fopen(meminfo_path, "r"); if (!meminfo) { virReportSystemError(errno, - _("cannot open %s"), MEMINFO_PATH); + _("cannot open %s"), meminfo_path); + VIR_FREE(meminfo_path); return -1; } ret = linuxNodeGetMemoryStats(meminfo, cellNum, params, nparams); VIR_FORCE_FCLOSE(meminfo); + VIR_FREE(meminfo_path); return ret; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org