Re: [PATCH 6/6] perf/x86/intel/pt: Stop tracing across system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/x86/intel/pt: pt_syscore_resume() restarts throttled Intel PT events
--

commit ec625eb052a829ee0305912ebebc209a35b2be35
Author: Adrian Hunter <adrian.hunter@intel.com>

perf/x86/intel/pt: Stop tracing across system suspend

This commit adds syscore suspend and resume callbacks to stop the active
Intel PT event on the online CPU before system suspend and restart it
on resume after PT MSRs have been reset. For snapshot mode, it saves the
output offsets before suspend so the output MSRs can be reprogrammed on
resume without reading reset MSR values.

> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 272808f2b483b..1d304ba6d9c6a 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c

[ ... ]

> @@ -1659,6 +1665,11 @@ static void pt_event_start(struct perf_event *event, int mode)
>  	 */
>  	if (!(hwc->state & PERF_HES_UPTODATE)) {
>  		hwc->state &= ~PERF_HES_STOPPED;
> +		/* After suspend, need to fully re-configure */
> +		if (READ_ONCE(pt_suspended_event)) {
> +			buf = perf_get_aux(&pt->handle);
> +			goto fully_configure;
> +		}
>  		pt_config_enable(event);
>  		return;
>  	}
> @@ -1675,7 +1686,11 @@ static void pt_event_start(struct perf_event *event, int mode)
>  
>  	hwc->state &= ~(PERF_HES_STOPPED | PERF_HES_UPTODATE);
>  
> +fully_configure:
>  	pt_config_buffer(buf);
> +	/* Output MSRs are valid again, so pt_read_offset() may read them */
> +	if (unlikely(READ_ONCE(pt_suspended_event)))
> +		WRITE_ONCE(pt_suspended_event, NULL);
>  	pt_config(event);
>  
>  	return;
> @@ -1866,6 +1881,49 @@ static int pt_event_init(struct perf_event *event)
>  	return 0;
>  }
>  
> +/*
> + * The CPU that stays online during system suspend may lose its PT state, so
> + * stop its event beforehand and start it again afterwards. The other CPUs
> + * were taken offline, which stopped their events.
> + */
> +static int pt_syscore_suspend(void *data)
> +{
> +	struct pt *pt = this_cpu_ptr(&pt_ctx);
> +	struct perf_event *event = pt->handle.event;
> +	struct pt_buffer *buf;
> +
> +	if (!event)
> +		return 0;
> +
> +	pt_event_stop(event, 0);
> +
> +	/*
> +	 * Save the output offsets while the MSRs are still valid, for
> +	 * pt_event_snapshot_aux() and for reprogramming them on resume.
> +	 */
> +	buf = perf_get_aux(&pt->handle);
> +	if (buf && buf->snapshot)
> +		pt_read_offset(buf);
> +
> +	WRITE_ONCE(pt_suspended_event, event);
> +
> +	return 0;
> +}
> +
> +static void pt_syscore_resume(void *data)
> +{
> +	struct perf_event *event = pt_suspended_event;
> +
> +	if (!event)
> +		return;
> +
> +	/* Guard against perf core having scheduled the event out meanwhile */
> +	if (event->state == PERF_EVENT_STATE_ACTIVE)
> +		pt_event_start(event, 0);
> +
> +	WRITE_ONCE(pt_suspended_event, NULL);
> +}

[Severity: High]
Can pt_syscore_resume() prematurely restart an Intel PT event that has been
throttled by perf_event_throttle(), leading to a duplicate pt_event_start()
call when the event is unthrottled on the next timer tick?

When an Intel PT event is grouped with a sampling event (for example,
perf record -C 0 --aux-sample -c100000 -e '{intel_pt//,cycles}') and the
sibling sampling event overflows max_samples_per_tick times in NMI context,
__perf_event_account_interrupt() -> perf_event_throttle_group() calls
perf_event_throttle():

kernel/events/core.c:perf_event_throttle() {
	if (event->state != PERF_EVENT_STATE_ACTIVE)
		return;

	event->hw.interrupts = MAX_INTERRUPTS;
	event->pmu->stop(event, 0);
	if (event == event->group_leader)
		perf_log_throttle(event, 0);
}

This stops the event via pt_event_stop(event, 0) and sets
event->hw.interrupts = MAX_INTERRUPTS while leaving event->state set to
PERF_EVENT_STATE_ACTIVE.

This can happen in two ways across suspend:

1. In snapshot mode before pt_syscore_suspend() runs: pt_event_stop(event, 0)
sets PERF_HES_STOPPED without calling perf_aux_output_end(), so
pt->handle.event remains non-NULL and PERF_HES_UPTODATE remains clear.
pt_syscore_suspend() then sees pt->handle.event != NULL and records
pt_suspended_event = event even though the event was already stopped by
throttling.

2. In either normal or snapshot mode between pt_syscore_suspend() and
pt_syscore_resume(): the sibling hardware PMU counter on the online CPU
remains active while local IRQs and timer ticks are disabled (for instance,
during hibernation image creation in swsusp_arch_suspend()), and can overflow
in NMI context and call perf_event_throttle_group().

In both cases, pt_syscore_resume() only checks whether event->state is
PERF_EVENT_STATE_ACTIVE, so it calls pt_event_start(event, 0) on the
throttled event, clearing PERF_HES_STOPPED and PERF_HES_UPTODATE and enabling
RTIT_CTL_TRACEEN while event->hw.interrupts is still MAX_INTERRUPTS.

Once interrupts are re-enabled after syscore_resume(), the next timer tick
runs perf_adjust_freq_unthr_context() -> perf_adjust_freq_unthr_events() ->
perf_event_unthrottle_group() -> perf_event_unthrottle():

kernel/events/core.c:perf_event_unthrottle() {
	if (event->state != PERF_EVENT_STATE_ACTIVE)
		return;

	event->hw.interrupts = 0;
	if (start)
		event->pmu->start(event, 0);
    ...
}

Because pt_syscore_resume() already started the event and cleared
PERF_HES_UPTODATE, this second pt_event_start(event, 0) call enters the
!(hwc->state & PERF_HES_UPTODATE) branch on an already-running event (even
in non-snapshot mode) and calls pt_config_enable() -> pt_config_start()
concurrently with active tracing and PT PMIs. If intel_pt_interrupt() fires
in that window and stops the event because the AUX buffer is full (setting
pt->handle.event = NULL and PERF_HES_STOPPED), pt_config_start() can then
write MSR_IA32_RTIT_CTL with RTIT_CTL_TRACEEN set while no AUX transaction
is open.

Also, if pt_syscore_resume() skips calling pt_event_start() when the event is
throttled, wouldn't clearing pt_suspended_event in pt_syscore_resume() before
perf_event_unthrottle() later calls pt_event_start() cause a throttled
snapshot event to read reset PT MSRs in pt_read_offset() and take the
pt_config_enable() path without reprogramming the output MSRs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102074039.26983-1-adrian.hunter@intel.com?part=6