Re: [PATCH v2 3/5] alpha/perf: implement ProfileMe sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] alpha/perf: missing ProfileMe overflow reset in alpha_pmu_del()
- [Medium] alpha/perf: stale PERF_HES_UPTODATE in alpha_pmu_stop() drops counts
--

commit 52cd21b0cc0eea12519bbbab88ca2330695b853a
Author: Matt Turner <mattst88@gmail.com>

alpha/perf: implement ProfileMe sampling

This commit implements ProfileMe sampling mode on Alpha EV67 and later
processors, selectable via the profileme config bit or precise_ip. It
attributes samples to the instruction address latched in PMPC and handles
EV67 PALcode restrictions around stopped counters near overflow.

> diff --git a/arch/alpha/kernel/perf_event.c b/arch/alpha/kernel/perf_event.c
> index 70840c05d39bf..c7f0ccf8ec804 100644
> --- a/arch/alpha/kernel/perf_event.c
> +++ b/arch/alpha/kernel/perf_event.c

[ ... ]

> @@ -457,7 +519,8 @@ static void maybe_change_configuration(struct cpu_hw_events *cpuc)
>  		}
>  
>  		if (!(hwc->state & PERF_HES_STOPPED))
> -			cpuc->idx_mask |= (1<<cpuc->current_idx[j]);
> +			cpuc->idx_mask |= alpha_event_idx_mask(hwc,
> +							cpuc->current_idx[j]);
>  	}
>  	cpuc->config = cpuc->event[0]->hw.config_base;
>  }
> @@ -548,7 +611,7 @@ static void alpha_pmu_del(struct perf_event *event, int flags)
>  			alpha_perf_event_update(event, hwc, idx, 0);
>  			perf_event_update_userpage(event);
>  
> -			cpuc->idx_mask &= ~(1UL<<idx);
> +			cpuc->idx_mask &= ~alpha_event_idx_mask(hwc, idx);
>  			cpuc->n_events--;
>  			break;
>  		}

[Severity: High]
Can scheduling out a ProfileMe event via alpha_pmu_del() leave PCTR0 stopped
near overflow without clearing its overflow state?

In kernel/events/core.c, event_sched_out() removes an active event by calling
perf_pmu_disable() -> alpha_pmu_disable() followed directly by
event->pmu->del() -> alpha_pmu_del(), without calling alpha_pmu_stop():

  event_sched_out()
    perf_pmu_disable(event->pmu)
    event->pmu->del(event, 0) -> alpha_pmu_del()

Because alpha_pmu_del() does not call alpha_pmu_stop() or
alpha_write_pmc_profileme(), it looks like PCTR0 is not moved away from
overflow in several cases:

1. If a ProfileMe event is scheduled out via alpha_pmu_del() while a
   ProfileMe window is open (so PCTR0 has overflowed and stopped, before the
   window closes to generate a PMI) and no subsequent event rewrites PCTR0
   twice (for example, when cpuc->n_events becomes 0 on context switch or
   task exit, or when an Aggregate event on PCTR1 is scheduled in), PCTR0 is
   left disabled near overflow, which can block interrupts under EV67 PALcode
   restrictions 32 and 38.

2. During multiplexing in perf_rotate_context() or a context switch (which
   runs ctx_sched_out() and ctx_sched_in() with hardirqs disabled), if a
   ProfileMe window closes while IRQs are disabled, alpha_pmu_del() removes
   the ProfileMe event and ctx_sched_in() -> alpha_pmu_enable() ->
   maybe_change_configuration() schedules in an Aggregate event and overwrites
   cpuc->config:

   arch/alpha/kernel/perf_event.c:maybe_change_configuration() {
       ...
       cpuc->config = cpuc->event[0]->hw.config_base;
   }

   If the new Aggregate event is on PCTR1, cpuc->config no longer has
   EV67_PCTR_MODE_PROFILEME set when IRQs are re-enabled and the pending
   PCTR0 interrupt reaches alpha_perf_event_irq_handler():

   arch/alpha/kernel/perf_event.c:alpha_perf_event_irq_handler() {
       ...
       if (unlikely(j == cpuc->n_events)) {
           if (cpuc->config & EV67_PCTR_MODE_PROFILEME)
               alpha_write_pmc_profileme(-alpha_pmu->pmc_max_period[0]);
           wrperfmon(PERFMON_CMD_ENABLE, cpuc->idx_mask);
           return;
       }
   }

   And if the new Aggregate event is on PCTR0, alpha_write_pmc() only writes
   PCTR0 once and the pending PMI matches current_idx[0] == 0, attributing
   an overflow to the new Aggregate event.

