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

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 20 Jul 2026, at 4:46 PM, sashiko-bot@kernel.org wrote:
> 
> 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?

I will add a sighandler variable to catch this in V3
> 
>> +
>> + 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?

V3 will address this by using : record__bytes_written(rec)
> 
> 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?
Will change this approach to handle retry in V3

> 
>> + 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?
Thanks for pointing out these catches, I will send a V3 addressing these

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