Re: [PATCH V3 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 2 potential issue(s) to consider:
- [Medium] SIGCHLD incorrectly aborts the final data drain loop, bypassing the intended double Ctrl+C behavior for child workloads.
- [Medium] Final data drain fails to read worker threads' mmaps when --threads is enabled.
--

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

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

While collecting samples using perf record, __cmd_record() disables the evlist
once recording is complete. After that, event fds are no longer read and any
remaining PMU-specific data cannot be drained. This patch adds a weak
arch_perf_record__need_read() hook so architecture code can indicate that
more data remains to be collected before events are disabled and closed.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e915390556752..e158197c2d15c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c

[ ... ]

> @@ -693,6 +694,8 @@ static void sig_handler(int sig)
>  	else
>  		signr = sig;
>  
> +	if (done)
> +		drain_interrupted = 1;

[Severity: Medium]
Will this unconditionally set drain_interrupted when a child workload
terminates and sends SIGCHLD?

If running perf record on a child workload and interrupting it with a single
Ctrl+C, the child workload terminates and sends SIGCHLD. If done was already
set by the SIGINT, would this cause the single SIGCHLD to prematurely abort
the drain loop intended for double Ctrl+C?

>  	done = 1;
>  #ifdef HAVE_EVENTFD_SUPPORT
>  	if (done_fd >= 0) {

[ ... ]

> @@ -2425,6 +2428,54 @@ static unsigned long record__waking(struct record *rec)
>  	return waking;
>  }
>  
> +/*
> + * Weak symbol - architecture can override to indicate if more
> + * data needs to be collected before finishing output.
> + *
> + * Returns: 1 if more data exists, 0 if collection is complete
> + */
> +__weak int arch_perf_record__need_read(struct evlist *evlist __maybe_unused)
> +{
> +	return 0;  /* Default: no arch-specific data to collect */
> +}
> +
> +static void record__final_data(struct record *rec)
> +{
> +	u64 last_bytes_written = 0;
> +	int retries = 0;
> +#define FINAL_DATA_MAX_RETRIES 20  /* 20 * 1 ms = 20 ms max wait */
> +
> +	/*
> +	 * Collect any remaining architecture-specific data.
> +	 * The arch code checks if more data exists, and we do the actual
> +	 * reading here since we have access to record__mmap_read_all().
> +	 * This code performs the additional read pass while events are
> +	 * still live.  A second SIGINT during drain sets drain_interrupted
> +	 * and aborts the loop immediately.
> +	 */
> +	while (arch_perf_record__need_read(rec->evlist)) {
> +		if (drain_interrupted)
> +			break;
> +
> +		last_bytes_written = record__bytes_written(rec);
> +
> +		if (record__mmap_read_all(rec, true) < 0)

[Severity: Medium]
When using the --threads option, will this fail to read worker threads' mmaps?

Looking at record__mmap_read_evlist(), the read target is mapped to
thread->maps (a __thread thread-local variable):

    nr_mmaps = thread->nr_mmaps;
    maps = overwrite ? thread->overwrite_maps : thread->maps;

Since record__final_data() is executed on the main thread, it seems it will
only access the main thread's mmaps and completely skip worker mmaps. Could
this lead to a 20ms spin loop and premature event disablement while worker
threads remain blocked in poll?

> +			break;
> +
> +		if (record__bytes_written(rec) == last_bytes_written) {
> +			if (++retries >= FINAL_DATA_MAX_RETRIES) {
> +				pr_warning("Final data drain made no forward progress after %d retries.\n",
> +					   FINAL_DATA_MAX_RETRIES);
> +				break;
> +			}
> +			usleep(1000); /* 1 ms: let AUX ring buffer consumer advance */
> +		} else {
> +			retries = 0;
> +			usleep(100);
> +		}
> +	}
> +}
> +

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