Re: [PATCH bpf-next] bpftool: add {i,d}tlb_misses support for bpftool profile

[Yonghong Song <yhs@fb.com>]

On 11/18/20 9:03 PM, Song Liu wrote:
> 
>> On Nov 18, 2020, at 8:23 PM, Yonghong Song <yhs@fb.com> wrote:
> 
> [...]
> 
>>
>> Cc: Song Liu <songliubraving@fb.com>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> tools/bpf/bpftool/prog.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>> index acdb2c245f0a..e33f27b950a5 100644
>> --- a/tools/bpf/bpftool/prog.c
>> +++ b/tools/bpf/bpftool/prog.c
>> @@ -1717,11 +1717,39 @@ struct profile_metric {
>> 		.ratio_desc = "LLC misses per million insns",
>> 		.ratio_mul = 1e6,
>> 	},
>> +	{
>> +		.name = "itlb_misses",
>> +		.attr = {
>> +			.type = PERF_TYPE_HW_CACHE,
>> +			.config =
>> +				PERF_COUNT_HW_CACHE_ITLB |
>> +				(PERF_COUNT_HW_CACHE_OP_READ << 8) |
>> +				(PERF_COUNT_HW_CACHE_RESULT_MISS << 16),
>> +			.exclude_user = 1
>> +		},
>> +		.ratio_metric = 2,
>> +		.ratio_desc = "itlb misses per million insns",
>> +		.ratio_mul = 1e6,
>> +	},
>> +	{
>> +		.name = "dtlb_misses",
>> +		.attr = {
>> +			.type = PERF_TYPE_HW_CACHE,
>> +			.config =
>> +				PERF_COUNT_HW_CACHE_DTLB |
>> +				(PERF_COUNT_HW_CACHE_OP_READ << 8) |
>> +				(PERF_COUNT_HW_CACHE_RESULT_MISS << 16),
>> +			.exclude_user = 1
>> +		},
>> +		.ratio_metric = 2,
>> +		.ratio_desc = "dtlb misses per million insns",
>> +		.ratio_mul = 1e6,
>> +	},
>> };
>>
>> static __u64 profile_total_count;
>>
>> -#define MAX_NUM_PROFILE_METRICS 4
>> +#define MAX_NUM_PROFILE_METRICS 6
> 
> This change is not necessary. This is the max number of enabled metrics.
> We don't stop the perf counters, each enabled metric adds some error to
> the final reading. Therefore, we don't want to enable too many metrics
> in the same run. If we don't have a use case for more metrics in one run,
> let's keep the cap of 4 metrics.
> 
> If we do want to increase this, we should also increase MAX_NUM_MATRICS
> in profiler.bpf.c.

Thanks for explanation. I missed that we also need to change 
MAX_NUM_MATRICS to really make it work. Ideally it would be best if we
only need to change in one header and that header is shared by both
bpf program and non-bpf program but probably is a overkill here just
for a single macro.

Also, your concern makes sense. We do not want to enable too many
metrics as this will cause counter sharing and may sacrifice accuracy.
I will leave current max profile metrics 4 then.

Will respin v2.

> 
> Other than this,
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> 
> Thanks!
>