On Fri, 20 May 2011 11:42:26 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:

On Tue, May 17, 2011 at 04:00:07PM +0900, Minoru Usui wrote:

...

virNodeGetCPUTimeParameters: Implement virsh support

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

diff --git a/tools/virsh.c b/tools/virsh.c index a38750f..7c9fa29 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3453,6 +3453,112 @@ cmdNodeinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) }

/* + * "nodecputime" command + */ +static const vshCmdInfo info_nodecputime[] = { + {"help", N_("Prints cpu utilization of the node.")}, + {"desc", N_("Returns cpu utilization of the node.(%)")}, + {NULL, NULL} +}; + +static bool +cmdNodeCPUTimeParameters(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + int i, j; + bool flag_utilization = false; + virCPUTimeParameterPtr params; + int nparams = 0; + bool ret = false; + struct cpu_time { + unsigned long long user; + unsigned long long sys; + unsigned long long idle; + unsigned long long iowait; + unsigned long long util; + } cpu_time[2]; + double user_time, sys_time, idle_time, iowait_time, total_time; + double usage; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (virNodeGetCPUTimeParameters(ctl->conn, NULL, &nparams, 0) != 0) { + vshError(ctl, "%s", + _("Unable to get number of cpu time parameters")); + return false; + } + if (nparams == 0) { + /* nothing to output */ + return true; + } + + memset(cpu_time, 0, sizeof(struct cpu_time) * 2); + params = vshCalloc(ctl, nparams, sizeof(*params)); + for (i = 0; i < 2; i++) { + if (virNodeGetCPUTimeParameters(ctl->conn, params, &nparams, 0) != 0) { + vshError(ctl, "%s", _("Unable to get node cpu time parameters")); + goto cleanup; + } + + for (j = 0; j < nparams; j++) { + unsigned long long value = params[j].value; + + if (strcmp(params[j].field, VIR_CPU_TIME_KERNEL) == 0) + cpu_time[i].sys = value; + + if (strcmp(params[j].field, VIR_CPU_TIME_USER) == 0) + cpu_time[i].user = value; + + if (strcmp(params[j].field, VIR_CPU_TIME_IDLE) == 0) + cpu_time[i].idle = value; + + if (strcmp(params[j].field, VIR_CPU_TIME_IOWAIT) == 0) + cpu_time[i].iowait = value; + + if (strcmp(params[j].field, VIR_CPU_TIME_UTILIZATION) == 0) { + cpu_time[i].util = value; + flag_utilization = true; + break; + } + } + + sleep(1); + } + + if (flag_utilization == true) { + usage = cpu_time[0].util; + + vshPrint(ctl, "%-15s %5.1lf%%\n", _("usage:"), usage); + vshPrint(ctl, "%-15s %5.1lf%%\n", _("idle :"), 100 - usage); + } else { + user_time = cpu_time[1].user - cpu_time[0].user; + sys_time = cpu_time[1].sys - cpu_time[0].sys; + idle_time = cpu_time[1].idle - cpu_time[0].idle; + iowait_time = cpu_time[1].iowait - cpu_time[0].iowait; + total_time = user_time + sys_time + idle_time + iowait_time; + + usage = (user_time + sys_time) / total_time * 100; + + vshPrint(ctl, "%-15s %5.1lf%%\n", + _("usage:"), usage); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _(" user :"), user_time / total_time * 100); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _(" system:"), sys_time / total_time * 100); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _("idle :"), idle_time / total_time * 100); + vshPrint(ctl, "%-15s %5.1lf%%\n", + _("iowait:"), iowait_time / total_time * 100); + } + + ret = true; + + cleanup: + VIR_FREE(params); + return ret; +}

I'm not sure this is the best way todo this command. I think we want a way to see the raw/absolute values without a sleep() in there by default. Only do the repeated calls+sleep+delta calculation if asked by some flag

Please see the following thread. http://www.mail-archive.com/libvir-list@redhat.com/msg36821.html I received same comment for v2 patch from you, and I asked you the question but you didn't answer. Could I ask you a same question? If we don't care the ESX hypervisor, I'll change this virsh subcommand prints out raw/absolute values. But I think it is not legal for libvirt.

...

