Re: [PATCH V4 3/6] tools/perf: Add arch hook to drain remaining data before event close

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The main thread accesses worker thread mmaps via `record__final_data()` while worker threads are still actively polling, leading to severe data races and memory corruption.
--

--- Patch [3]: [PATCH V4 3/6] tools/perf: Add arch hook to drain remaining data before event close ---
commit 1746069e521d6537fd752aeade6ae6e917cfb5f9
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add arch hook to drain remaining data before event close

This commit adds a weak architecture hook, arch_perf_record__need_read(), to
indicate if more data needs to be collected before events are disabled and
closed. It introduces record__final_data() to perform an additional read pass
when the hook reports pending data. This allows architectures like powerpc
HTM to drain trace data and associated metadata.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e915390556752..e8ece174ba86e 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
[ ... ]
> @@ -2425,6 +2428,68 @@ static unsigned long record__waking(struct record *rec)
[ ... ]
> +static void record__final_data(struct record *rec)
> +{
> +	u64 last_bytes_written = 0;
> +	int retries = 0;
> +	int t;
> +#define FINAL_DATA_MAX_RETRIES 20  /* 20 * 1 ms = 20 ms max wait */
[ ... ]
> +	while (arch_perf_record__need_read(rec->evlist)) {
> +		if (drain_interrupted)
> +			break;
> +
> +		last_bytes_written = record__bytes_written(rec);
> +
> +		for (t = 0; t < rec->nr_threads; t++) {
> +			thread = &rec->thread_data[t];
> +			if (record__mmap_read_all(rec, true) < 0) {
> +				thread = &rec->thread_data[0];
> +				return;
> +			}
> +		}
> +		thread = &rec->thread_data[0];
[ ... ]
> @@ -2852,11 +2917,23 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
[ ... ]
>  		/*
>  		 * When perf is starting the traced process, at the end events
>  		 * die with the process and we wait for that. Thus no need to
>  		 * disable events in this case.
>  		 */
>  		if (done && !disabled && !target__none(&opts->target)) {
>  			trigger_off(&auxtrace_snapshot_trigger);
> +			record__final_data(rec);

[Severity: High]
Is it safe for the main thread to iterate over worker thread mmaps and call
record__mmap_read_all() here?

When --threads is enabled, worker threads are concurrently running
record__thread() and reading from their own mmaps:

tools/perf/builtin-record.c:record__thread() {
    ...
    if (record__mmap_read_all(thread->rec, false) < 0 || terminate)
        break;
    ...
}

Because record__final_data() is called here before record__stop_threads() is
called at the end of __cmd_record(), the main thread temporarily redirects its
TLS `thread` pointer to each worker thread's data and reads their mmaps.

Since the underlying mmap reads lack locking, this concurrent access to the
ring buffer boundaries (md->start, md->end, md->prev) could lead to data
races and corrupted perf.data events.

Could this concurrent access corrupt the ring buffer state, or is there some
other synchronization mechanism preventing the worker threads from reading
while record__final_data() runs?

>  			evlist__disable(rec->evlist);
>  			disabled = true;
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729124400.65009-1-atrajeev@linux.ibm.com?part=3