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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Mar 2020 00:10:50 +0000
From:   Song Liu <songliubraving@...com>
To:     Yonghong Song <yhs@...com>
CC:     Networking <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "arnaldo.melo@...il.com" <arnaldo.melo@...il.com>,
        "jolsa@...nel.org" <jolsa@...nel.org>
Subject: Re: [PATCH v2 bpf-next 1/2] bpftool: introduce "prog profile" command



> On Mar 1, 2020, at 8:24 PM, Yonghong Song <yhs@...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. 

> 
>> +
>> +u64 profile_total_count;
> 
[...]
>> +
>> +	reading_map_fd = bpf_map__fd(obj->maps.accum_readings);
>> +	count_map_fd = bpf_map__fd(obj->maps.counts);
>> +	if (reading_map_fd < 0 || count_map_fd < 0) {
>> +		p_err("failed to get fd for map");
>> +		return;
>> +	}
>> +
>> +	assert(bpf_map_lookup_elem(count_map_fd, &key, counts) == 0);
> 
> In the patch, I see sometime bpf_map_lookup_elem() result is checked
> with failure being handled. Sometimes, assert() is used. Maybe be
> consistent with checking result approach?

Will fix. 

[...]
> 
>> 
>> +}
>> +
>> +#define PROFILE_DEFAULT_LONG_DURATION (3600 * 24)
> 
> We need to let user know this value in "help" at least.
> In "man" page it may be get updated but I think we probably
> should add there as well.

I am planning to just use UINT_MAX. 

> 
>> +
[...]

>> +#define BPF_PROG(name, args...)						    \
>> +name(unsigned long long *ctx);						    \
>> +static __always_inline typeof(name(0))					    \
>> +____##name(unsigned long long *ctx, ##args);				    \
>> +typeof(name(0)) name(unsigned long long *ctx)				    \
>> +{									    \
>> +	_Pragma("GCC diagnostic push")					    \
>> +	_Pragma("GCC diagnostic ignored \"-Wint-conversion\"")		    \
>> +	return ____##name(___bpf_ctx_cast(args));			    \
>> +	_Pragma("GCC diagnostic pop")					    \
>> +}									    \
>> +static __always_inline typeof(name(0))					    \
>> +____##name(unsigned long long *ctx, ##args)
> 
> I know it is internal. But all the above macros are not great in
> a bpf program. If we can reuse/amend current infrastructure.
> That will be great. It may benefit users writing a similar
> bpf program to here.

This is copied from tools/testing/selftests/bpf/bpf_trace_helpers.h. 
I think we will move them to libbpf later. Then we can use that 
version instead. 

Thanks,
Song