+ +/* * "capabilities" command */ static const vshCmdInfo info_capabilities[] = { @@ -10908,6 +11014,7 @@ static const vshCmdDef hostAndHypervisorCmds[] = { {"freecell", cmdFreecell, opts_freecell, info_freecell}, {"hostname", cmdHostname, NULL, info_hostname}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo}, + {"nodecputime", cmdNodeCPUTimeParameters, NULL, info_nodecputime}, {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo}, {"uri", cmdURI, NULL, info_uri}, diff --git a/tools/virsh.pod b/tools/virsh.pod index d11a0e3..fe0755f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -237,6 +237,10 @@ 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<nodecputime> + +Returns cpu utilization of the node. + =item B<capabilities>

Print an XML document describing the capabilities of the hypervisor

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 :|

-- Minoru Usui <usui@mxm.nes.nec.co.jp>

On Tue, May 17, 2011 at 04:01:10PM +0900, Minoru Usui wrote:

virNodeGetCPUTimeParameters: 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 | 127 ++++++++++++++++++++++++++++++++++++++++++++++ src/nodeinfo.h | 5 ++- src/qemu/qemu_driver.c | 1 + src/uml/uml_driver.c | 1 + 6 files changed, 135 insertions(+), 1 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index acd5af3..c3c0a88 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -687,6 +687,7 @@ virNodeDeviceObjUnlock;

# nodeinfo.h nodeCapsInitNUMA; +nodeGetCPUTimeParameters; nodeGetCellsFreeMemory; nodeGetFreeMemory; nodeGetInfo; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2bb592d..4e07836 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2752,6 +2752,7 @@ static virDriver lxcDriver = { .domainGetSchedulerParameters = lxcGetSchedulerParameters, /* 0.5.0 */ .domainSetSchedulerParameters = lxcSetSchedulerParameters, /* 0.5.0 */ .domainInterfaceStats = lxcDomainInterfaceStats, /* 0.7.3 */ + .nodeGetCPUTimeParameters = nodeGetCPUTimeParameters, /* 0.9.2 */ .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..7fdf878 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -57,12 +57,19 @@ #ifdef __linux__ # define CPUINFO_PATH "/proc/cpuinfo" # define CPU_SYS_PATH "/sys/devices/system/cpu" +# define PROCSTAT_PATH "/proc/stat" + +#define LINUX_NB_CPU_TIME_PARAM 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 linuxNodeGetCPUTimeParameters(FILE *procstat, + virCPUTimeParameterPtr 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 +385,99 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, return 0; }

+#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) + +int linuxNodeGetCPUTimeParameters(FILE *procstat, + virCPUTimeParameterPtr 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; + + if ((*nparams) == 0) { + /* Current number of cpu time parameters supported by linux */ + *nparams = LINUX_NB_CPU_TIME_PARAM; + ret = 0; + goto cleanup; + } + + if ((*nparams) != LINUX_NB_CPU_TIME_PARAM) { + nodeReportError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto cleanup; + } + + while (fgets(line, sizeof(line), procstat) != NULL) { + char *buf = line; + + if (STRPREFIX(buf, "cpu ")) { /* aka total 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++) { + virCPUTimeParameterPtr param = ¶ms[i]; + + switch (i) { + case 0: /* fill kernel cpu time here */ + if (virStrcpyStatic(param->field, VIR_CPU_TIME_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_TIME_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_TIME_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_TIME_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_INTERNAL_ERROR, "%s", _("no \'cpu \' line found")); + +cleanup: + return ret; +} #endif

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

+int nodeGetCPUTimeParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virCPUTimeParameterPtr 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 = linuxNodeGetCPUTimeParameters(procstat, params, nparams); + VIR_FORCE_FCLOSE(procstat); + + return ret; + } +#else + nodeReportError(VIR_ERR_NO_SUPPORT, "%s", + _("node CPU time 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..784492c 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -30,7 +30,10 @@ int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); int nodeCapsInitNUMA(virCapsPtr caps);

