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

[Adrian Hunter <adrian.hunter@intel.com>]

On 07/08/2026 17:41, Athira Rajeev wrote:
> Implement the arch_perf_record__need_read() architecture-specific hook
> for powerpc in arch/powerpc/util/evsel.c.
> 
> The HTM kernel driver sets event->count to the number of records still
> staged in its internal buffers (total_size / record_size), and to 0
> once the stream is exhausted.  This hook reads that count for every open
> htm evsel via perf_evsel__read() and accumulates the values into
> total_pending_records.  A non-zero total means at least one HTM target
> still has records pending; the recording loop added in the previous
> patch will perform another mmap-read pass.
> 
> The drain uses a two-layer safety check: event->count detects records
> staged by the driver, and record__bytes_written() in the drain loop
> confirms data was actually moved into perf.data.  This combination
> handles the case where the driver count is briefly stale while hardware
> is still flushing.
> 
> The implementation scans the evlist using evsel__pmu_name() to identify
> HTM events by their kernel-assigned PMU name rather than the
> user-visible event name, preventing false matches.  It iterates the fd/
> sample-id xyarray, and skips any evsel whose fd and sample-id arrays are
> mismatched to avoid reading stale state.  When the accumulated record
> count reaches zero the hook returns 0 and the recording loop proceeds to
> disable and close the events.

This looks like the proposed driver:

	https://lore.kernel.org/all/20260701083806.79358-1-atrajeev@linux.ibm.com/

breaks the perf ABI.

I am not going to review any more tools patches for now.

> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
> ---
> Changes in V5:
> - When an HTM evsel is a group sibling (evsel->core.leader !=
>   &evsel->core), read through its group leader's struct perf_evsel
>   instead of the sibling directly.  perf_evsel__read_size() uses
>   evsel->nr_members to compute the read buffer size; nr_members is 0
>   for siblings, so size=0 is passed to readn(), which returns <=0 and
>   leaves count.val=0, causing the drain loop to terminate prematurely.
>   Reading through the leader avoids the zero-size buffer and correctly
>   accumulates the leader's pending count.  HTM events are always
>   standalone or per-target leaders in practice; the leader redirect
>   handles any grouped configuration without losing counts.
> 
> Changes in V4:
> - No changes from V3.
> 
> Changes in V3:
> - Use evsel__pmu_name(evsel) instead of strstarts(evsel->name, "htm")
> to identify HTM events, matching by kernel-assigned PMU name rather
> than user-visible event name.
> - Remove the redundant two-pass loop (first pass to set found_htm,
> second to accumulate counts); a single pass with evsel__pmu_name()
> is sufficient. if no HTM event exists total_pending_records stays 0
> and the function returns 0.
> - Remove the dead !strcmp(evsel->name, "dummy:u") check;
> - evsel__pmu_name() will never return "htm" for a dummy:u software
> event.
> - Rename total_pending_bytes -> total_pending_records to match what
> the driver actually reports (event->count = total_size / record_size,
> a record count, not a byte count).
> - Add #include <string.h> for musl compatibility (strcmp() without
> it warns on some toolchains).
> 
> Changes in V2:
> - Implements the renamed arch_perf_record__need_read() hook (V1
>   implemented arch_record__collect_final_data()).
> - Skips evsels whose fd and sample-id xyarrays are mismatched, avoiding
>   stale-state reads.  V1 had no such guard.
> - evlist__enable cycling is removed; that responsibility now belongs to
>   the drain loop in builtin-record.c added in patch 3.
> - File location changed to arch/powerpc/util/evsel.c (V1 used
>   arch/powerpc/util/powerpc-htm.c).
> - Patch is now 4/6 instead of 4/9.
> 
>  tools/perf/arch/powerpc/util/evsel.c | 77 ++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/tools/perf/arch/powerpc/util/evsel.c b/tools/perf/arch/powerpc/util/evsel.c
> index 2f733cdc8dbb..2b7851c70677 100644
> --- a/tools/perf/arch/powerpc/util/evsel.c
> +++ b/tools/perf/arch/powerpc/util/evsel.c
> @@ -1,8 +1,85 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <stdio.h>
> +#include <string.h>
> +#include <unistd.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>
> +#include <internal/lib.h>
>  
>  void arch_evsel__set_sample_weight(struct evsel *evsel)
>  {
>  	evsel__set_sample_bit(evsel, WEIGHT_STRUCT);
>  }
> +
> +/*
> + * powerpc implementation of arch_perf_record__need_read().
> + *
> + * Reads event->count for every open HTM evsel by issuing a direct
> + * read() on the event fd with a plain u64 buffer, bypassing the
> + * PERF_FORMAT_GROUP path in perf_evsel__read().  When an HTM evsel is
> + * a group sibling, evsel__config() sets PERF_FORMAT_GROUP on its attr;
> + * perf_evsel__read() would then call perf_evsel__read_group() which
> + * sizes the buffer by evsel->nr_members (0 for siblings), causing the
> + * kernel to return -ENOSPC.  Reading the fd directly with sizeof(u64)
> + * retrieves the HTM driver's plain pending-record count regardless of
> + * group membership.
> + *
> + * Returns: 1 if more data exists, 0 if collection is complete
> + */
> +int arch_perf_record__need_read(struct evlist *evlist)
> +{
> +	struct evsel *evsel;
> +	u64 total_pending_records = 0;
> +	int x, y;
> +
> +	/* there was an error during record__open */
> +	if (!evlist)
> +		return 0;
> +
> +	/* Read HTM event counts to check if more data is available */
> +	evlist__for_each_entry(evlist, evsel) {
> +		struct perf_evsel *rd_evsel;
> +		struct xyarray *xy;
> +
> +		if (strcmp(evsel__pmu_name(evsel), "htm"))
> +			continue;
> +
> +		/*
> +		 * For group siblings nr_members == 0, which makes
> +		 * perf_evsel__read_size() return 0 and readn() fail.
> +		 * Read through the leader instead; perf_evsel__read_group()
> +		 * extracts the leader's own count from the group buffer.
> +		 */
> +		if (evsel->core.leader != &evsel->core)
> +			rd_evsel = evsel->core.leader;
> +		else
> +			rd_evsel = &evsel->core;
> +
> +		xy = rd_evsel->sample_id;
> +
> +		if (xy == NULL || rd_evsel->fd == NULL)
> +			continue;
> +
> +		if (xyarray__max_x(rd_evsel->fd) != xyarray__max_x(xy) ||
> +		    xyarray__max_y(rd_evsel->fd) != xyarray__max_y(xy)) {
> +			pr_debug("Unmatched FD vs sample ID array for HTM event\n");
> +			continue;
> +		}
> +
> +		for (x = 0; x < xyarray__max_x(xy); x++) {
> +			for (y = 0; y < xyarray__max_y(xy); y++) {
> +				struct perf_counts_values count = { .val = 0 };
> +
> +				if (perf_evsel__read(rd_evsel, x, y, &count) == 0)
> +					total_pending_records += count.val;
> +			}
> +		}
> +	}
> +
> +	/* Collection is complete only when ALL hardware queues have no pending records */
> +	return (total_pending_records > 0) ? 1 : 0;
> +}