Re: [PATCH] perf: skew perf_mux_hrtimer_handler firing across cpus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: data race on cpc->hrtimer_interval in perf_mux_hrtimer_handler()
--

commit 21db2df032d9ff638545ff8ead07c480ea100710
Author: Benjamin Segall <bsegall@google.com>

perf: skew perf_mux_hrtimer_handler firing across cpus

This commit adds a per-CPU skew offset to perf_mux_hrtimer_handler() when
the mux hrtimer interval changes so that timers across CPUs do not fire at
the same time and contend on jiffies_lock in nohz_full configurations.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6c8e38a31104..80d142986eba7 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1279,6 +1279,7 @@ static enum hrtimer_restart perf_mux_hrtimer_handler(struct hrtimer *hr)
>  {
>  	struct perf_cpu_pmu_context *cpc;
>  	bool rotations;
> +	u64 offset;
>  
>  	lockdep_assert_irqs_disabled();
>  
> @@ -1286,6 +1287,17 @@ static enum hrtimer_restart perf_mux_hrtimer_handler(struct hrtimer *hr)
>  	rotations = perf_rotate_context(cpc);
>  
>  	raw_spin_lock(&cpc->hrtimer_lock);
> +	if (cpc->hrtimer_old_interval != cpc->hrtimer_interval) {
> +		/*
> +		 * Skew the timer to avoid contention on jiffies_lock in
> +		 * nohz_full, whenever we change interval
> +		 */
> +		offset = cpc->hrtimer_interval;
> +		do_div(offset, num_possible_cpus());
> +		offset *= smp_processor_id(); /* we are PINNED_HARD */
> +		hrtimer_set_expires(hr, offset);
> +		cpc->hrtimer_old_interval = cpc->hrtimer_interval;
> +	}
>  	if (rotations)
>  		hrtimer_forward_now(hr, cpc->hrtimer_interval);

[Severity: Medium]
Can a concurrent update to cpc->hrtimer_interval from
perf_event_mux_interval_ms_store() race with perf_mux_hrtimer_handler() and
leave the timer with a mismatched skew offset?

In kernel/events/core.c, perf_event_mux_interval_ms_store() updates
cpc->hrtimer_interval across all online CPUs from the sysfs writer CPU
without holding cpc->hrtimer_lock or using WRITE_ONCE():

	for_each_online_cpu(cpu) {
		struct perf_cpu_pmu_context *cpc;
		cpc = *per_cpu_ptr(pmu->cpu_pmu_context, cpu);
		cpc->hrtimer_interval = ns_to_ktime(NSEC_PER_MSEC * timer);

		cpu_function_call(cpu, perf_mux_hrtimer_restart_ipi, cpc);
	}

Meanwhile, perf_mux_hrtimer_handler() reads cpc->hrtimer_interval multiple
times without READ_ONCE() or caching it in a local variable. Between reading
cpc->hrtimer_interval into offset and storing cpc->hrtimer_interval into
cpc->hrtimer_old_interval, calls such as do_div() and smp_processor_id()
(which calls debug_smp_processor_id() out-of-line when preemption debugging
is enabled) allow the compiler to reload cpc->hrtimer_interval from memory.

If perf_event_mux_interval_ms_store() modifies cpc->hrtimer_interval in that
window, offset can be computed from the previous interval while
cpc->hrtimer_old_interval records the newly written interval, preventing
subsequent timer callbacks from recomputing the skew for the new interval
(and potentially tearing the 64-bit ktime_t load on 32-bit architectures).

Should cpc->hrtimer_interval be accessed with WRITE_ONCE() and read once
into a local variable via READ_ONCE() (or updated under cpc->hrtimer_lock)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/xm265wykhyrv.fsf@google.com?part=1