From 5475717e77fe9dddbe3e1f4676d2226c29cf93e6 Mon Sep 17 00:00:00 2001 From: Stephen Dickey Date: Mon, 15 Mar 2021 11:17:33 -0700 Subject: [PATCH] walt/pause: switch over to mutex and protect call to pause/resume Client code can miss the actual paused state of a cpu, if ref-counting is performed when the pause/resume operation is incomplete, or if an additional pause/resume operation executes prior to updating ref-counts. This becomes a problem with the online work handler, which can run at the same time as any walt_pause/walt_resume call. Protect the calls to pause/resume CPUs, such that the actual state of the CPUS is known and stored, prior to beginning a new operation. Ensure that the work function has accurate information before performing its resume operation. Change-Id: Iac279ac731f7c97b289547d36cce7de6e2b91c3a Signed-off-by: Stephen Dickey --- kernel/sched/walt/walt_pause.c | 86 +++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/kernel/sched/walt/walt_pause.c b/kernel/sched/walt/walt_pause.c index 6f28cc7ff53b..132e01d3d86e 100644 --- a/kernel/sched/walt/walt_pause.c +++ b/kernel/sched/walt/walt_pause.c @@ -8,7 +8,7 @@ #ifdef CONFIG_HOTPLUG_CPU -DEFINE_SPINLOCK(pause_lock); +static DEFINE_MUTEX(pause_lock); struct pause_cpu_state { int ref_count; @@ -22,14 +22,12 @@ static void inc_ref_counts(struct cpumask *cpus) int cpu; struct pause_cpu_state *pause_cpu_state; - spin_lock(&pause_lock); for_each_cpu(cpu, cpus) { pause_cpu_state = per_cpu_ptr(&pause_state, cpu); if (pause_cpu_state->ref_count) cpumask_clear_cpu(cpu, cpus); pause_cpu_state->ref_count++; } - spin_unlock(&pause_lock); } /* @@ -41,7 +39,6 @@ static void dec_test_ref_counts(struct cpumask *cpus) int cpu; struct pause_cpu_state *pause_cpu_state; - spin_lock(&pause_lock); for_each_cpu(cpu, cpus) { pause_cpu_state = per_cpu_ptr(&pause_state, cpu); WARN_ON_ONCE(pause_cpu_state->ref_count == 0); @@ -49,30 +46,37 @@ static void dec_test_ref_counts(struct cpumask *cpus) if (pause_cpu_state->ref_count) cpumask_clear_cpu(cpu, cpus); } - spin_unlock(&pause_lock); } /* cpus will be modified */ int walt_pause_cpus(struct cpumask *cpus) { - int ret; - cpumask_t saved_cpus; + int ret = 0; + cpumask_t requested_cpus; - cpumask_copy(&saved_cpus, cpus); - - /* add ref counts for all cpus in mask */ + mutex_lock(&pause_lock); inc_ref_counts(cpus); - /* only actually pause online CPUs */ + /* + * Add ref counts for all cpus in mask, but + * only actually pause online CPUs + */ cpumask_and(cpus, cpus, cpu_online_mask); + if (cpumask_empty(cpus)) + goto unlock; + + cpumask_copy(&requested_cpus, cpus); ret = pause_cpus(cpus); if (ret < 0) { - dec_test_ref_counts(&saved_cpus); + dec_test_ref_counts(&requested_cpus); pr_err("pause_cpus failure ret=%d cpus=%*pbl\n", ret, - cpumask_pr_args(&saved_cpus)); + cpumask_pr_args(&requested_cpus)); } +unlock: + mutex_unlock(&pause_lock); + return ret; } EXPORT_SYMBOL(walt_pause_cpus); @@ -80,35 +84,60 @@ EXPORT_SYMBOL(walt_pause_cpus); /* cpus will be modified */ int walt_resume_cpus(struct cpumask *cpus) { - int ret; - cpumask_t saved_cpus; + int ret = 0; + cpumask_t requested_cpus; - cpumask_copy(&saved_cpus, cpus); + mutex_lock(&pause_lock); - /* remove ref counts for all cpus in mask */ dec_test_ref_counts(cpus); /* only actually resume online CPUs */ cpumask_and(cpus, cpus, cpu_online_mask); + if (cpumask_empty(cpus)) + goto unlock; + + cpumask_copy(&requested_cpus, cpus); ret = resume_cpus(cpus); if (ret < 0) { - inc_ref_counts(&saved_cpus); + inc_ref_counts(&requested_cpus); pr_err("resume_cpus failure ret=%d cpus=%*pbl\n", ret, - cpumask_pr_args(&saved_cpus)); + cpumask_pr_args(&requested_cpus)); } +unlock: + mutex_unlock(&pause_lock); + return ret; } EXPORT_SYMBOL(walt_resume_cpus); struct work_struct walt_pause_online_work; -/* workfn to perform re-pause operation by detecting - * ref-counts and attempting to restore the state. - * It must not adjust the ref-counts for each cpu, or - * the actual paused state will no longer reflect client's - * expectations. +/* + * With refcounting and online/offline operations of the CPU + * a recent and accurate value for the requested CPUs versus + * ref-counted CPUs, must be made. + * + * When a CPU is onlined, this chain of events gets out of order. + * The online workfn can be entered at the same time as the + * walt_resume. If both are resuming the same set of CPUs + * the call to walt_pause will decrement ref-counts and think that + * the CPU is unpaused. If the workfn has already found all the + * ref-counts (and they were still set) it will re-pause + * the CPUs thinking that is what the client intended. This + * leads to a conflict, because the client software is no longer + * tracking these CPUs, and the state doesn't match what the client + * intended. + * + * This case needs protection to maintain a valid state + * of the device (where ref-counts == # of pause requests) + * Use a mutex such that the values read at the start of walt_pause, + * walt_resume, or walt_pause_online_workfn remain valid until the + * operation is complete. A mutex must be used because pause_cpus + * (and resume_cpus) cannot be called with a spinlock held, and + * the operation is not complete + * until those routines return. */ static void walt_pause_online_workfn(struct work_struct *work) { @@ -116,22 +145,25 @@ static void walt_pause_online_workfn(struct work_struct *work) cpumask_t re_pause_cpus; int cpu, ret = 0; + mutex_lock(&pause_lock); + cpumask_clear(&re_pause_cpus); /* search and test all online cpus */ - spin_lock(&pause_lock); for_each_online_cpu(cpu) { pause_cpu_state = per_cpu_ptr(&pause_state, cpu); if (pause_cpu_state->ref_count) cpumask_set_cpu(cpu, &re_pause_cpus); } - spin_unlock(&pause_lock); if (cpumask_empty(&re_pause_cpus)) - return; + goto unlock; /* will wait for existing hp operations to complete */ ret = pause_cpus(&re_pause_cpus); + +unlock: + mutex_unlock(&pause_lock); if (ret < 0) { pr_err("pause_cpus during online failure ret=%d cpus=%*pb1\n", ret, cpumask_pr_args(&re_pause_cpus));