On 13/08/2026 21:40, Athira Rajeev wrote: > > >> On 13 Aug 2026, at 1:17 PM, Adrian Hunter <[email protected]> wrote: >> >> 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. > > Hi Adrian, > Thanks for taking the time to check this. > > I agree with the assessment. The core problems I understand are: > - pmu->read() issues hcalls that stop the hardware trace and dump the data, > so any read() call ends up > silently draining and stopping the trace — which isn't what read() is for. > - event->count is being used to signal "more data pending" which breaks the > expected semantics. > > I'm going to rework this rather than continue posting on top of it. > Here is my rework plan for your review: > > Kernel side: workqueue-based approach: > - pmu->read() becomes a no-op. No hcalls, no side effects. > - pmu->stop() issues hypervisor call to freeze the hardware buffer, then > schedules a > work_struct (htm_drain_work_fn()) that runs in process context and > drains the frozen buffer > chunk by chunk into the AUX ring via perf_aux_output_end(), backing off > on -ENOSPC to let > userspace consume between chunks. event->count is no longer used as a > flow-control signal.
That sounds a bit like pmu->stop() wouldn't actually stop. I can imagine there could be issues with that. I suspect only someone like Peter Zijlstra can advise you. > > Tools side: > - arch_perf_record__need_read() hook and record__final_aux_data() are > dropped entirely — no changes to builtin-record.c. > - “read_finish" callback is implemented in the HTM auxtrace_record: > During normal recording (evsel->disabled == false) it returns > immediately. > At session teardown, evlist__disable() sets evsel->disabled = true > before the > final record__mmap_read_all() call. > The read_finish callback detects this and polls aux_head (via > perf_mmap__read_head()) > in a loop, yielding delay between polls, until the AUX head stops > advancing — signalling the > workqueue drain is complete. > This gives the kernel workqueue time to finish copying chunks before > evlist__close() frees the mmaps. > The wait has a bounded maximum, so a stalled drain can't hang perf > record indefinitely. > - snapshot_finish returns -EINVAL since HTM does not support snapshot mode. > > HTM has no interrupt and trace data is consumed after pmu stop . Hence > making this read_finish callback for > auxtrace_record to consume data. > > With this, data collection is moved of from .read() entirely and uses work > queue based approach. > Can you please review , Does this approach look acceptable before I send the > updated series? > > Thanks, > Athira > > >> >>> >>> 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; >>>>> +} > >
