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 !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list