> On 14 Aug 2026, at 7:51 PM, Adrian Hunter <[email protected]> wrote:
>
> 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.
Sure, thanks for directing Adrian,
Adding Peter Zijlstra to the thread..
Hi Peter,
I am working on a perf PMU driver for HTM (Hardware Trace Macro) on
IBM POWER systems. HTM captures hardware trace data via the H_HTM
hypervisor call. The driver in the patch series exposes it as a perf AUX PMU
so trace
data can be correlated with other perf events in perf.data.
Patch series:
https://lore.kernel.org/linux-perf-users/[email protected]/
https://lore.kernel.org/linux-perf-users/[email protected]/
Adrian Hunter reviewed the series and we have some queries. I am reaching out
for
your guidance on the right kernel mechanism for draining trace data into the
perf AUX buffer. Please read details below and help share your thoughts.
Apologies for the long text, I tried to explain context and concerns that
came out of the review.
Hardware constraints
--------------------
HTM is a one-shot, firmware-managed trace buffer:
- Buffer size is fixed at boot time.
Cannot be changed without a reboot.
- No hardware interrupt (no PMI, no DMA completion signal).
- The entire buffer is read after issuing a stop hcall.
Not while tracing is active.
- Buffer must be copied chunk by chunk via a hypervisor hcall into
the perf AUX ring (physically contiguous pages).
What V5 of patch series did
-----------
In V5, pmu->read() did all the work:
- On the first call: issued stop hcall to freeze the hardware
buffer, then called hcall to dump the trace data into the
AUX ring via perf_aux_output_begin() / perf_aux_output_end().
- Subsequent calls: skipped the stop (already frozen), copied the
next chunk, advanced aux_buf->head.
- Set event->count to the record count written, or 1 if the AUX
ring was full (-ENOSPC, meaning "retry"), or 0 at EOF.
- Userspace polled event->count via perf_evsel__read() in a new
weak arch_perf_record__need_read() hook added to builtin-record.c;
a non-zero count triggered another record__mmap_read_all() + retry.
Review from Adrian flagged concerns with this:
Concern 1: pmu->read() issues stop on its first call —
a hardware side effect inside pmu->read().
Concern 2: event->count is used as a flow-control signal
(1 = "retry", 0 = "done") rather than a real counter.
Addressing the two concerns for V6
-----------------------------------
Concern 2 is straightforward to fix: remove all event->count
manipulation from pmu->read(). Loop termination is detected by
AUX head not advancing. No event->count needed.
Concern 1 is harder. The query I have is:
Is it acceptable for pmu->read() to issue stop hcall exactly
once (on the first drain call, not on all subsequent calls),
then copy trace data chunk via perf_aux_output_begin() / dump trace data
/ perf_aux_output_end()?
The reason for considering this approach for HTM specifically:
- htm_event_init() rejects all non-sampling opens with -EOPNOTSUPP:
if (!is_sampling_event(event))
return -EOPNOTSUPP;
So perf stat, BPF helpers, and any other non-record caller get
-EOPNOTSUPP at perf_event_open() time. An HTM fd can only exist
inside a perf record session. The only caller of read(fd) on an
HTM event is from read hook - HTM's own drain code — which
calls it deliberately, knowing stop fires on the first
call. There is no unsuspecting caller path for HTM.
- After the first call, pmu->read() is a pure data-mover with no
further hardware side effects.
Alternative: workqueue after pmu->stop()
-----------------------------------------
If pmu->read() having any hardware side effect is not right path,
the alternative I was proposing is a workqueue-based approach:
- pmu->read() becomes a true no-op.
- pmu->stop() issues hcall stop operation to freeze the hardware buffer,
then schedules a work_struct (say 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_begin() / perf_aux_output_end(),
backing off on -ENOSPC between chunks.
- event->count is not used as a flow-control signal.
- deconfigure the tracing ( releasing the resources ) in event->destroy()
Tools side:
- arch_perf_record__need_read() hook is dropped — no changes to
builtin-record.c.
- A custom "read_finish" callback in the HTM "auxtrace_record" polls
auxtrace_mmap__read_head() until the AUX head stops advancing,
with a bounded maximum wait. This ensures the workqueue drain
completes before evlist__close() frees the mmaps.
Adrian's concern with this approach: "That sounds like pmu->stop()
wouldn't actually stop.”
Traced through the tools code, workqueue lifetime looks safe to me based on
this teardown sequence:
main poll loop exits
|
v
out_child: (builtin-record.c)
record__mmap_read_all(rec, true) <- final AUX ring drain
|
| <- read_finish called here,
| fd still open at this point
|
perf_session__delete() |
v
return from __cmd_record()
|
v
cmd_record():
evlist__delete(rec->evlist)
+-- evlist__close()
+-- close(*fd) <- fd closed here
-> release the resources
So event->destroy fires after "read_finish" .
cancel_work_sync() in event->destroy ensures the drain
workqueue finishes before releasing the hypervisor
buffer. The fd is still open when read_finish runs,
so it can safely wait for the drain.
Peterz, I have tried to summarize these below and I am seeking for
your guidance here. Please share your thoughts.
--------------------
1. Is it acceptable for pmu->read() to issue stop tracing once on
the first drain call, given that HTM events can only be opened
by perf record (non-sampling opens rejected in
htm_event_init()), so there is no unsuspecting caller path?
2. If not, is the workqueue-after-pmu->stop() approach safe?
Releasing the resources ( deconfigure ) will be placed in
event->destroy (not pmu->del()), which fires only at close(fd) — after
cancel_work_sync() ensures the workqueue is done. The AUX mmaps
also remain valid until evlist__munmap() at that same point.
So the workqueue can call perf_aux_output_begin() /
perf_aux_output_end() safely because the hypervisor buffer and
AUX mmaps are still live when it runs. Is this reasoning correct,
or does the perf core free AUX resources (free_aux / rb teardown)
earlier than event->destroy?
3. Is there a third approach we can go with ? please suggest if there is
another way.
Thank you,
Athira Rajeev
>
>>
>> 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;
>>>>>> +}
