> On 17 Aug 2026, at 11:32 PM, Athira Rajeev <[email protected]> wrote:
>
>
>
>> 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.
>
Hi Peter,
Gentle ping on this. Sorry to bother you again. Just wanted to check if you
could a chance to read through my query and
if had any thoughts or if you need any additional context from my side.
Thanks,
Athira
>
> 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;
>>>>>>> +}
