Re: [libvirt] [PATCH 1/5 V3] libvirt: Add new public API virDomainGetPcpusUsage

[Richard W.M. Jones]

On Wed, Jan 18, 2012 at 05:07:16PM -0700, Eric Blake wrote:
 If others like my thoughts, can you respin your patch series to adjust
 your API accordingly?

Sounds good to me.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

Re: [libvirt] [PATCH 1/5 V3] libvirt: Add new public API virDomainGetPcpusUsage

[Eric Blake]

On 01/18/2012 12:12 AM, Lai Jiangshan wrote:
 Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
 ---
  include/libvirt/libvirt.h.in |5 
  python/generator.py  |1 +
  src/driver.h |7 +
  src/libvirt.c|   51 
 ++
  src/libvirt_public.syms  |5 
  5 files changed, 69 insertions(+), 0 deletions(-)

 +
 +/**
 + * virDomainGetPcpusUsage:
 + * @dom: pointer to domain object
 + * @usages: returned physical cpu usages
 + * @nr_usages: length of @usages
 + * @flags: flags to control the operation
 + *
 + * Get the cpu usages for every physical cpu since the domain started (in 
 nanoseconds).
 + *
 + * Returns 0 if success, -1 on error
 + */
 +int virDomainGetPcpusUsage(virDomainPtr dom,
 +   unsigned long long *usages,
 +   int *nr_usages,
 +   unsigned int flags)

Overall, I think this patch series is headed in the right direction.
And the idea of using the cpuacct cgroup for per-cpu usage numbers is a
cool hack.  However, I think this API is a bit too restricted, and that
a few tweaks can make the API more powerful and avoid the need for
adding new API in the future (instead, just adding new named parameter
types to this API).

First, we don't use the term Pcpu anywhere else in libvirt.  I'm
thinking it would be better to name this virDomainGetCPUStats, to match
our existing virNodeGetCPUStats.  (Too bad that we weren't consistent
between CPU vs. Vcpu in capitalization.)  Also, by naming it just
CPUStats, rather than PcpusUsate, I think we can open the door to
further stats - for example, I can envision that we might want to get
stats not only on how much cpu accounting has been attributed to the
host qemu process, but we can also communicate to a guest agent and get
the guest's own view of its cpu usage; the difference between the two
values would thus be the overhead consumed by the hypervisor in
presenting an environment to the guest.  Of course, we don't have to
integrate with a guest agent now, but we should be designing the API
with that in mind.

Next, I notice that right now, cpuacct provides the following
properties: cpuacct.stat (gives elapsed user and system times for the
cgroup, similar to 2 of the 3 cumulative accounting values given by the
time(1) utility; looks like the unit is roughly in microseconds),
cpuacct.usage_percpu (gives just nanosecond usage, without division
between user or system), and cpuacct.usage (the sum of cpuacct.usage).
And it is conceivable that future kernels will add even more stats under
the cpuacct umbrella.  But your patch only uses cpuacct.usage_percpu.
Why should we be hard-coding things to just one stat?

Additionally, I'm comparing this with existing APIs, for things we have
learned in the past about querying statistics.  virNodeGetCPUStats is
rather limited in that it can only return the stats of one processor at
a time; but has the benefit that the magic VIR_NODE_CPU_STATS_ALL_CPUS
(aka -1) can provide the sum across all processors into that one stat.
So a machine with 8 processors requires 8 calls to the API, but you can
also get the summary in one blow without doing any additions yourself.
It may also be the case that some stats are available only on the entire
process, rather than per-cpu.  virNodeGetCPUStats also has the drawback
that it can only return unsigned long long values, rather than using
virTypedParameter; but at least it has the benefit that it can return an
arbitrary number of name/value stat tuples.  Meanwhile,
virNodeGetCellsFreeMemory allows the user to request a start and stop
limit, and thus grab multiple stats in one call, but doesn't allow
passing -1 to get a summary (that is, with 8 NUMA cells you can make
just 1 call, but have to do the addition yourself).  It has a drawback
that it can only return one specific stat; it cannot be extended to
other statistics.  And virDomainGetVcpuPinInfo is an example of a
function that returns a 2-dimensional array through a single output
pointer, by passing in two separate length parameters; but again with a
limited output of only one specific stat.

