Re: [PATCH V3 1/6] powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 25 Jul 2026, at 1:15 PM, sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Returning -ENODEV from pmu->add() does not act as a hard failure, causing a continuous retry loop that floods the hypervisor.
> - [Medium] Ignored PERF_EF_RELOAD and PERF_EF_UPDATE flags break frequency-based unthrottling and fail to prevent context-switch interruption.
> --
> 
> --- Patch [1]: [PATCH V3 1/6] powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data ---
> commit e3070ea34290d3e6db9f94435894d38b1866fb0b
> Author: Athira Rajeev <atrajeev@linux.ibm.com>
> 
> powerpc/perf: Add HTM PMU driver to expose Hardware Trace Macro data
> 
> This commit adds a new "htm" Performance Monitoring Unit (PMU) driver to
> expose Hardware Trace Macro (HTM) control via the perf interface. It
> configures the trace using the H_HTM hypervisor call with a 28-bit packed
> event configuration. The driver tracks tracing state to manage start, stop,
> add, and del operations and attempts to handle hypervisor errors.
> 
>> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
>> new file mode 100644
>> index 0000000000000..4e4c924ecfd05
>> --- /dev/null
>> +++ b/arch/powerpc/perf/htm-perf.c
> 
> [ ... ]
> 
>> @@ -201,15 +201,6 @@ static int htm_event_init(struct perf_event *event)
>> 
>> static void htm_event_start(struct perf_event *event, int flags)
>> {
>> int rc, ret, retries = 0;
>> struct htm_config cfg;
>> struct htm_target_id *target = event->pmu_private;
>> 
>> /*
>> * Ignore context-switch re-enables. The perf core passes
>> * PERF_EF_RELOAD on context-switch-in. HTM tracing is
>> * continuous at hardware level, no hcall needed.
>> */
>> if (flags & PERF_EF_RELOAD)
>> return;
> 
> [Severity: Medium]
> Does ignoring PERF_EF_RELOAD here break unthrottling for frequency-based
> events?
Thanks for all the feedback.

Frequency based attribute is not applicable for HTM pmu.
it doesn't use frequency mode,
it uses PERF_SAMPLE_RAW + AUX. 
PERF_EF_RELOAD for unthrottling is not applicable.

> 
> When an event opened in frequency mode is unthrottled, the core restarts it
> by calling event->pmu->start(event, PERF_EF_RELOAD). Will ignoring this flag
> leave the hardware trace permanently halted?
> 
> Also, does ignoring this flag actually prevent context switches from stopping
> the hardware? Context-switch-in calls pmu->add(event, PERF_EF_START), which
> this driver translates to htm_event_start(event, 0), and context-switch-out
> calls pmu->del(event, 0), translated to htm_event_stop(event, 0). Will the
> hardware still be interrupted on every context switch despite these checks?

The driver is protected from context-switch add/del callbacks by .task_ctx_nr = perf_invalid_context. 
perf_event_open() rejects any attempt to open an HTM event against a specific task .
HTM events can only be opened CPU-wide. CPU-wide events live in cpuctx->ctx, not task->perf_event_ctxp. 
perf_event_context_sched_out/in() only walks task->perf_event_ctxp and returns immediately
if it is NULL, which it is for any task with no task-context events. Therefore pmu->add() and pmu->del()
Won’t be called on context switch for HTM events. 

> 
> [ ... ]
> 
>> @@ -235,15 +235,6 @@ static void htm_event_start(struct perf_event *event, int flags)
>> static void htm_event_stop(struct perf_event *event, int flags)
>> {
>> int rc, ret, retries = 0;
>> struct htm_config cfg;
>> struct htm_target_id *target = event->pmu_private;
>> 
>> /*
>> * Ignore context-switch-out stops. The perf core passes
>> * PERF_EF_UPDATE on context-switch-out. Do not stop the hcall.
>> */
>> if (flags & PERF_EF_UPDATE)
>> return;
> 
> [Severity: Medium]
> Does ignoring PERF_EF_UPDATE here prevent the core from correctly stopping
> the event during dynamic frequency adjustments?
> 
> [ ... ]
> 
>> @@ -275,15 +275,6 @@ static void htm_event_stop(struct perf_event *event, int flags)
>> static int htm_event_add(struct perf_event *event, int flags)
>> {
>> int rc, ret, retries = 0;
>> unsigned long param1 = -1, param2 = -1;
>> struct htm_target_id *target = event->pmu_private;
>> struct htm_config cfg = target->cfg;
>> 
>> /*
>> * pmu->add() is invoked in an atomic context with interrupts disabled
>> * and hardware context locks held. sleeping is impossible.  Only retry
>> * on transient H_BUSY. On H_LONG_BUSY_* (-EAGAIN) and other errors, the
>> * loop exits and we return -ENODEV, which is a hard failure: the perf
>> * core will not reschedule the event. Returning -EAGAIN here would cause
>> * the perf core to re-queue the event and call pmu->add() again on every
>> * context switch, flooding the hypervisor with H_HTM_OP_CONFIGURE hcalls
>> * exactly when it has requested a long backoff delay.
>> */
>> do {
>> rc = htm_hcall_wrapper(htmflags, cfg.nodeindex, cfg.nodalchipindex,
>>       cfg.coreindexonchip, cfg.htmtype,
>>       H_HTM_OP_CONFIGURE, param1, param2, 0);
>> ret = htm_return_check(rc);
>> } while (ret == -EBUSY && ++retries < MAX_RETRIES);
>> 
>> if (ret <= 0)
>> return -ENODEV;
> 
> [Severity: High]
> Does returning -ENODEV here actually prevent the core from rescheduling the
> event?
> 
> Looking at event_sched_in() in kernel/events/core.c, any non-zero return
> code from pmu->add() is overridden to -EAGAIN and the event state is set to
> inactive:
> 
> kernel/events/core.c:event_sched_in() {
>    ...
>    if (event->pmu->add(event, PERF_EF_START)) {
>        perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>        event->oncpu = -1;
>        ret = -EAGAIN;
>        goto out;
>    }
>    ...
> }
> 
> Will this cause the multiplexing logic to retry scheduling on the next timer
> tick, creating an infinite loop that floods the hypervisor with configure
> hcalls exactly when it requested a long backoff delay?

You are correct — thank you. event_sched_in() converts any non-zero pmu->add() return to -EAGAIN and leaves the event as PERF_EVENT_STATE_INACTIVE, which is eligible for mux rescheduling. The comment in the patch describes the intended behaviour but the mechanism doesn't achieve it. The fix is to return 0 and call perf_event_disable_inatomic() instead, which schedules a deferred disable that sets PERF_EVENT_STATE_OFF — permanently excluded from the mux. We will fix this in the next revision.

Thanks
Athira


> 
> Should this instead return 0 and call perf_event_disable_inatomic() so the
> core processes the disable request and marks it PERF_EVENT_STATE_OFF?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260725065942.78839-1-atrajeev@linux.ibm.com?part=1