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

[Benjamin Segall <bsegall@google.com>]

sashiko-bot@kernel.org writes:

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

This is correct but a pre-existing race between updates and all the
readers. I don't know how much we care since as I understand it, it's
root config changes hitting a race with the readers, but it's easy
enough so I'll send a separate patch for it.