linux-kernel - Re: [PATCH v8 03/10] perf record --off-cpu: Parse off-cpu event

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]

Message-ID: <Zzuu06ybvy8IpH5m@google.com>
Date: Mon, 18 Nov 2024 13:17:07 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Howard Chu <howardchu95@...il.com>
Cc: acme@...nel.org, peterz@...radead.org, irogers@...gle.com,
	mingo@...hat.com, jolsa@...nel.org, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	James Clark <james.clark@...aro.org>,
	Mark Rutland <mark.rutland@....com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH v8 03/10] perf record --off-cpu: Parse off-cpu event

On Tue, Nov 12, 2024 at 04:28:11PM -0800, Howard Chu wrote:
> Parse the off-cpu event using parse_event(), as bpf-output.
> 
> no-inherit should be set to 1, here's the reason:
> 
> We update the BPF perf_event map for direct off-cpu sample dumping (in
> following patches), it executes as follows:
> 
> bpf_map_update_value()
>  bpf_fd_array_map_update_elem()
>   perf_event_fd_array_get_ptr()
>    perf_event_read_local()
> 
> In perf_event_read_local(), there is:
> 
> int perf_event_read_local(struct perf_event *event, u64 *value,
> 			  u64 *enabled, u64 *running)
> {
> ...
> 	/*
> 	 * It must not be an event with inherit set, we cannot read
> 	 * all child counters from atomic context.
> 	 */
> 	if (event->attr.inherit) {
> 		ret = -EOPNOTSUPP;
> 		goto out;
> 	}
> 
> Which means no-inherit has to be true for updating the BPF perf_event
> map.
> 
> Moreover, for bpf-output events, we primarily want a system-wide event
> instead of a per-task event.
> 
> The reason is that in BPF's bpf_perf_event_output(), BPF uses the CPU
> index to retrieve the perf_event file descriptor it outputs to.
> 
> Making a bpf-output event system-wide naturally satisfies this
> requirement by mapping CPU appropriately.

I'm afraid the inherit attribute would be updated later:

  __cmd_record()
    evlist__config()
      evsel__config()

You can add a logic to check the config term when setting the inherit
value.

Thanks,
Namhyung

> 
> Suggested-by: Namhyung Kim <namhyung@...nel.org>
> Reviewed-by: Ian Rogers <irogers@...gle.com>
> Signed-off-by: Howard Chu <howardchu95@...il.com>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: James Clark <james.clark@...aro.org>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Kan Liang <kan.liang@...ux.intel.com>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Link: https://lore.kernel.org/r/20241108204137.2444151-4-howardchu95@gmail.com
> Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> ---
>  tools/perf/util/bpf_off_cpu.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/util/bpf_off_cpu.c b/tools/perf/util/bpf_off_cpu.c
> index a590a8ac1f9d..558c5e5c2dc3 100644
> --- a/tools/perf/util/bpf_off_cpu.c
> +++ b/tools/perf/util/bpf_off_cpu.c
> @@ -38,32 +38,21 @@ union off_cpu_data {
>  
>  static int off_cpu_config(struct evlist *evlist)
>  {
> +	char off_cpu_event[64];
>  	struct evsel *evsel;
> -	struct perf_event_attr attr = {
> -		.type	= PERF_TYPE_SOFTWARE,
> -		.config = PERF_COUNT_SW_BPF_OUTPUT,
> -		.size	= sizeof(attr), /* to capture ABI version */
> -	};
> -	char *evname = strdup(OFFCPU_EVENT);
> -
> -	if (evname == NULL)
> -		return -ENOMEM;
>  
> -	evsel = evsel__new(&attr);
> -	if (!evsel) {
> -		free(evname);
> -		return -ENOMEM;
> +	scnprintf(off_cpu_event, sizeof(off_cpu_event), "bpf-output/no-inherit=1,name=%s/", OFFCPU_EVENT);
> +	if (parse_event(evlist, off_cpu_event)) {
> +		pr_err("Failed to open off-cpu event\n");
> +		return -1;
>  	}
>  
> -	evsel->core.attr.freq = 1;
> -	evsel->core.attr.sample_period = 1;
> -	/* off-cpu analysis depends on stack trace */
> -	evsel->core.attr.sample_type = PERF_SAMPLE_CALLCHAIN;
> -
> -	evlist__add(evlist, evsel);
> -
> -	free(evsel->name);
> -	evsel->name = evname;
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (evsel__is_offcpu_event(evsel)) {
> +			evsel->core.system_wide = true;
> +			break;
> +		}
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.43.0
>