- +int nodeGetCPUTimeParameters(virConnectPtr conn ATTRIBUTE_UNUSED, + virCPUTimeParameterPtr 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 ccaae66..579e3e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7631,6 +7631,7 @@ static virDriver qemuDriver = { .domainBlockPeek = qemudDomainBlockPeek, /* 0.4.4 */ .domainMemoryPeek = qemudDomainMemoryPeek, /* 0.4.4 */ .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */ + .nodeGetCPUTimeParameters = nodeGetCPUTimeParameters, /* 0.9.2 */ .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..c9138ce 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 */ + .nodeGetCPUTimeParameters = nodeGetCPUTimeParameters, /* 0.9.2 */ .nodeGetCellsFreeMemory = nodeGetCellsFreeMemory, /* 0.5.0 */ .nodeGetFreeMemory = nodeGetFreeMemory, /* 0.5.0 */ .isEncrypted = umlIsEncrypted, /* 0.7.3 */

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 Tue, May 17, 2011 at 04:01:36PM +0900, Minoru Usui wrote:

virNodeGetCPUTimeParameters: Expose new API

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

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f4d0b40..723fdf8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -280,6 +280,58 @@ struct _virNodeInfo { unsigned int threads;/* number of threads per core */ };

+/** + * VIR_CPU_TIME_FIELD_LENGTH: + * + * Macro providing the field length of virNodeCPUTimeParameters + */ +#define VIR_CPU_TIME_FIELD_LENGTH 80 + +/** + * VIR_CPU_TIME_KERNEL: + * + * Macro for the cumulative CPU time which spends by kernel, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_KERNEL "kernel" + +/** + * The cumulative CPU time which spends by user processes, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_USER "user" + +/** + * The cumulative idle CPU time, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_IDLE "idle" + +/** + * The cumulative I/O wait CPU time, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_IOWAIT "iowait" + +/** + * The CPU utilization. + * The usage value is in percent and 100% represents all CPUs on + * the server. + */ +#define VIR_CPU_TIME_UTILIZATION "utilization" + +/** + * virCPUTimeParameter: + * + * a virNodeCPUTimeParameter is a structure filled by virNodeGetCPUTime() + * and providing the information for the cpu time of the node. + */ +typedef struct _virCPUTimeParameter virCPUTimeParameter; + +struct _virCPUTimeParameter { + char field[VIR_CPU_TIME_FIELD_LENGTH]; + unsigned long long value; +};

/** * virDomainSchedParameterType: @@ -512,6 +564,14 @@ int virDomainMigrateSetMaxSpeed(virDomainPtr domain, typedef virNodeInfo *virNodeInfoPtr;

/** + * virCPUTimeParameterPtr: + * + * a virCPUTimeParameterPtr is a pointer to a virCPUTimeParameter structure. + */ + +typedef virCPUTimeParameter *virCPUTimeParameterPtr; + +/** * virConnectFlags * * Flags when opening a connection to a hypervisor @@ -645,6 +705,11 @@ int virNodeGetInfo (virConnectPtr conn, virNodeInfoPtr info); char * virConnectGetCapabilities (virConnectPtr conn);

+int virNodeGetCPUTimeParameters (virConnectPtr conn, + virCPUTimeParameterPtr 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 1444b55..fb8d3e0 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -441,6 +441,7 @@ LIBVIRT_0.9.2 { virDomainGetState; virDomainInjectNMI; virDomainScreenshot; + virNodeGetCPUTimeParameters; } LIBVIRT_0.9.0;

# .... define new API here using predicted next version number ....

ACK. I'm wondering whether virCPUTimeParameter should be dropped and just use the generic virTypedParameter instead. It would be rather overkill since we only really want to ever use 'unsigned long long' for this data, but it might be simpler for apps to have the same struct, even if we only use one field of it. 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 :|

Hi, Daniel, Matthias. Thank you for your comment. On Fri, 20 May 2011 13:55:20 +0200 Matthias Bolte <matthias.bolte@googlemail.com> wrote:

2011/5/20 Daniel P. Berrange <berrange@redhat.com>:

...

On Tue, May 17, 2011 at 04:01:36PM +0900, Minoru Usui wrote:

...

virNodeGetCPUTimeParameters: Expose new API

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

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f4d0b40..723fdf8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -280,6 +280,58 @@ struct _virNodeInfo {      unsigned int threads;/* number of threads per core */  };

+/** + * VIR_CPU_TIME_FIELD_LENGTH: + * + * Macro providing the field length of virNodeCPUTimeParameters + */ +#define VIR_CPU_TIME_FIELD_LENGTH 80 + +/** + * VIR_CPU_TIME_KERNEL: + * + * Macro for the cumulative CPU time which spends by kernel, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_KERNEL "kernel" + +/** + * The cumulative CPU time which spends by user processes, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_USER "user" + +/** + * The cumulative idle CPU time, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_IDLE "idle" + +/** + * The cumulative I/O wait CPU time, + * when the node booting up.(in nanoseconds). + */ +#define VIR_CPU_TIME_IOWAIT "iowait" + +/** + * The CPU utilization. + * The usage value is in percent and 100% represents all CPUs on + * the server. + */ +#define VIR_CPU_TIME_UTILIZATION "utilization" + +/** + * virCPUTimeParameter: + * + * a virNodeCPUTimeParameter is a structure filled by virNodeGetCPUTime() + * and providing the information for the cpu time of the node. + */ +typedef struct _virCPUTimeParameter virCPUTimeParameter; + +struct _virCPUTimeParameter { +    char field[VIR_CPU_TIME_FIELD_LENGTH]; +    unsigned long long value; +};

 /**   * virDomainSchedParameterType: @@ -512,6 +564,14 @@ int virDomainMigrateSetMaxSpeed(virDomainPtr domain,  typedef virNodeInfo *virNodeInfoPtr;

 /** + * virCPUTimeParameterPtr: + * + * a virCPUTimeParameterPtr is a pointer to a virCPUTimeParameter structure. + */ + +typedef virCPUTimeParameter *virCPUTimeParameterPtr; + +/**   * virConnectFlags   *   * Flags when opening a connection to a hypervisor @@ -645,6 +705,11 @@ int                     virNodeGetInfo          (virConnectPtr conn,                                                   virNodeInfoPtr info);  char *                  virConnectGetCapabilities (virConnectPtr conn);

+int                     virNodeGetCPUTimeParameters (virConnectPtr conn, +                                                 virCPUTimeParameterPtr 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 1444b55..fb8d3e0 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -441,6 +441,7 @@ LIBVIRT_0.9.2 {          virDomainGetState;          virDomainInjectNMI;          virDomainScreenshot; +        virNodeGetCPUTimeParameters;  } LIBVIRT_0.9.0;

 # .... define new API here using predicted next version number ....

ACK.

I'm wondering whether virCPUTimeParameter should be dropped and just use the generic virTypedParameter instead. It would be rather overkill since we only really want to ever use  'unsigned long long' for this data, but it might be simpler for apps to have the same struct, even if we only use one field of it.

Daniel

Actually virNodeGetCPUTimeParameters is the wrong name. It's not a parameter function, it's a statistics function. Therefore, it should be called virNodeGetCPUTimeStats or virNodeGetCPUStats.

Matthias

I'll rename to virNodeGetCPUTimeStats(). And I'll change to use virTypeParameter because it's simpler the code. -- Minoru Usui <usui@mxm.nes.nec.co.jp>

On Fri, 20 May 2011 11:35:20 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:

On Tue, May 17, 2011 at 04:02:02PM +0900, Minoru Usui wrote:

...

virNodeGetCPUTime: Implement public API

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

diff --git a/src/libvirt.c b/src/libvirt.c index 5a5439d..6828e1a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4930,6 +4930,93 @@ error: }

/** + * virNodeGetCPUTime: + * @conn: a connection object + * @params: pointer to node cpu time parameter objects + * @nparams: number of node cpu time parameter (this value should be same or + * less than the number of parameters supported) + * @flags: currently unused, for future extension. always pass 0. + * + * This function provides cpu time statistics of the node. + * The @params array will be filled with the values equal to the number of + * parameters 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(@virCPUTimeParameter) * @nparams) bytes and call + * the API again. + * + * Here is the sample code snippet: + * + * if ((virNodeGetCPUTimeParameters(conn, NULL, &nparams, 0) == 0) && + * (nparams != 0)) { + * params = vshMalloc(ctl, sizeof(virCPUTimeParameter) * nparams); + * memset(params, 0, sizeof(virCPUTimeParameter) * nparams); + * if (virNodeGetCPUTimeParameters(conn, params, &nparams, 0)) { + * vshError(ctl, "%s", _("Unable to get node cpu time parameters")); + * goto error; + * } + * } + * + * This function requires privileged access to the hypervisor. This function + * expects the caller to allocate the @params. + * + * CPU time Statistics: + * + * VIR_NODE_CPU_TIME_KERNEL: + * The cumulative CPU time which spends by kernel, + * when the node booting up.(nanoseconds) + * VIR_NODE_CPU_TIME_USER: + * The cumulative CPU time which spends by user processes, + * when the node booting up.(nanoseconds) + * VIR_NODE_CPU_TIME_IDLE: + * The cumulative idle CPU time, when the node booting up.(nanoseconds) + * VIR_NODE_CPU_TIME_IOWAIT: + * The cumulative I/O wait CPU time, when the node booting up.(nanoseconds) + * VIR_NODE_CPU_TIME_UTILIZATION: + * The CPU utilization. The usage value is in percent and 100% + * represents all CPUs on the server. + * + * Returns -1 in case of error, 0 in case of success. + */ +int virNodeGetCPUTimeParameters (virConnectPtr conn, + virCPUTimeParameterPtr params, + int *nparams, unsigned int flags) +{ + VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=%u", + conn, params, (nparams) ? *nparams : -1, flags);

'(nparams)' is redundant, just use 'nparams'

OK. I'll change it.

...

+ + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + virCheckFlags(flags, 0);

The virCheckFlags call should only be in the per-hypervisor method implementations. This entry point needs to allow any/all flags to be passed through, since it does not know which the hypervisor will considered appropriate. So just delete this line

OK. I'll fix it.

...

+ + if ((nparams == NULL) || (*nparams < 0)) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->nodeGetCPUTimeParameters) { + int ret; + ret = conn->driver->nodeGetCPUTimeParameters (conn, 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 *

The comment says this requires privileged access to the hypervisor, but the code is not checking for whether the readonly flag is set. IMHO this api is safe for readonly usage, so perhaps the comment needs changing to say it does not require privileged access.

OK. I'll change it. -- Minoru Usui <usui@mxm.nes.nec.co.jp>

On Tue, May 17, 2011 at 04:02:21PM +0900, Minoru Usui wrote:

virNodeGetCPUTimeParameters: Implement remote protocol

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

diff --git a/daemon/remote.c b/daemon/remote.c index ea36bf5..7ebaec7 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1756,6 +1756,82 @@ cleanup: return rv; }

+static int +remoteDispatchNodeGetCPUTimeParameters (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_time_parameters_args *args, + remote_node_get_cpu_time_parameters_ret *ret) +{ + virCPUTimeParameterPtr params = NULL; + int i; + 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_TIME_PARAMETERS_MAX) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + if (VIR_ALLOC_N(params, nparams) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virNodeGetCPUTimeParameters(conn, params, &nparams, flags) < 0) + goto cleanup; + + /* In this case, we need to send back the number of parameters + * 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 45d0f80..55f53ba 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1725,6 +1725,70 @@ done: }

static int +remoteNodeGetCPUTimeParameters (virConnectPtr conn, + virCPUTimeParameterPtr params, int *nparams, + unsigned int flags) +{ + int rv = -1; + remote_node_get_cpu_time_parameters_args args; + remote_node_get_cpu_time_parameters_ret ret; + int i = -1; + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + args.nparams = *nparams; + args.flags = flags; + + memset (&ret, 0, sizeof ret); + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_TIME_PARAMETERS, + (xdrproc_t) xdr_remote_node_get_cpu_time_parameters_args, + (char *) &args, + (xdrproc_t) xdr_remote_node_get_cpu_time_parameters_ret, + (char *) &ret) == -1) + goto done; + + /* Check the length of the returned list carefully. */ + if (ret.params.params_len > REMOTE_NODE_CPU_TIME_PARAMETERS_MAX || + ret.params.params_len > *nparams) { + remoteError(VIR_ERR_RPC, "%s", + _("remoteNodeGetCPUTimeParameters: " + "returned number of parameters exceeds limit")); + goto cleanup; + } + /* Handle the case when the caller does not know the number of parameters + * and is asking for the number of parameters 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, + _("Parameter %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_time_parameters_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, int startCell, @@ -6836,6 +6900,7 @@ static virDriver remote_driver = { .domainBlockPeek = remoteDomainBlockPeek, /* 0.4.2 */ .domainMemoryPeek = remoteDomainMemoryPeek, /* 0.4.2 */ .domainGetBlockInfo = remoteDomainGetBlockInfo, /* 0.8.1 */ + .nodeGetCPUTimeParameters = remoteNodeGetCPUTimeParameters, /* 0.9.2 */ .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 e204eb0..610d51f 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 cpu time stats. */ +const REMOTE_NODE_CPU_TIME_PARAMETERS_MAX = 16; + /* Upper limit on number of NUMA cells */ const REMOTE_NODE_MAX_CELLS = 1024;

@@ -359,6 +362,11 @@ struct remote_memory_param { remote_memory_param_value value; };

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

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

+struct remote_node_get_cpu_time_parameters_args { + int nparams; + unsigned int flags; +}; + +struct remote_node_get_cpu_time_parameters_ret { + remote_node_get_cpu_time_param params<REMOTE_NODE_CPU_TIME_PARAMETERS_MAX>; + int nparams; +}; + struct remote_node_get_cells_free_memory_args { int startCell; int maxCells; @@ -2284,7 +2302,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3 = 215, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_MIGRATE_PERFORM3 = 216, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_MIGRATE_FINISH3 = 217, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_MIGRATE_CONFIRM3 = 218 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_MIGRATE_CONFIRM3 = 218, /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_CPU_TIME_PARAMETERS = 219 /* skipgen skipgen */

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 :|