Re: [PATCH] kvm: powerpc: add exit timing statistics v4

[Hollis Blanchard <hollisb@us.ibm.com>]

On Tue, 2008-11-11 at 16:43 +0100, Christian Ehrhardt wrote:
> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
> 
> *update to v4*
> - EMUL_CORE no longer had more than wrtee emulation, therefore it now accounts
>   for WRTEE in the output and set_exit_type calls are in the wrtee handlers to
>   let any residual core op be counted as EMULINST"
> 
> *update to v3*
> - ensure build time optimization when calling exit accouting functions using
>   build time bug / constant check
> - migrate most of the exit timing code from powerpc.c and
>   kvm_timing_stats.h to a separate exittiming.c file
> - renamed a lot of constants used to better fit generic/core specific code
> - added an accidenially removed optimization comment
> - use pid of userspace process instead of an own atomic count to identify a vm
> - changed loop labels in tul/tbu loops (booke_interrupt.S) to anonymous 1/1b
> - removed the indirection of additional EMULATE_*_DONE types. Instead now
>   the exit timing supports "accounting" an exit which consists of set type and
>   increase kvm_stat counters. But also provides both sub-tasks as separate
>   functions. Using that emulation now sets a default EMUL_INST type that can
>   be overwritten by detailed subcategories (set_exit_type). Accouting for
>   kvm_stat is done with account_exit_stat for all emulinst exits together on
>   top level (as it was before).
> 
> *update to v2*
> The update fixes accounting for sets to MSR[WE] which should not be accoutned
> as instruction emulation. While adding that and analyzing the data it became
> obvious that several types of emulations hould be accounted separately.
> I'm not yet really happy with adding all these EMULATE_*_DONE types but I had
> no better idea to not break flood the code with account calls and split
> account/set_type. The issue is that emulation is also called e.g. for some mmio
> paths and therefore the accouting should not be called inside emulation, but on
> the returning path that can now evaluate the different kind of emulations done.
> This is not yet finished, and posted as RFC only.
> 
> Other existing kvm statistics are either just counters (kvm_stat) reported for
> kvm generally or trace based aproaches like kvm_trace.
> For kvm on powerpc we had the need to track the timings of the different exit
> types. While this could be achieved parsing data created with a kvm_trace
> extension this adds too muhc overhead (at least on embedded powerpc) slowing
> down the workloads we wanted to measure.
> 
> Therefore this patch adds a in kernel exit timing statistic to the powerpc kvm
> code. These statistic is available per vm&vcpu under the kvm debugfs directory.
> As this statistic is low, but still some overhead it can be enabled via a
> .config entry and should be off by default.
> 
> Since this patch touched all powerpc kvm_stat code anyway this code is now
> merged and simpliefied together with the exit timing statistic code (still
> working with exit timing disabled in .config).

You forgot to include exittiming.c in this patch. Since I can't build
it, I might as well tell you the additional changes I was going to
make... :)

      * Prefix all the functions in kvm_timing_stats.h to begin with
        "kvmppc_". (I really like to keep the layering as clear as
        possible.) I don't think we need to add "booke" in there, since
        a hypothetical kvmppc 970 implementation could use the same
        function names, just a different set of exit types. 
      * Rename kvm_timing_stats.h. Does that need to go in the asm
        directory btw? If so, call it kvm_timing.h; if not, please put
        it inside arch/powerpc/kvm, and name it timing.h. 
      * Rename exittiming.c to match whatever you name the header. 
      * Use empty static functions instead of empty macros in
        kvm_timing.h. (I'm not the only one who doesn't like macros; see
        http://lwn.net/Articles/306045/) 
      * I know it's just leftovers from previous iterations of this
        patch, but drop the whitespace changes to include/asm/kvm_ppc.h
        (and send as a separate patch if you like). 
      * Remove vm_id from kvm_arch (you just missed this one spot :). 
      * I don't like the debugfs file names you've chosen, but I'm not
        sure what's best. Definitely make them lowercase and omit the
        underscore, but the directory layout feels a little odd to me.
        Maybe it should be /sys/kernel/debug/kvm/exit_times/vm52/vcpu0/
        instead of /sys/kernel/debug/kvm/vm52/vcpu0/exit_times ? 
      * Remove atomic.h and seq_file.h from arch/powerpc/kvm/powerpc.c 
      * Slight edits for Kconfig:

config KVM_EXIT_TIMING
	bool "Detailed exit timing"
	depends on KVM
	---help---
	  Calculate elapsed time for every exit/enter cycle. A per-vcpu
	  report is available in debugfs kvm/vm###/vcpu###_exit_timing.
	  The overhead is relatively small, however it is not recommended for
	  production environments.

	  If unsure, say N.

A lot of the naming stuff is just personal preference, but in general I
feel like the names you chose are too verbose and inconsistent. I've
found that name changes are easiest to do in the unapplied patch itself
with global find/replace. 

I'm happy with the rest of what I can see. :) If I have comments after
you've left for vacation, I'll apply those on top as a separate patch.
This patch is really important, and I feel like we're almost there...

Thanks very much for http://kvm.qumranet.com/kvmwiki/PowerPC_Exittimings
too; that's critical data. 

-- 
Hollis Blanchard
IBM Linux Technology Center