[libvirt] [PATCHv6 0/6] Add virNodeGetCPUStats() API

[Minoru Usui]

On Fri, 27 May 2011 22:33:00 +0800
Daniel Veillard <veillard at redhat.com> wrote:

> On Fri, May 27, 2011 at 02:34:50PM +0100, Daniel P. Berrange wrote:
> > On Wed, May 25, 2011 at 03:01:29PM +0900, Minoru Usui wrote:
> > > Hi,
> > > 
> > > This is v6 of virNodeGetCPUTimeParameters() API. 
> > > (This time, I rename it to virNodeGetCPUStats())
> > > It returns cpu utilization or 
> > > cumulative cpu time of the node from /proc/stat since node boots up.
> > > This patch only supports linux host.
> > 
> > The code in this patch series looks fine to me now. I would
> > ACK the whole series, but I have one design question I should
> > have asked previously.
> > 
> > This API returns CPU information for the host as a whole.
> > Historically, when adding an API like this, someone has always
> > then asked for a per-CPU breakdown of the same data.
> > 
> > So I wonder if we should add a parameter for the CPU num
> > to the API:
> > 
> >  int                     virNodeGetCPUStats (virConnectPtr conn,
> >                                              int cpuNum,
> >                                              virCPUStatsPtr params,
> >                                              int *nparams,
> >                                              unsigned int flags);
> > 
> > And have a constant for getting the host data as a whole:
> > 
> >    #define  VIR_NODE_CPU_STATS_ALL_CPUS -1
> > 
> > so if you want to get individual data for each CPU you
> > can do
> > 
> >    virNodeGetCPUStats(conn, 0, params, nparams, flags);
> >    virNodeGetCPUStats(conn, 1, params, nparams, flags);
> >    virNodeGetCPUStats(conn, 2, params, nparams, flags);
> >    virNodeGetCPUStats(conn, 3, params, nparams, flags);
> > 
> > But if you just want the data for the host as a whole
> > you can do
> > 
> >    virNodeGetCPUStats(conn, VIR_NODE_CPU_STATS_ALL_CPUS,
> >                       params, nparams, flags);
> > 
> > The /proc/stat file already has the data we need for both
> > these styles
> > 
> > $ grep cpu /proc/stat 
> > cpu  2632143 17400 1341532 32819705 1177560 674 21606 0 11997 0
> > cpu0 1263069 9280 596278 16274365 620428 631 17303 0 6131 0
> > cpu1 1369073 8119 745254 16545339 557131 43 4302 0 5865 0
> 
>   Agreed, similary as for the virNodeGetMemoryStats() patch set, the
> patches looks good enough for being applied but doing the suggested
> changes (i.e. adding a new argument for the cpu number or the cell
> number).
>   For virsh command I would make querying all the default but just
> add an optionnal argument --cpu
>     virsh nodecpustats --cpu cpunumber
> and similary on the memry patch set add an optional --cell argument
>     virsh nodememstats --cell cellnumber
> 
>  That way the API and virsh command are ready for more precise use
> and we won't need to make a second set of APIs.
> 
>   thanks !

OK.
I'll add cpu/cell argments for more generalization.

> 
> Daniel
> -- 
> Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library  http://libvirt.org/


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