Re: [libvirt] [PATCHv2 1/6] virNodeGetCPUTime: Expose new API

On Sun, Apr 10, 2011 at 12:58:56PM +0800, Daniel Veillard wrote: > On Fri, Apr 08, 2011 at 08:33:12PM +0900, Minoru Usui wrote: > > virNodeGetCPUTime: Expose new API > > > > Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; > > --- > > include/libvirt/libvirt.h.in | 64 ++++++++++++++++++++++++++++++++++++++++++ > > src/libvirt_public.syms | 5 +++ > > 2 files changed, 69 insertions(+), 0 deletions(-) > > > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > > index bd36015..154c138 100644 > > --- a/include/libvirt/libvirt.h.in > > +++ b/include/libvirt/libvirt.h.in > > @@ -228,6 +228,57 @@ struct _virNodeInfo { > > unsigned int threads;/* number of threads per core */ > > }; > > > > +/** > > + * virNodeCpuTime: > > + * > > + * a virNodeCpuTime is a structure filled by virNodeGetCpuTime() and providing > > + * the information for the cpu time of the node. > > + */ > > + > > +/** > > + * Cpu Time Statistics Tags: > > + */ > > +typedef enum { > > + /* > > + * The cumulative CPU time which spends by kernel, > > + * when the node booting up.(in nanoseconds). > > + */ > > + VIR_NODE_CPU_TIME_KERNEL = 0, > > + /* > > + * The cumulative CPU time which spends by user processes, > > + * when the node booting up.(in nanoseconds). > > + */ > > + VIR_NODE_CPU_TIME_USER = 1, > > + /* > > + * The cumulative idle CPU time, > > + * when the node booting up.(in nanoseconds). > > + */ > > + VIR_NODE_CPU_TIME_IDLE = 2, > > + /* > > + * The cumulative I/O wait CPU time, > > + * when the node booting up.(in nanoseconds). > > + */ > > + VIR_NODE_CPU_TIME_IOWAIT = 3, > > + /* > > + * The CPU utilization. > > + * The usage value is in percent and 100% represents all CPUs on > > + * the server. > > + */ > > + VIR_NODE_CPU_TIME_UTILIZATION = 4, > > + > > + /* > > + * The number of statistics supported by this version of the interface. > > + * To add new statistics, add them to the enum and increase this value. > > + */ > > + VIR_NODE_CPU_TIME_NR = 5, > > +} virNodeCpuTimeTags; > > + > > +typedef struct _virNodeCpuTime virNodeCpuTime; > > + > > +struct _virNodeCpuTime { > > + virNodeCpuTimeTags tag; > > + unsigned long long val; > > +}; > > NACK, the size of an enum is not defined by the C language and > hence is compiler dependant. We cannot use it as a component of a > structure in the API. > > > /** > > * virDomainSchedParameterType: > > @@ -460,6 +511,14 @@ int virDomainMigrateSetMaxSpeed(virDomainPtr domain, > > typedef virNodeInfo *virNodeInfoPtr; > > > > /** > > + * virNodeCpuTimePtr: > > + * > > + * a virNodeCpuTimePtr is a pointer to a virNodeCpuTime structure. > > + */ > > + > > +typedef virNodeCpuTime *virNodeCpuTimePtr; > > + > > +/** > > * virConnectFlags > > * > > * Flags when opening a connection to a hypervisor > > @@ -593,6 +652,11 @@ int virNodeGetInfo (virConnectPtr conn, > > virNodeInfoPtr info); > > char * virConnectGetCapabilities (virConnectPtr conn); > > > > +int virNodeGetCpuTime (virConnectPtr conn, > > + virNodeCpuTimePtr stats, > > + unsigned int nr_stats, > > + unsigned int flags); > > + > > I don't understand how the API is suppoed to work. Suppose you want > the cumulative CPU time, how do you ask for it ? Seems you can't ! You > just ask for a big stucture hoping that on return you may find it within > the results. That looks broken to me. This is the same design we've used in the virDomainGetMemoryStats, virDomainGetBlkioParameters, virDomainGetSchedularParamters without any undue trouble.

> > Either you make a simple API giving back an unsigned long long and > you use the virNodeCpuTimeTags as the flag value > > int virNodeGetCpuTime(virConnectPtr conn, unsigned long long *time, > unsigned int flags); > > and the flags indicate the value you're interested into, but in that > case you have to ask multiple times. Or you make the VIR_NODE_CPU_TIME_* > values a bit field, and you pass an array > > int virNodeGetCpuTime(virConnectPtr conn, > unsigned long long *stats, > unsigned int nr_stats, > unsigned int flags); > > where flags is the logical or of the values you are interested into, and > the implementation put them in order based on the VIR_NODE_CPU_TIME_* > values. But in that case you must fail if one of the statistics is not > available. I think these are worse because you're relying on ordering of values, and not all hypervisors will be able to supply all values. In addition it makes this stats API completely different to the other stats APIs we have.

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