Received-SPF: pass (google.com: domain of linux-kernel+bounces-162861-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 29 Apr 2024 15:53:08 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Howard Chu <howardchu95@gmail.com>
Cc: peterz@infradead.org, mingo@redhat.com, namhyung@kernel.org,
	mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
	jolsa@kernel.org, irogers@google.com, adrian.hunter@intel.com,
	kan.liang@linux.intel.com, zegao2021@gmail.com, leo.yan@linux.dev,
	ravi.bangoria@amd.com, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH v1 1/4] perf record: Dump off-cpu samples directly
Message-ID: <Zi_slOlsZBjTbNIH@x1>
References: <20240422083645.1930939-1-howardchu95@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240422083645.1930939-1-howardchu95@gmail.com>

On Mon, Apr 22, 2024 at 04:36:45PM +0800, Howard Chu wrote:
> Parse off-cpu events using parse_event(). Change the placement of
> record__config_off_cpu to after record__open because we need to write
> mmapped fds into BPF's perf_event_array map, also, write 
> sample_id/sample_type into BPF. In record__pushfn and record__aio_pushfn, 
> handle off-cpu samples using off_cpu_strip. This is because the off-cpu 
> samples that we want to write to perf.data is in off-cpu samples' raw_data 
> section:

Hey,

	This lacks a cover letter and the chainig of patches so that b4
can fetch the series.

	Also all 5 patches have the same summary and different
descriptions and contents, can you please rework the patch series, using
'git format-patch', and make the description reflect what each patch is
doing?

Thanks,

- Arnaldo
 
