Re: [PATCH -tip] perf_counter tools: shorten names for events

[Ingo Molnar <mingo@elte.hu>]

* Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:

> After :
> 
>  Performance counter stats for 'ls -lR /usr/include/':
> 
>       283542921  dL1-loads             (scaled from 23.28%)
>         1848314  dL1-load-misses       (scaled from 22.94%)
>          168963  dL1-stores            (scaled from 22.94%)
>          739249  dL1-prefetches        (scaled from 22.45%)
>          501021  dL1-prefetch-misses   (scaled from 22.25%)
>       275037259  iL1-loads             (scaled from 23.40%)
>         6030825  iL1-load-misses       (scaled from 23.26%)
>          166760  iL1-prefetches        (scaled from 24.31%)
>         7224781  LLC-loads             (scaled from 24.76%)
>          821097  LLC-load-misses       (scaled from 24.07%)
>         7070549  LLC-stores            (scaled from 24.45%)
>       251586242  dTLB-loads            (scaled from 24.65%)
>         5127780  dTLB-load-misses      (scaled from 23.99%)
>       276782014  iTLB-loads            (scaled from 23.77%)
>           16787  iTLB-load-misses      (scaled from 23.72%)
>       123408502  branches              (scaled from 22.88%)
>         5843856  branch-misses         (scaled from 22.87%)
> 
>     1.417039891  seconds time elapsed.

ok, this output looks pretty good and intuitive to me (please 
integrate suggestions from Thomas), but the patch itself needs 
another iteration i think:

>  static char *hw_cache[][MAX_ALIASES] = {
> -	{ "L1-data",		"l1-d",		"l1d"			},
> -	{ "L1-instruction",	"l1-i",		"l1i"			},
> -	{ "L2",			"l2"					},
> -	{ "Data-TLB",		"dtlb",		"d-tlb"			},
> -	{ "Instruction-TLB",	"itlb",		"i-tlb"			},
> -	{ "Branch",		"bpu" ,		"btb",		"bpc"	},
> + { "dL1",	"L1-d",		"l1d",					},
> + { "iL1",	"L1-i",		"l1i",					},
> + { "LLC",	"L2",							},
> + { "dTLB",	"d-tlb",						},
> + { "iTLB",	"i-tlb",						},
> + { "branch",	"branches",	"bpu",		"btb",		"bpc",	},
>  };
>  
>  static char *hw_cache_op[][MAX_ALIASES] = {
> -	{ "Load",		"read"					},
> -	{ "Store",		"write"					},
> -	{ "Prefetch",		"speculative-read", "speculative-load"	},
> + { "load",	"loads",	"read",					},
> + { "store",	"stores",	"write",				},
> + { "prefetch",	"prefetches",	"speculative-read", "speculative-load",	},
>  };
>  
>  static char *hw_cache_result[][MAX_ALIASES] = {
> -	{ "Reference",		"ops",		"access"		},
> -	{ "Miss"							},
> + { "refs",	"ops",		"access",				},
> + { "misses",	"miss",							},
>  };
>  
>  char *event_name(int counter)
> @@ -123,10 +123,25 @@ char *event_name(int counter)
>  		if (cache_result > PERF_COUNT_HW_CACHE_RESULT_MAX)
>  			return "unknown-ext-hardware-cache-result";
>  
> -		sprintf(name, "%s-Cache-%s-%ses",
> -			hw_cache[cache_type][0],
> -			hw_cache_op[cache_op][0],
> -			hw_cache_result[cache_result][0]);
> +		/*
> +		 * special handling for branches
> +		 * we are only interested in BPU, READ
> +		 */ 
> +		if (cache_type == PERF_COUNT_HW_CACHE_BPU && cache_op)
> +			return "unknown";
> +		else if (cache_type == PERF_COUNT_HW_CACHE_BPU) {
> +			if (cache_result)
> +				sprintf(name, "%s-%s", hw_cache[cache_type][0],
> +					hw_cache_result[cache_result][0]);
> +			else
> +				sprintf(name, "%s", hw_cache[cache_type][1]);
> +		} else if (cache_result)
> +			sprintf(name, "%s-%s-%s", hw_cache[cache_type][0],
> +				hw_cache_op[cache_op][0],
> +				hw_cache_result[cache_result][0]);
> +		else
> +			sprintf(name, "%s-%s", hw_cache[cache_type][0],
> +				hw_cache_op[cache_op][1]);
>  
>  		return name;

Firstly, please run your patches through checkpatch - it will report 
a real problem in your patch.

Secondly, this special-casing of the BPU isnt very clean in this 
form. The BPU isnt 'special' because it deals with instructions - 
it's special because it's for all practical purposes read-only.

So we should extend our table with a read-only flag, and the BPU and 
the iTLB should be listed as read-only. (iTLB-store-miss is another 
thing that makes no sense) For those we should skip the 'store' 
bits.

That way the generic code does not have this special-case wart 
dependent on PERF_COUNT_HW_CACHE_BPU.

	Ingo