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

On 13/08/2026 10:36, Athira Rajeev wrote:

quoted

On 13 Aug 2026, at 11:52 AM, Adrian Hunter [off-list ref] wrote:

On 07/08/2026 17:41, Athira Rajeev wrote:

quoted

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/ (local)

Hi Adrian,
Thank you for the review.

Could you please help clarify which specific aspect of the driver breaks the perf ABI? Is it the sysfs format ABI where encoding raw hardware topology identifiers (nodeindex, nodalchipindex, coreindexonchip) are used as perf_event_attr.config bit fields?
I am trying to confirm so that I get your feedback correctly for reworking on changes.

This "draining" and "read" usage looked unusual, so I asked AI:

  I have just applied the patches for powerpc htm kernel driver - they are now the last 5 patches committed. Examine the design in
  comparison to how other PMU drivers are implemented. Does it violate the kernel API for PMUs? 

It said yes.

Thanks,
Athira

quoted

breaks the perf ABI.

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

quoted

Signed-off-by: Athira Rajeev <redacted>
---
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;
+}