> regular samples:
> [sample: sample_data]
> 
> off-cpu samples:
> [sample: [raw_data: sample_data]]
> 
> We need to extract the real useful sample data out before writing.
> 
> Hooks record_done just before evlist__disable to stop BPF program from
> outputting, otherwise, we lose some samples.
> 
> After samples are collected, change sample_type of off-cpu event to
> the OFFCPU_SAMPLE_TYPES for parsing correctly, it was PERF_SAMPLE_RAW and
> some others, because BPF can only output to a specific type of perf_event,
> which is why `evsel->core.attr.sample_type &= OFFCPU_SAMPLE_TYPES;` is
> deleted in util/evsel.c. 
> 
> Signed-off-by: Howard Chu <howardchu95@gmail.com>
> ---
>  tools/perf/builtin-record.c | 98 ++++++++++++++++++++++++++++++++++---
>  tools/perf/util/evsel.c     |  8 ---
>  2 files changed, 91 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 2ff718d3e202..c31b23905f1b 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -389,6 +389,8 @@ struct record_aio {
>  static int record__aio_pushfn(struct mmap *map, void *to, void *buf, size_t size)
>  {
>  	struct record_aio *aio = to;
> +	char *bf_stripped = NULL;
> +	size_t stripped;
>  
>  	/*
>  	 * map->core.base data pointed by buf is copied into free map->aio.data[] buffer
> @@ -404,6 +406,31 @@ static int record__aio_pushfn(struct mmap *map, void *to, void *buf, size_t size
>  	 * from the beginning of the kernel buffer till the end of the data chunk.
>  	 */
>  
> +	if (aio->rec->off_cpu) {
> +		if (size == 0)
> +			return 0;
> +
> +		map->core.start -= size;
> +		size = map->core.end - map->core.start;
> +
> +		bf_stripped = malloc(size);
> +
> +		if (bf_stripped == NULL) {
> +			pr_err("Failed to allocate off-cpu strip buffer\n");
> +			return -ENOMEM;
> +		}
> +
> +		stripped = off_cpu_strip(aio->rec->evlist, map, bf_stripped, size);
> +
> +		if (stripped < 0) {
> +			size = (int)stripped;
> +			goto out;
> +		}
> +
> +		size = stripped;
> +		buf = bf_stripped;
> +	}
> +
>  	if (record__comp_enabled(aio->rec)) {
>  		ssize_t compressed = zstd_compress(aio->rec->session, NULL, aio->data + aio->size,
>  						   mmap__mmap_len(map) - aio->size,
> @@ -432,6 +459,9 @@ static int record__aio_pushfn(struct mmap *map, void *to, void *buf, size_t size
>  
>  	aio->size += size;
>  
> +out:
> +	free(bf_stripped);
> +
>  	return size;
>  }
>  
> @@ -635,6 +665,38 @@ static int process_locked_synthesized_event(struct perf_tool *tool,
>  static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size)
>  {
>  	struct record *rec = to;
> +	int err;
> +	char *bf_stripped = NULL;
> +	size_t stripped;
> +
> +	if (rec->off_cpu) {
> +		/*
> +		 * We'll read all the events at once without masking.
> +		 * When reading the remainder from a map, the size is 0, because
> +		 * start is shifted to the end so no more data is to be read.
> +		 */
> +		if (size == 0)
> +			return 0;
> +
> +		map->core.start -= size;
> +		/* get the total size */
> +		size = map->core.end - map->core.start;
> +
> +		bf_stripped = malloc(size);
> +
> +		if (bf_stripped == NULL) {
> +			pr_err("Failed to allocate off-cpu strip buffer\n");
> +			return -ENOMEM;
> +		}
> +
> +		stripped = off_cpu_strip(rec->evlist, map, bf_stripped, size);
> +
> +		if (stripped < 0)
> +			return (int)stripped;
> +
> +		size = stripped;
> +		bf = bf_stripped;
> +	}
>  
>  	if (record__comp_enabled(rec)) {
>  		ssize_t compressed = zstd_compress(rec->session, map, map->data,
> @@ -648,7 +710,11 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size)
>  	}
>  
>  	thread->samples++;
> -	return record__write(rec, map, bf, size);
> +	err = record__write(rec, map, bf, size);
> +
> +	free(bf_stripped);
> +
> +	return err;
>  }
>  
>  static volatile sig_atomic_t signr = -1;
> @@ -1790,6 +1856,7 @@ record__finish_output(struct record *rec)
>  		if (rec->buildid_all)
>  			perf_session__dsos_hit_all(rec->session);
>  	}
> +
>  	perf_session__write_header(rec->session, rec->evlist, fd, true);
>  
>  	return;
> @@ -2501,6 +2568,14 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  		}
>  	}
>  
> +	if (rec->off_cpu) {
> +		err = record__config_off_cpu(rec);
> +		if (err) {
> +			pr_err("record__config_off_cpu failed, error %d\n", err);
> +			goto out_free_threads;
> +		}
> +	}
> +
>  	/*
>  	 * Normally perf_session__new would do this, but it doesn't have the
>  	 * evlist.
> @@ -2764,6 +2839,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  		 * disable events in this case.
>  		 */
>  		if (done && !disabled && !target__none(&opts->target)) {
> +			perf_hooks__invoke_record_done();
>  			trigger_off(&auxtrace_snapshot_trigger);
>  			evlist__disable(rec->evlist);
>  			disabled = true;
> @@ -2827,14 +2903,17 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  	} else
>  		status = err;
>  
> -	if (rec->off_cpu)
> -		rec->bytes_written += off_cpu_write(rec->session);
> -
>  	record__read_lost_samples(rec);
>  	record__synthesize(rec, true);
>  	/* this will be recalculated during process_buildids() */
>  	rec->samples = 0;
>  
> +	/* change to the correct sample type for parsing */
> +	if (rec->off_cpu && off_cpu_change_type(rec->evlist)) {
> +		pr_err("ERROR: Failed to change sample type for off-cpu event\n");
> +		goto out_delete_session;
> +	}
> +
>  	if (!err) {
>  		if (!rec->timestamp_filename) {
>  			record__finish_output(rec);
> @@ -3198,7 +3277,7 @@ static int switch_output_setup(struct record *rec)
>  	unsigned long val;
>  
>  	/*
> -	 * If we're using --switch-output-events, then we imply its 
> +	 * If we're using --switch-output-events, then we imply its
>  	 * --switch-output=signal, as we'll send a SIGUSR2 from the side band
>  	 *  thread to its parent.
>  	 */
> @@ -4221,9 +4300,14 @@ int cmd_record(int argc, const char **argv)
>  	}
>  
>  	if (rec->off_cpu) {
> -		err = record__config_off_cpu(rec);
> +		char off_cpu_event[64];
> +
> +		snprintf(off_cpu_event, sizeof(off_cpu_event),
> +			 "bpf-output/no-inherit=1,name=%s/", OFFCPU_EVENT);
> +
> +		err = parse_event(rec->evlist, off_cpu_event);
>  		if (err) {
> -			pr_err("record__config_off_cpu failed, error %d\n", err);
> +			pr_err("Failed to open off-cpu event\n");
>  			goto out;
>  		}
>  	}
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 3536404e9447..c08ae6a3c8d6 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1092,11 +1092,6 @@ static void evsel__set_default_freq_period(struct record_opts *opts,
>  	}
>  }
>  
> -static bool evsel__is_offcpu_event(struct evsel *evsel)
> -{
> -	return evsel__is_bpf_output(evsel) && evsel__name_is(evsel, OFFCPU_EVENT);
> -}
> -
>  /*
>   * The enable_on_exec/disabled value strategy:
>   *
> @@ -1363,9 +1358,6 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
>  	if (evsel__is_dummy_event(evsel))
>  		evsel__reset_sample_bit(evsel, BRANCH_STACK);
>  
> -	if (evsel__is_offcpu_event(evsel))
> -		evsel->core.attr.sample_type &= OFFCPU_SAMPLE_TYPES;
> -
>  	arch__post_evsel_config(evsel, attr);
>  }
>  
> -- 
> 2.44.0