On Tue, 2008-11-11 at 16:43 +0100, Christian Ehrhardt wrote: > From: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx> > > *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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html