Borrowing from these ideas, I think your API should return an array of
virTypedParameter per cpu, so that we can add new stats without needing
new API.  It should also allow the ability to return a summary over all
cpus, instead of a 2-dimensional list of statistics per cpu.  I'm
thinking something like the following:


/**
 * virDomainGetCPUStats:
 * @dom: 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
 *
 * 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 

[libvirt] [PATCH 1/5 V3] libvirt: Add new public API virDomainGetPcpusUsage

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
 include/libvirt/libvirt.h.in |5 
 python/generator.py  |1 +
 src/driver.h |7 +
 src/libvirt.c|   51 ++
 src/libvirt_public.syms  |5 
 5 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index e436f3c..167e89f 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3606,6 +3606,11 @@ int virConnectSetKeepAlive(virConnectPtr conn,
int interval,
unsigned int count);
 
+int virDomainGetPcpusUsage(virDomainPtr dom,
+   unsigned long long *usages,
+   int *nr_usages,
+   unsigned int flags);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/python/generator.py b/python/generator.py
index 6fee3a4..0311004 100755
--- a/python/generator.py
+++ b/python/generator.py
@@ -421,6 +421,7 @@ skip_impl = (
 'virDomainGetBlockIoTune',
 'virDomainSetInterfaceParameters',
 'virDomainGetInterfaceParameters',
+'virDomainGetPcpusUsage', # not implemented yet
 )
 
 qemu_skip_impl = (
diff --git a/src/driver.h b/src/driver.h
index 24636a4..2a3c46d 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -794,6 +794,12 @@ typedef int
   int *nparams,
   unsigned int flags);
 
+typedef int
+(*virDrvDomainGetPcpusUsage)(virDomainPtr dom,
+ unsigned long long *usages,
+ int *nr_usages,
+ unsigned int flags);
+
 /**
  * _virDriver:
  *
@@ -962,6 +968,7 @@ struct _virDriver {
 virDrvNodeSuspendForDuration nodeSuspendForDuration;
 virDrvDomainSetBlockIoTune domainSetBlockIoTune;
 virDrvDomainGetBlockIoTune domainGetBlockIoTune;
+virDrvDomainGetPcpusUsage domainGetPcpusUsage;
 };
 
 typedef int
diff --git a/src/libvirt.c b/src/libvirt.c
index a540424..bd19bf5 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -17882,3 +17882,54 @@ error:
 virDispatchError(dom-conn);
 return -1;
 }
+
+/**
+ * virDomainGetPcpusUsage:
+ * @dom: pointer to domain object
+ * @usages: returned physical cpu usages
+ * @nr_usages: length of @usages
+ * @flags: flags to control the operation
+ *
+ * Get the cpu usages for every physical cpu since the domain started (in 
nanoseconds).
+ *
+ * Returns 0 if success, -1 on error
+ */
+int virDomainGetPcpusUsage(virDomainPtr dom,
+   unsigned long long *usages,
+   int *nr_usages,
+   unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(dom, usages=%p, nr_usages=%d, flags=%x,
+ usages, (nr_usages) ? *nr_usages : -1, flags);
+
+virResetLastError();
+
+if (!VIR_IS_CONNECTED_DOMAIN (dom)) {
+virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+virDispatchError(NULL);
+return -1;
+}
+
+if (nr_usages == NULL  *nr_usages != 0) {
+virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+goto error;
+}
+
+conn = dom-conn;
+
+if (conn-driver-domainGetPcpusUsage) {
+int ret;
+ret = conn-driver-domainGetPcpusUsage(dom, usages, nr_usages, flags);
+if (ret  0)
+goto error;
+return ret;
+}
+
+virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+virDispatchError(dom-conn);
+return -1;
+}
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 4ca7216..15d944c 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -516,4 +516,9 @@ LIBVIRT_0.9.9 {
 virDomainSetNumaParameters;
 } LIBVIRT_0.9.8;
 
+LIBVIRT_0.9.10 {
+global:
+virDomainGetPcpusUsage;
+} LIBVIRT_0.9.9;
+
 #  define new API here using predicted next version number 
-- 
1.7.4.4

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