Re: [PATCH V2 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 4 potential issue(s) to consider:
- [Medium] `record__final_data()` fails to drain architecture-specific data and falsely aborts in parallel trace streaming mode due to incorrect byte tracking and thread-local mmap limitations.
- [Medium] The final data drain hook is completely bypassed on normal exit if the normal event ring buffer is empty when the termination signal is evaluated.
- [Medium] The final data drain loop aborts on the first empty read pass, failing to wait for asynchronous PMU data to arrive.
- [Medium] The force-abort mechanism `if (done > 1)` inside `record__final_data()` is mathematically impossible and will never successfully abort a stuck drain loop.
--

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

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

This patch adds a weak architecture hook to allow collecting remaining
PMU-specific data before events are disabled and closed. It introduces
a loop in the record command to perform additional read passes when the
architecture reports pending data.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
[ ... ]
> @@ -2425,6 +2425,45 @@ 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;
> +	/*
> +	 * 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.
> +	 */
> +	while (arch_perf_record__need_read(rec->evlist)) {
> +		/* If user presses Ctrl+C again during draining, abort cleanly */
> +		if (done > 1)
> +			break;

[Severity: Medium]
Is it possible for this abort condition to trigger? Looking at sig_handler()
for SIGINT, the variable done is set to 1 directly:

sig_handler() {
    ...
    done = 1;
    ...
}

Since done is never incremented, could this block of code fail to force-abort
if the PMU gets stuck during the drain?

> +
> +		last_bytes_written = rec->bytes_written;
> +
> +		if (record__mmap_read_all(rec, true) < 0)
> +			break;
> +
> +		if (rec->bytes_written == last_bytes_written) {
> +			pr_warning("Final data drain made no forward progress.\n");
> +			break;
> +		}

[Severity: Medium]
Does this check properly account for multithreaded mode? In parallel trace
streaming mode, worker threads maintain their own mmaps, write to their own
files, and increment rec->thread_bytes_written instead of rec->bytes_written.
Additionally, when record__mmap_read_all() is invoked from the main thread,
it only drains mmaps assigned to the main thread. Will this cause the loop
to abort prematurely with a false warning when running with multiple threads?

Also, if the first non-blocking read pass yields no data because the hardware
needs a moment to flush asynchronous data, the loop breaks immediately here.
Does this bypass the usleep(100) and defeat the purpose of waiting for the
pending data?

> +		usleep(100);
> +	}
> +}
> +
[ ... ]
> @@ -2845,6 +2885,11 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  			done = 1;
>  		}
>  
> +		if (done && !disabled && !final_data_drained) {
> +			record__final_data(rec);
> +			final_data_drained = true;
> +		}
> +
>  		/*
>  		 * When perf is starting the traced process, at the end events
>  		 * die with the process and we wait for that. Thus no need to

[Severity: Medium]
If done is set to 1 by a termination signal (like SIGINT) and the subsequent
read pass yields no normal events, will we ever reach this newly added code?

Looking earlier in the polling loop of __cmd_record():

    if (hits == thread->samples) {
        if (done || draining)
            break;
        ...

The break statement exits the loop completely. Does this mean that for
low-frequency workloads, if the ring buffer happens to be empty when the
signal is evaluated, the final data drain hook is completely bypassed?

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