Re: [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The function identifies HTM events using a highly brittle prefix match on the event name (`strstarts(evsel->name, "htm")`), which will erroneously match unrelated user-named events or tracepoints.
- [Low] The file uses `strcmp()` and `bool` but fails to explicitly include the standard POSIX headers `<string.h>` and `<stdbool.h>`, violating the subsystem musl compatibility guidelines.
- [Low] The string comparison against 'dummy:u' is mathematically unreachable and constitutes dead code.
--

--- Patch [4]: [PATCH V2 4/6] tools/perf: Add powerpc callback support for arch_perf_record__need_read ---
commit 08efcd22d3e33aa6575dbd818f05eb3b46a80a92
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add powerpc callback support for arch_perf_record__need_read

Implement the arch_perf_record__need_read() architecture-specific hook
for powerpc in arch/powerpc/util/evsel.c. The hook reads event counts for
open htm evsels to determine if data is pending.

> diff --git a/tools/perf/arch/powerpc/util/evsel.c b/tools/perf/arch/powerpc/util/evsel.c
> index 2f733cdc8dbbe..8f2805a680261 100644
> --- a/tools/perf/arch/powerpc/util/evsel.c
> +++ b/tools/perf/arch/powerpc/util/evsel.c
> @@ -1,8 +1,77 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <stdio.h>
> +#include <linux/string.h>
>  #include "util/evsel.h"
> +#include "util/record.h"
> +#include "util/evlist.h"
> +#include "util/debug.h"
> +#include <internal/xyarray.h>

[Severity: Low]
Does this file need to explicitly include <string.h> and <stdbool.h>?

This file uses bool and strcmp(), but relies on implicit inclusion or
<linux/string.h>. Compiling perf against musl libc requires direct inclusion
of standard POSIX headers for these types and functions to avoid build
failures.

[ ... ]

> +int arch_perf_record__need_read(struct evlist *evlist)
> +{
> +	struct evsel *evsel;

[ ... ]

> +	/* Read HTM event counts to check if more data is available */
> +	evlist__for_each_entry(evlist, evsel) {
> +		struct xyarray *xy = evsel->core.sample_id;
> +
> +		if (!evsel->name || !strstarts(evsel->name, "htm"))
> +			continue;

[Severity: Medium]
Is it safe to identify HTM events using a prefix match on the event name?

If a user traces a standard event and names it with an "htm" prefix (e.g.,
perf record -e cpu-cycles/name=htm_cycles/), this hook would falsely
identify it as an HTM event and read its event count.

For standard events, this count contains the actual accumulated hits, not the
0/1 driver staging flag, causing total_pending_bytes to always be > 0. Would
this force arch_perf_record__need_read() to always return 1 and cause the
drain loop in record__final_data() to instantly abort with a spurious warning
about making no forward progress?

Would it be more robust to identify these events by checking the PMU type
instead of the name?

> +
> +		if (!strcmp(evsel->name, "dummy:u"))
> +			continue;

[Severity: Low]
Is this string comparison reachable?

The loop explicitly filters events above with:
if (!evsel->name || !strstarts(evsel->name, "htm")) continue;

Because "dummy:u" does not start with "htm", any "dummy:u" event is already
caught and skipped by the first condition, making this check dead code.

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