On 13/08/2026 10:36, Athira Rajeev wrote: > > >> On 13 Aug 2026, at 11:52 AM, Adrian Hunter <[email protected]> wrote: >> >> 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/[email protected]/ > > 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 > >> >> breaks the perf ABI. >> >> I am not going to review any more tools patches for now. >> >>> >>> Signed-off-by: Athira Rajeev <[email protected]> >>> --- >>> 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; >>> +} >> >
