Re: [PATCH v2 bpf-next 1/2] bpftool: introduce "prog profile" command

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Tue, Mar 03, 2020 at 12:10:50AM +0000, Song Liu escreveu:
> > On Mar 1, 2020, at 8:24 PM, Yonghong Song <yhs@fb.com> wrote:
> >> +	},
> >> +	{
> >> +		.name = "instructions",
> >> +		.attr = {
> >> +			.freq = 0,
> >> +			.sample_period = SAMPLE_PERIOD,
> >> +			.inherit = 0,
> >> +			.type = PERF_TYPE_HARDWARE,
> >> +			.read_format = 0,
> >> +			.sample_type = 0,
> >> +			.config = PERF_COUNT_HW_INSTRUCTIONS,
> >> +		},
> >> +		.ratio_metric = 1,
> >> +		.ratio_mul = 1.0,
> >> +		.ratio_desc = "insn per cycle",
> >> +	},
> >> +	{
> >> +		.name = "l1d_loads",
> >> +		.attr = {
> >> +			.freq = 0,
> >> +			.sample_period = SAMPLE_PERIOD,
> >> +			.inherit = 0,
> >> +			.type = PERF_TYPE_HW_CACHE,
> >> +			.read_format = 0,
> >> +			.sample_type = 0,
> >> +			.config =
> >> +				PERF_COUNT_HW_CACHE_L1D |
> >> +				(PERF_COUNT_HW_CACHE_OP_READ << 8) |
> >> +				(PERF_COUNT_HW_CACHE_RESULT_ACCESS << 16),
> >> +		},
> > 
> > why we do not have metric here?
> 
> This follows perf-stat design: some events have another event to compare 
> against, like instructions per cycle, etc. 
> 
> > 
> >> +	},
> >> +	{
> >> +		.name = "llc_misses",
> >> +		.attr = {
> >> +			.freq = 0,
> >> +			.sample_period = SAMPLE_PERIOD,
> >> +			.inherit = 0,
> >> +			.type = PERF_TYPE_HW_CACHE,
> >> +			.read_format = 0,
> >> +			.sample_type = 0,
> >> +			.config =
> >> +				PERF_COUNT_HW_CACHE_LL |
> >> +				(PERF_COUNT_HW_CACHE_OP_READ << 8) |
> >> +				(PERF_COUNT_HW_CACHE_RESULT_MISS << 16),
> >> +		},
> >> +		.ratio_metric = 2,
> >> +		.ratio_mul = 1e6,
> >> +		.ratio_desc = "LLC misses per million isns",
> >> +	},
> >> +};

> > icache miss and itlb miss might be useful as well as the code will jump
> > to a different physical page. I think we should addd them. dtlb_miss
> > probably not a big problem, but it would be good to be an option.

> I plan to add more events later on. 

> > For ratio_metric, we explicitly assign a slot here. Any specific reason?
> > We can just say this metric *permits* ratio_metric and then ratio_matric
> > is assigned dynamically by the user command line options?

> > I am thinking how we could support *all* metrics the underlying system
> > support based on `perf list`. This can be the future work though.
 
> We are also thinking about adding similar functionality to perf-stat, 
> which will be more flexible. 

Yeah, being able to count events bpf programs using the technique you're
using here but instead using 'perf stat' to set it up and then use what
is already in 'perf stat' would be really great, having the same
interface for BPF programs as we have for tid, pid, cgroups, system
wide, etc.

If you try it and find any problems with the codebase I'll be happy to
help as I think others working with 'perf stat' will too,

Cheers,

- Arnaldo