3. Similarly, if alpha_pmu_stop() is called with flags == 0 (such as from
   perf_event_throttle()) after alpha_pmu_start() has cleared hwc->state to
   0, PERF_HES_UPTODATE is not set in hwc->state, so alpha_pmu_stop() also
   skips alpha_write_pmc_profileme().

Should alpha_pmu_del() (or a shared helper in alpha_pmu_stop() called from
alpha_pmu_del()) unconditionally clear PCTR0's overflow state when stopping
or removing a ProfileMe event?

> @@ -573,7 +636,7 @@ static void alpha_pmu_stop(struct perf_event *event, int flags)
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  
>  	if (!(hwc->state & PERF_HES_STOPPED)) {
> -		cpuc->idx_mask &= ~(1UL<<hwc->idx);
> +		cpuc->idx_mask &= ~alpha_event_idx_mask(hwc, hwc->idx);
>  		hwc->state |= PERF_HES_STOPPED;
>  	}
>  
> @@ -583,7 +646,23 @@ static void alpha_pmu_stop(struct perf_event *event, int flags)
>  	}
>  
>  	if (cpuc->enabled)
> -		wrperfmon(PERFMON_CMD_DISABLE, (1UL<<hwc->idx));
> +		wrperfmon(PERFMON_CMD_DISABLE, alpha_event_idx_mask(hwc, hwc->idx));
> +
> +	/*
> +	 * A ProfileMe event can be stopped with a window open and PCTR0 just
> +	 * past overflow. Do not leave it there while it is disabled, see
> +	 * alpha_write_pmc_profileme(). The count was read above, so rebase
> +	 * prev_count on the value written.
> +	 */
> +	if ((hwc->flags & ALPHA_PERF_FLAG_PROFILEME) &&
> +	    (hwc->state & PERF_HES_UPTODATE)) {
> +		long left = local64_read(&hwc->period_left);
> +
> +		left = clamp(left, alpha_pmu->pmc_left[0],
> +			     (long)alpha_pmu->pmc_max_period[0]);
> +		local64_set(&hwc->prev_count, (unsigned long)(-left));
> +		alpha_write_pmc_profileme((unsigned long)(-left));
> +	}
>  }

[Severity: Medium]
Does alpha_pmu_stop() overwrite PCTR0 and hwc->prev_count with a stale
period_left when an event was started via alpha_pmu_add()?

When an event is scheduled in via event_sched_in() ->
alpha_pmu_add(event, PERF_EF_START), alpha_pmu_add() sets:

arch/alpha/kernel/perf_event.c:alpha_pmu_add() {
    ...
    hwc->state = PERF_HES_UPTODATE;
    if (!(flags & PERF_EF_START))
        hwc->state |= PERF_HES_STOPPED;
    ...
}

When alpha_pmu_enable() subsequently calls maybe_change_configuration() to
program the counter, PERF_HES_UPTODATE is never cleared from hwc->state:

arch/alpha/kernel/perf_event.c:maybe_change_configuration() {
    ...
    if (cpuc->current_idx[j] == PMC_NO_INDEX) {
        alpha_perf_event_set_period(pe, hwc, idx);
        cpuc->current_idx[j] = idx;
    }

    if (!(hwc->state & PERF_HES_STOPPED))
        cpuc->idx_mask |= alpha_event_idx_mask(hwc,
                        cpuc->current_idx[j]);
}

If alpha_pmu_stop(event, PERF_EF_UPDATE) is then called (for example, from
perf_adjust_freq_unthr_events() on the scheduler tick):

arch/alpha/kernel/perf_event.c:alpha_pmu_stop() {
    ...
    if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
        alpha_perf_event_update(event, hwc, hwc->idx, 0);
        hwc->state |= PERF_HES_UPTODATE;
    }
}

Because PERF_HES_UPTODATE is already set in hwc->state,
alpha_perf_event_update() is skipped, but the ProfileMe block right below it
sees hwc->state & PERF_HES_UPTODATE as true and rewrites hwc->prev_count and
PCTR0 with the un-updated hwc->period_left, discarding the counts accumulated
in PCTR0 since the event was added or last updated.

Should maybe_change_configuration() clear PERF_HES_UPTODATE when starting an
event (or should alpha_pmu_add() use alpha_pmu_start() like other PMU
drivers)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-alpha-profileme-v2-0-c702f1c65272@gmail.com?part=3