Re: [PATCH 2/2] perf_counter: powerpc: Implement generalized cache events for POWER processors

[Ingo Molnar <mingo@elte.hu>]

* Paul Mackerras <paulus@samba.org> wrote:

> This adds tables of event codes for the generalized cache events 
> for all the currently supported powerpc processors: 
> POWER{4,5,5+,6,7} and PPC970*, plus powerpc-specific code to use 
> these tables when a generalized cache event is requested.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
>  arch/powerpc/include/asm/perf_counter.h |    3 ++
>  arch/powerpc/kernel/perf_counter.c      |   42 +++++++++++++++++++++++++++-
>  arch/powerpc/kernel/power4-pmu.c        |   41 +++++++++++++++++++++++++++
>  arch/powerpc/kernel/power5+-pmu.c       |   45 +++++++++++++++++++++++++++++-
>  arch/powerpc/kernel/power5-pmu.c        |   41 +++++++++++++++++++++++++++
>  arch/powerpc/kernel/power6-pmu.c        |   46 +++++++++++++++++++++++++++++-
>  arch/powerpc/kernel/power7-pmu.c        |   41 +++++++++++++++++++++++++++
>  arch/powerpc/kernel/ppc970-pmu.c        |   41 +++++++++++++++++++++++++++
>  8 files changed, 294 insertions(+), 6 deletions(-)

Ah, cool! I tried to construct the table so that Power would be able 
to fill it in a meaningful way - it seems like that was indeed 
possible.

Any particular observations you have about the cache events 
generalization? Would you do more of them (which ones?), fewer of 
them?

We can also add transparent fallback logic to the tools perhaps: for 
example a 'hits == total-misses' combo counter.

This can be expressed in the sampling space too: the latest tools do 
weighted samples, so we can actually do _negative_, weighted 
sampling: the misses can subtract from a function's ->count value.

I dont know whether we should do such combo counters in the kernel 
itself - i'm slightly against that notion. (seems complex)

One last-minute change we are thinking about is to change 'L2' to 
'LLC'. This matters on systems which have a L3 cache. The first 
level and the last level cache are generally the most important 
ones. What do you think?

> +	[C(BPU)] = {		/* 	RESULT_ACCESS	RESULT_MISS */
> +		[C(OP_READ)] = {	0x430e6,	0x400052	},
> +		[C(OP_WRITE)] = {	-1,		-1		},
> +		[C(OP_PREFETCH)] = {	-1,		-1		},

Ah, the RESULT_ACCESS/RESULT_MISS tabularization is a nice aesthetic 
touch - will do that for x86 too.

> @@ -483,8 +524,9 @@ struct power_pmu power6_pmu = {
>  	.get_constraint = p6_get_constraint,
>  	.get_alternatives = p6_get_alternatives,
>  	.disable_pmc = p6_disable_pmc,
> +	.limited_pmc_event = p6_limited_pmc_event,
> +	.flags = PPMU_LIMITED_PMC5_6 | PPMU_ALT_SIPR,
>  	.n_generic = ARRAY_SIZE(power6_generic_events),
>  	.generic_events = power6_generic_events,
> -	.flags = PPMU_LIMITED_PMC5_6 | PPMU_ALT_SIPR,
> -	.limited_pmc_event = p6_limited_pmc_event,
> +	.cache_events = &power6_cache_events,

Btw., a very small nit, any way i could convince you to do such 
mass-initializations in the Power code, in the way we do elsewhere 
in perfcounters, by using vertical spacing:

 	.get_constraint		= p6_get_constraint,
 	.get_alternatives	= p6_get_alternatives,
 	.disable_pmc		= p6_disable_pmc,
	.limited_pmc_event	= p6_limited_pmc_event,
	.flags			= PPMU_LIMITED_PMC5_6 | PPMU_ALT_SIPR,
  	.n_generic		= ARRAY_SIZE(power6_generic_events),
  	.generic_events		= power6_generic_events,
	.cache_events		= &power6_cache_events,

IMHO that form is infinitely more readable.

Thanks,

	Ingo