On Fri, 27 May 2011 22:33:00 +0800 Daniel Veillard <veillard@xxxxxxxxxx> 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ > http://veillard.com/ | virtualization library http://libvirt.org/ -- Minoru Usui <usui@xxxxxxxxxxxxxxxxx> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list