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

[sashiko-bot@kernel.org]

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?

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?

[ ... ]

> @@ -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?

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