Re: [PATCH v6 3/8] perf record --off-cpu: Parse offcpu-time event

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Sep 27, 2024 at 01:27:31PM -0700, Howard Chu wrote:
> Parse offcpu-time event using parse_event, in off_cpu_start(), write
> evlist fds got from evlist__open() to perf_event_array BPF map.
> 
> Signed-off-by: Howard Chu <howardchu95@gmail.com>
> ---
>  tools/perf/util/bpf_off_cpu.c          | 56 +++++++++++++++-----------
>  tools/perf/util/bpf_skel/off_cpu.bpf.c |  9 +++++
>  2 files changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/bpf_off_cpu.c b/tools/perf/util/bpf_off_cpu.c
> index eaef643f50e3..f7233a09ec77 100644
> --- a/tools/perf/util/bpf_off_cpu.c
> +++ b/tools/perf/util/bpf_off_cpu.c
> @@ -13,6 +13,7 @@
>  #include "util/cgroup.h"
>  #include "util/strlist.h"
>  #include <bpf/bpf.h>
> +#include <internal/xyarray.h>
>  
>  #include "bpf_skel/off_cpu.skel.h"
>  
> @@ -36,41 +37,27 @@ union off_cpu_data {
>  	u64 array[1024 / sizeof(u64)];
>  };
>  
> +u64 off_cpu_raw_data[1024 / sizeof(u64)];
> +
>  static int off_cpu_config(struct evlist *evlist)
>  {
> -	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;
> +	char off_cpu_event[64];
>  
> -	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);

Is the 'no-inherit=1' really needed?  I guess that's the default and
you won't need it if you open it as a per-cpu 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;
> -
>  	return 0;
>  }
>  
>  static void off_cpu_start(void *arg)
>  {
>  	struct evlist *evlist = arg;
> +	struct evsel *evsel;
> +	struct perf_cpu pcpu;
> +	int i, err;
>  
>  	/* update task filter for the given workload */
>  	if (skel->rodata->has_task && skel->rodata->uses_tgid &&
> @@ -84,6 +71,27 @@ static void off_cpu_start(void *arg)
>  		bpf_map_update_elem(fd, &pid, &val, BPF_ANY);
>  	}
>  
> +	/* sample id and fds in BPF's perf_event_array can only be set after record__open() */
> +	evsel = evlist__find_evsel_by_str(evlist, OFFCPU_EVENT);
> +	if (evsel == NULL) {
> +		pr_err("%s evsel not found\n", OFFCPU_EVENT);
> +		return;
> +	}
> +
> +	if (evsel->core.id)
> +		skel->bss->sample_id = evsel->core.id[0];

Where's it used?

> +
> +	perf_cpu_map__for_each_cpu(pcpu, i, evsel->core.cpus) {
> +		err = bpf_map__update_elem(skel->maps.offcpu_output,
> +					   &pcpu.cpu, sizeof(__u32),

I understand you used pcpu.cpu as a key to the offcpu_output map because
it's a basically per-cpu array.


> +					   xyarray__entry(evsel->core.fd, pcpu.cpu, 0),

But I suspect you should use 'i' instead of pcpu.cpu as an index to the
xyarray.

Thanks,
Namhyung


> +					   sizeof(__u32), BPF_ANY);
> +		if (err) {
> +			pr_err("Failed to update perf event map for direct off-cpu dumping\n");
> +			return;
> +		}
> +	}
> +
>  	skel->bss->enabled = 1;
>  }
>  
> diff --git a/tools/perf/util/bpf_skel/off_cpu.bpf.c b/tools/perf/util/bpf_skel/off_cpu.bpf.c
> index 5ea320aa9a53..e2a887228fd9 100644
> --- a/tools/perf/util/bpf_skel/off_cpu.bpf.c
> +++ b/tools/perf/util/bpf_skel/off_cpu.bpf.c
> @@ -18,6 +18,8 @@
>  #define MAX_STACKS   32
>  #define MAX_ENTRIES  102400
>  
> +#define MAX_CPUS  4096
> +
>  struct tstamp_data {
>  	__u32 stack_id;
>  	__u32 state;
> @@ -39,6 +41,13 @@ struct {
>  	__uint(max_entries, MAX_ENTRIES);
>  } stacks SEC(".maps");
>  
> +struct {
> +	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
> +	__uint(key_size, sizeof(__u32));
> +	__uint(value_size, sizeof(__u32));
> +	__uint(max_entries, MAX_CPUS);
> +} offcpu_output SEC(".maps");
> +
>  struct {
>  	__uint(type, BPF_MAP_TYPE_TASK_STORAGE);
>  	__uint(map_flags, BPF_F_NO_PREALLOC);
> -- 
> 2.43.0
>