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

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?

> +	/*
> +	 * 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.

>  	trigger_off(&auxtrace_snapshot_trigger);
>  	trigger_off(&switch_output_trigger);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3