Re: [PATCH 3/9] tools/perf: Add arch_record__collect_final_data to collect additional data before closing the event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `record__final_data` implements a defective busy-wait loop that causes CPU pegging and infinite loops.
- [High] Redundant and potentially harmful calls to `evlist__enable` during the shutdown phase.
- [High] Collection of final architecture-specific data is randomly skipped due to an early loop break race condition.
- [High] Final data collection is completely omitted when tracing child workloads.
- [Low] Unnecessary full header inclusions in `record.h` violate subsystem guidelines.
- [Low] The function name mentioned in the commit message does not match the actual code implementation.
--

commit 4b101c454c9b203ae14e9902eb253bcd6be33523
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add arch_record__collect_final_data to collect additional data before closing the event

While collecting samples using "perf record", function
"__cmd_record" checks if monitoring is done. Once recording
is done, event list will be disabled using "evlist__disable".
After this, event fd won't be read and event will be removed.

Before removing the event, if any additional data needs
to be captured/written to perf.data, currently its not
possible. Introduce arch_record__collect_final_data to
collect additional data before closing the event

> Introduce arch_record__collect_final_data to
> collect additional data before closing the event

[Severity: Low]
This isn't a bug, but the commit message mentions introducing
arch_record__collect_final_data, but the function added is actually named
arch_perf_record__need_read.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e915390556752..a60bc039401a2 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2425,6 +2425,34 @@ 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)
> +{
> +	/*
> +	 * 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().
> +	 */
> +	while (arch_perf_record__need_read(rec->evlist)) {
> +		if (record__mmap_read_all(rec, false) < 0)
> +			break;

[Severity: High]
Does this create a busy-wait loop that pegs the CPU? This loop repeatedly calls
record__mmap_read_all() without yielding or polling.

Additionally, record__mmap_read_all() flushes mmap buffers based on the
thread-local variable, so when running on the main thread it only flushes
thread_data[0]. If the data to be consumed resides in a worker thread's buffer,
could this starve the worker thread and cause an infinite loop?

> +		/* Re-enable events for next batch */
> +		evlist__enable(rec->evlist);

[Severity: High]
Is it intentional to re-enable the events here during the shutdown phase?

Since this is called when done is true to drain the final buffers, calling
evlist__enable() instructs the kernel to resume generating normal profiling
samples. Could this continuously feed new samples into the buffers and trap
the tool in an infinite loop while generating massive amounts of ioctls?

> +	}
> +
> +	return;
> +}
> +

[ ... ]

> @@ -2852,6 +2880,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  		 */
>  		if (done && !disabled && !target__none(&opts->target)) {
>  			trigger_off(&auxtrace_snapshot_trigger);
> +			record__final_data(rec);

[Severity: High]
Could the early break condition earlier in the event loop cause this final data
collection to be silently bypassed?

If the main thread's mmap buffers contain no new normal events during the
iteration when the done flag is observed:

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

It breaks out of the loop and skips over this block, randomly missing the
final architecture-specific data.

[Severity: High]
Will this fail to collect final data when tracing child workloads?

The condition !target__none(&opts->target) evaluates to false when a child
workload is specified (e.g., perf record ./my_app). Because record__final_data()
is placed inside this pre-existing guard, won't it silently omit the data
collection for normal child workloads?

>  			evlist__disable(rec->evlist);
>  			disabled = true;
>  		}
> diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
> index 93627c9a73387..21f51efd36fc7 100644
> --- a/tools/perf/util/record.h
> +++ b/tools/perf/util/record.h
> @@ -8,6 +8,8 @@
>  #include <linux/stddef.h>
>  #include <linux/perf_event.h>
>  #include "util/target.h"
> +#include "util/evlist.h"
> +#include "util/util.h"

[Severity: Low]
Is it possible to use a forward declaration here instead of full header
inclusions?

Since arch_perf_record__need_read() only references struct evlist by pointer, a
forward declaration like "struct evlist;" would avoid including these heavy
headers, aligning with the perf subsystem guidelines.

>  
>  struct option;
>  
> @@ -95,4 +97,6 @@ static inline bool record_opts__no_switch_events(const struct record_opts *opts)
>  	return opts->record_switch_events_set && !opts->record_switch_events;
>  }
>  
> +int arch_perf_record__need_read(struct evlist *evlist);
> +
>  #endif // _PERF_RECORD_H

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