[PATCH] perf: skew perf_mux_hrtimer_handler firing across cpus

[PATCH] perf: skew perf_mux_hrtimer_handler firing across cpus

[Benjamin Segall]

When in nohz_full, perf_mux_hrtimer will wind up checking if it needs to
update jiffies as of commit 53e87e3cdc15 ("timers/nohz: Last resort update
jiffies on nohz_full IRQ entry"). When perf is run systemwide or against
a wide process and needs the mux timer, this will fire against many cpus
at the same interval. Then, much like the timer tick did, this will
result in unnecessary lock contention on jiffies_lock. This is
particularly unfortunate since nohz_full is intended to reduce the time
spent in irqs on those cpus.

Adopt the same solution as sched_skew_tick and just have a different
offset for the timer on each cpu, and make sure to redo the skew if the
interval is changed.

Signed-off-by: Ben Segall <bsegall@google.com>
---
 include/linux/perf_event.h |  1 +
 kernel/events/core.c       | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index eb3f7bab498ca..e1cb052af3d2e 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1123,10 +1123,11 @@ struct perf_cpu_pmu_context {
 	int				pmu_disable_count;
 
 	raw_spinlock_t			hrtimer_lock;
 	struct hrtimer			hrtimer;
 	ktime_t				hrtimer_interval;
+	ktime_t				hrtimer_old_interval;
 	unsigned int			hrtimer_active;
 };
 
 /**
  * struct perf_event_cpu_context - per cpu event context structure
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 457d75ea71420..7885071bf0856 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1277,17 +1277,29 @@ static void perf_cgroup_switch(struct task_struct *task)
  */
 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();
 
 	cpc = container_of(hr, struct perf_cpu_pmu_context, hrtimer);
 	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);
 	else
 		cpc->hrtimer_active = 0;
 	raw_spin_unlock(&cpc->hrtimer_lock);
-- 
2.56.0.317.g454f8302e1-goog

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

[sashiko-bot]

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