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

> On 7 Aug 2026, at 8:50 PM, [email protected] wrote:
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `record__final_aux_data()` drain loop can spin infinitely during system-wide or attached-process tracing because it is called before disabling events, allowing the active PMU to continuously generate new data. Furthermore, if the main loop exits via early break, events are never explicitly disabled.
> --
> 
> --- Patch [3]: [PATCH V5 3/6] tools/perf: Add arch hook to drain remaining data before event close ---
> commit a89566ddd972102f4f5d6b30845f8dc3daa34fb4
> Author: Athira Rajeev <[email protected]>
> 
> tools/perf: Add arch hook to drain remaining data before event close
> 
> This commit introduces a mechanism to drain remaining AUX data before closing
> event file descriptors. It uses a weak architecture hook to check for pending
> data and retries reading until progress stalls.
> 
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index e915390556752..10f73aa2612ca 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
> 
> [ ... ]
> 
>> @@ -2425,6 +2428,57 @@ static unsigned long record__waking(struct record *rec)
>> return waking;
>> }
>> 
>> +/*
>> + * Weak arch hook called by record__final_data().
>> + * Returns 1 if the arch PMU driver still has records pending (the caller
>> + * will call record__mmap_read_all() and retry), 0 when done.
>> + * Implementations use perf_evsel__read() so this must be called while
>> + * events are still ACTIVE (before evlist__disable()).
>> + */
>> +__weak int arch_perf_record__need_read(struct evlist *evlist __maybe_unused)
>> +{
>> + return 0;
>> +}
>> +
>> +static void record__final_aux_data(struct record *rec)
>> +{
> 
> [ ... ]
> 
>> + if (record__bytes_written(rec) == last_bytes_written) {
>> + if (++retries >= FINAL_DATA_MAX_RETRIES) {
>> + pr_warning("Final AUX 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);
>> + }
>> + }
>> +}
> 
> [ ... ]
> 
>> @@ -2852,11 +2906,22 @@ 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);
>> + if (rec->opts.full_auxtrace)
>> + record__final_aux_data(rec);
>> evlist__disable(rec->evlist);
>> disabled = true;
>> }
>> }
> 
> [Severity: High]
> Could this lead to an infinite loop during system-wide or attached-process
> tracing?
> 
> Because record__final_aux_data() is called before evlist__disable(), the
> events are still active and generating new data.
> 
> If the workload is continuously generating data, record__bytes_written()
> will constantly increase, resetting the retry counter to 0 in the
> record__final_aux_data() loop we see above.
> 
> Would it be possible to disable the events first, and then drain the
> remaining data?

Thanks for the review.

record__final_aux_data() is called before evlist__disable() by design, for two reasons:
-  arch_perf_record__need_read() in powerpc callback is using perf_evsel__read() which requires an active event fd. Reading a disabled fd returns 0 immediately, which arch_perf_record__need_read() would interpret as “read complete", and not proceeding to further reads
- The driver needing this callback is expected to handle the event disable or disable of tracing before read.
- In case of powerpc callback which is using arch_perf_record__need_read() , issues an evsel read and  the read callback in PMU freezes the hardware trace buffer before dumping it. So its made sure there won't be infinite loop

I will update the comments documenting these details for any future architecture wiring a PMU into this hook.


> 
>> + /*
>> + * If the loop exited without entering the in-loop disable block
>> + * (early break, or child workload where target__none is true and
>> + * the block is never reached), drain any remaining AUX data now.
>> + * Events are still live at this point.
>> + */
>> + if ((target__none(&opts->target) || !disabled) && rec->opts.full_auxtrace)
>> + record__final_aux_data(rec);
> 
> [Severity: High]
> If the main loop exits via an early break and this fallback block is reached,
> does this leave the events permanently active?
> 
> Since evlist__disable() is never called in this path, we might spin
> infinitely in record__final_aux_data() for the same reason mentioned above,
> continuously reading newly generated data.

Thanks again for the review

For an early break, example:
if (hits == thread->samples) {
                        if (done || draining)
                                break;


We reaches to :

        /*
         * If the loop exited without entering the in-loop disable block
         * (early break, or child workload where target__none is true and
         * the block is never reached), drain any remaining AUX data now.
         * Events are still live at this point.
         */
        if ((target__none(&opts->target) || !disabled) && rec->opts.full_auxtrace)
                record__final_aux_data(rec);

The reason for having record__final_aux_data without disable is conveyed in above response,
But after this , we do call evlist__disable in the out_child and out_child_no_flush

out_child:
        evlist__disable(rec->evlist);
        record__stop_threads(rec);
        record__mmap_read_all(rec, true);
        goto out_free_threads;
out_child_no_flush:
        /* mmap read already failed — retrying would just fail again */
        evlist__disable(rec->evlist);
        record__stop_threads(rec);

So this won’t leave any event permanently active

Thanks
Athira


> 
>> trigger_off(&auxtrace_snapshot_trigger);
>> trigger_off(&switch_output_trigger);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3