On Tuesday 04 November 2008 06:33:39 Ehrhardt Christian wrote:
> From: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx>
>
> Other existing kvm stats 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.
It's not just that kvm_trace is "slow" -- I think the important point is that this code is *not* implementing another
trace mechanism. It's simply a set of counters. The reason it doesn't fit naturally into kvm_stat is that
kvm_stat is designed just to record event frequency, so it aggregates the counters from all vcpus. Right?
> 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).
In general this looks pretty good. I still need to go through it in more detail, but just a couple superficial
comments right now:
> diff --git a/arch/powerpc/include/asm/mmu-44x.h b/arch/powerpc/include/asm/mmu-44x.h
> --- a/arch/powerpc/include/asm/mmu-44x.h
> +++ b/arch/powerpc/include/asm/mmu-44x.h
> @@ -56,6 +56,7 @@
> #ifndef __ASSEMBLY__
>
> extern unsigned int tlb_44x_hwater;
> +extern unsigned int tlb_44x_index;
>
> typedef struct {
> unsigned long id;
This must have accidentally slipped in to your patch.
> @@ -108,6 +258,10 @@
> kvm = kzalloc(sizeof(struct kvm), GFP_KERNEL);
> if (!kvm)
> return ERR_PTR(-ENOMEM);
> +
> +#ifdef CONFIG_KVM_BOOKE_EXIT_TIMING
> + kvm->arch.vm_id = atomic_inc_return(&vm_count);
> +#endif
>
> return kvm;
> }
This "vm_count" stuff caught my eye as looking strange. Is there no better "ID" value you can use? What
about qemu's PID?
--
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
