[PATCH v3 0/2] governor: Fix races and stale baseline on prev_cpu_nice

[PATCH v3 0/2] governor: Fix races and stale baseline on prev_cpu_nice

[Zhongqiu Han]

Patch 1 fixes two baseline-consistency issues in the DBS governor
framework.

gov_update_cpu_data() writes prev_cpu_idle and prev_cpu_nice while
holding only attr_set->update_lock, whereas dbs_update() reads and
writes the same fields while holding only policy_dbs->update_mutex.
Because these are independent locks, the two paths are not mutually
exclusive.  The fix acquires policy_dbs->update_mutex inside
gov_update_cpu_data() for each policy.

Additionally, cpufreq_dbs_governor_start() reads io_is_busy before
acquiring policy_dbs->update_mutex.  A concurrent io_is_busy_store()
can update io_is_busy and call gov_update_cpu_data(), which writes
prev_cpu_idle with the new value under the mutex.
cpufreq_dbs_governor_start() then overwrites prev_cpu_idle with the
stale value.  The fix reads io_is_busy inside the mutex.

Patch 2 fixes a stale-baseline race on prev_cpu_nice when
ignore_nice_load is enabled via sysfs.  There is a race window
between setting ignore_nice_load and gov_update_cpu_data() acquiring
update_mutex: a concurrent dbs_update() can see the new ignore_nice_load
value but read a stale prev_cpu_nice, producing an inflated idle_time
for that sample.

The fix unconditionally advances prev_cpu_nice on every dbs_update()
call.  With prev_cpu_nice always reflecting the most recent sample,
enabling ignore_nice_load can never produce a stale-baseline spike.
To keep prev_cpu_nice handling consistent, gov_update_cpu_data()
resets prev_cpu_nice unconditionally alongside prev_cpu_idle, and
cpufreq_dbs_governor_start() initializes prev_cpu_nice unconditionally.

v3:
- Update linux-next base
- Fix two additional issues found during sashiko.dev review:
    - Patch 1: move io_busy read inside policy_dbs->update_mutex in
      cpufreq_dbs_governor_start() to close a TOCTOU window against
      concurrent io_is_busy_store().
    - Patch 2: restore unconditional prev_cpu_nice reset in
      gov_update_cpu_data() to keep both baselines consistent when
      io_is_busy changes with ignore_nice_load enabled; update comment.
    - Patch 2: keep gov_update_cpu_data() in ignore_nice_load_store() to
      prevent a stale-baseline spike in the governor-start window (when
      ignore_nice_load is set before the first dbs_update() call); only
      cpufreq_governor.c is modified.  Update commit message and add one
      more fixes tag '326c86deaed54a ("[CPUFREQ] Remove unneeded locks").'
    - Update the commit messages.
- Link to v2: https://lore.kernel.org/all/20260409111407.9775-1-zhongqiu.han@oss.qualcomm.com/
- Sashiko v2 comments: https://sashiko.dev/#/patchset/20260409111407.9775-1-zhongqiu.han%40oss.qualcomm.com

v2:
- Update linux-next base
- Based on v1 review, patch 1 is updated to add the missing
  protection around cpufreq_dbs_governor_start(), and patch 2/2 is added.
- Link to v1: https://lore.kernel.org/all/20260406110113.3475920-1-zhongqiu.han@oss.qualcomm.com/

Zhongqiu Han (2):
  cpufreq: governor: Fix data races on per-CPU idle/nice baselines
  cpufreq: governor: Fix stale prev_cpu_nice spike when enabling
    ignore_nice_load

 drivers/cpufreq/cpufreq_governor.c | 42 ++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 14 deletions(-)


base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
-- 
2.43.0

[PATCH v3 1/2] cpufreq: governor: Fix data races on per-CPU idle/nice baselines

[Zhongqiu Han]

gov_update_cpu_data() resets per-CPU prev_cpu_idle for every CPU in the
governed domain, and conditionally resets prev_cpu_nice when
ignore_nice_load is set. It is called from sysfs store callbacks
(e.g. ignore_nice_load_store) which run under attr_set->update_lock,
held by the surrounding governor_store().

Concurrently, dbs_work_handler() calls gov->gov_dbs_update() (which calls
dbs_update()) under policy_dbs->update_mutex. dbs_update() both reads and
writes the same prev_cpu_idle / prev_cpu_nice fields. The potential race
path is:

Path A (sysfs write, holds attr_set->update_lock only):

  governor_store()
    mutex_lock(&attr_set->update_lock)
    ignore_nice_load_store()
      dbs_data->ignore_nice_load = input
      gov_update_cpu_data(dbs_data)
        list_for_each_entry(policy_dbs, ...)
          for_each_cpu(j, ...)
            j_cdbs->prev_cpu_idle = get_cpu_idle_time(...)  /* write */
            j_cdbs->prev_cpu_nice = kcpustat_field(...)     /* write */
    mutex_unlock(&attr_set->update_lock)

Path B (work queue, holds policy_dbs->update_mutex only):

  dbs_work_handler()
    mutex_lock(&policy_dbs->update_mutex)
    gov->gov_dbs_update(policy)
      dbs_update()
        for_each_cpu(j, policy->cpus)
          idle_time = cur - j_cdbs->prev_cpu_idle           /* read  */
          j_cdbs->prev_cpu_idle = cur_idle_time             /* write */
          idle_time += cur_nice - j_cdbs->prev_cpu_nice     /* read  */
          j_cdbs->prev_cpu_nice = cur_nice                  /* write */
    mutex_unlock(&policy_dbs->update_mutex)

Because attr_set->update_lock and policy_dbs->update_mutex are two
completely independent locks, the two paths are not mutually exclusive.
This results in a data race on cpu_dbs_info.prev_cpu_idle and
cpu_dbs_info.prev_cpu_nice.

Fix this by also acquiring policy_dbs->update_mutex in
gov_update_cpu_data() for each policy, so that path A participates in
the mutual exclusion already established by dbs_work_handler(). Also
update the function comment to accurately reflect the two-level locking
contract.

Additionally, cpufreq_dbs_governor_start() initializes prev_cpu_idle
using io_busy read from dbs_data->io_is_busy without holding
policy_dbs->update_mutex.  A concurrent io_is_busy_store() can update
io_is_busy and call gov_update_cpu_data(), which writes prev_cpu_idle
with the new value under the mutex.  cpufreq_dbs_governor_start() then
overwrites prev_cpu_idle with the stale io_busy value, leaving the
baseline inconsistent with the tunable.  Fix this by reading io_busy
inside the mutex.

The root of this race dates back to the original ondemand/conservative
governors. Before commit ee88415caf73 ("[CPUFREQ] Cleanup locking in
conservative governor") and commit 5a75c82828e7 ("[CPUFREQ] Cleanup
locking in ondemand governor"), all accesses to prev_cpu_idle and
prev_cpu_nice in cpufreq_governor_dbs() (path X), store_ignore_nice_load()
/io_is_busy_store() (path Y), and do_dbs_timer() (path Z) were serialised
by the same dbs_mutex, so no race existed. Those two commits switched
do_dbs_timer() from dbs_mutex to a per-policy/per-cpu timer_mutex to
reduce lock contention, but left path Y (store) still holding dbs_mutex.
As a result, path Y (store) and path Z (do_dbs_timer) no longer shared a
common lock, introducing a potential race on prev_cpu_idle/prev_cpu_nice
between path Y (store) and dbs_check_cpu().

Commit 326c86deaed54a ("[CPUFREQ] Remove unneeded locks") then removed
dbs_mutex from store_ignore_nice_load()/io_is_busy_store() entirely,
introducing an additional potential race between path Y (now lockless)
and cpufreq_governor_dbs() (path X, still holding dbs_mutex), while the
race between path Y and path Z remained.

Fixes: ee88415caf736b ("[CPUFREQ] Cleanup locking in conservative governor")
Fixes: 5a75c82828e7c0 ("[CPUFREQ] Cleanup locking in ondemand governor")
Fixes: 326c86deaed54a ("[CPUFREQ] Remove unneeded locks")
Signed-off-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
---
 drivers/cpufreq/cpufreq_governor.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 86f35e451914..fc6f705c5a9c 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -90,7 +90,8 @@ EXPORT_SYMBOL_GPL(sampling_rate_store);
  * (that may be a single policy or a bunch of them if governor tunables are
  * system-wide).
  *
- * Call under the @dbs_data mutex.
+ * Call under the @dbs_data->attr_set.update_lock. The per-policy
+ * update_mutex is acquired and released internally for each policy.
  */
 void gov_update_cpu_data(struct dbs_data *dbs_data)
 {
@@ -99,6 +100,7 @@ void gov_update_cpu_data(struct dbs_data *dbs_data)
 	list_for_each_entry(policy_dbs, &dbs_data->attr_set.policy_list, list) {
 		unsigned int j;
 
+		mutex_lock(&policy_dbs->update_mutex);
 		for_each_cpu(j, policy_dbs->policy->cpus) {
 			struct cpu_dbs_info *j_cdbs = &per_cpu(cpu_dbs, j);
 
@@ -107,6 +109,7 @@ void gov_update_cpu_data(struct dbs_data *dbs_data)
 			if (dbs_data->ignore_nice_load)
 				j_cdbs->prev_cpu_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
 		}
+		mutex_unlock(&policy_dbs->update_mutex);
 	}
 }
 EXPORT_SYMBOL_GPL(gov_update_cpu_data);
@@ -527,8 +530,9 @@ int cpufreq_dbs_governor_start(struct cpufreq_policy *policy)
 
 	sampling_rate = dbs_data->sampling_rate;
 	ignore_nice = dbs_data->ignore_nice_load;
-	io_busy = dbs_data->io_is_busy;
 
+	mutex_lock(&policy_dbs->update_mutex);
+	io_busy = dbs_data->io_is_busy;
 	for_each_cpu(j, policy->cpus) {
 		struct cpu_dbs_info *j_cdbs = &per_cpu(cpu_dbs, j);
 
@@ -541,6 +545,7 @@ int cpufreq_dbs_governor_start(struct cpufreq_policy *policy)
 		if (ignore_nice)
 			j_cdbs->prev_cpu_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
 	}
+	mutex_unlock(&policy_dbs->update_mutex);
 
 	gov->start(policy);
 
-- 
2.43.0

[PATCH v3 2/2] cpufreq: governor: Fix stale prev_cpu_nice spike when enabling ignore_nice_load

[Zhongqiu Han]

When ignore_nice_load is toggled from 0 to 1 via sysfs, dbs_update() may
run concurrently and observe the new tunable value while prev_cpu_nice
still holds a stale baseline, producing a spurious massive idle_time that
results in an incorrect CPU load value.

The race can be illustrated with two concurrent paths:

Path A (sysfs write, holds attr_set->update_lock):

governor_store()
  mutex_lock(&attr_set->update_lock)
  ignore_nice_load_store()
    dbs_data->ignore_nice_load = 1              /* (A1) */
    gov_update_cpu_data(dbs_data)
      mutex_lock(&policy_dbs->update_mutex)     /* (A2) */
        j_cdbs->prev_cpu_nice = kcpustat_field(...)
      mutex_unlock(&policy_dbs->update_mutex)
  mutex_unlock(&attr_set->update_lock)

Path B (work queue, wins the race between A1 and A2):

dbs_work_handler()
  mutex_lock(&policy_dbs->update_mutex)         /* acquired before A2 */
  dbs_update()
    ignore_nice = dbs_data->ignore_nice_load    /* sees new value: 1 */
    cur_nice = kcpustat_field(...)
    idle_time += div_u64(cur_nice - j_cdbs->prev_cpu_nice, ..) /* stale */
    j_cdbs->prev_cpu_nice = cur_nice
  mutex_unlock(&policy_dbs->update_mutex)

Fix this by unconditionally sampling cur_nice and advancing prev_cpu_nice
in dbs_update() on every call, regardless of ignore_nice. With
prev_cpu_nice always reflecting the most recent sample, enabling
ignore_nice_load can never produce a stale-baseline spike: the delta will
always be the nice time accumulated in the last sampling interval, not
since boot. The additional kcpustat_field() call per CPU per sample is
negligible given that the sampling path already reads idle and load
accounting.

To keep prev_cpu_nice handling consistent with the always-tracking
semantics introduced above:

  - gov_update_cpu_data() unconditionally resets prev_cpu_nice alongside
    prev_cpu_idle, so both baselines share the same timestamp when
    io_is_busy changes.  This prevents an interval mismatch between
    idle_time and nice_delta on the next dbs_update() when
    ignore_nice_load is enabled.
  - cpufreq_dbs_governor_start() unconditionally initializes prev_cpu_nice
    so the baseline is always valid from the first dbs_update() call;
    remove the ignore_nice guard and the now-unused ignore_nice variable.

Fixes: ee88415caf736b ("[CPUFREQ] Cleanup locking in conservative governor")
Fixes: 5a75c82828e7c0 ("[CPUFREQ] Cleanup locking in ondemand governor")
Fixes: 326c86deaed54a ("[CPUFREQ] Remove unneeded locks")
Signed-off-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
---
 drivers/cpufreq/cpufreq_governor.c | 33 +++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index fc6f705c5a9c..8a85bd32defe 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -92,6 +92,12 @@ EXPORT_SYMBOL_GPL(sampling_rate_store);
  *
  * Call under the @dbs_data->attr_set.update_lock. The per-policy
  * update_mutex is acquired and released internally for each policy.
+ *
+ * Note: prev_cpu_nice is reset here unconditionally alongside prev_cpu_idle.
+ * When io_is_busy changes, both baselines must be advanced to the same
+ * timestamp so that the next dbs_update() computes idle_time and nice_delta
+ * over the same interval, preventing an artificially inflated idle_time when
+ * ignore_nice_load is enabled.
  */
 void gov_update_cpu_data(struct dbs_data *dbs_data)
 {
@@ -106,8 +112,7 @@ void gov_update_cpu_data(struct dbs_data *dbs_data)
 
 			j_cdbs->prev_cpu_idle = get_cpu_idle_time(j, &j_cdbs->prev_update_time,
 								  dbs_data->io_is_busy);
-			if (dbs_data->ignore_nice_load)
-				j_cdbs->prev_cpu_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
+			j_cdbs->prev_cpu_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
 		}
 		mutex_unlock(&policy_dbs->update_mutex);
 	}
@@ -121,6 +126,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
 	unsigned int ignore_nice = dbs_data->ignore_nice_load;
 	unsigned int max_load = 0, idle_periods = UINT_MAX;
 	unsigned int sampling_rate, io_busy, j;
+	u64 cur_nice;
 
 	/*
 	 * Sometimes governors may use an additional multiplier to increase
@@ -167,12 +173,18 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
 
 		j_cdbs->prev_cpu_idle = cur_idle_time;
 
-		if (ignore_nice) {
-			u64 cur_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
-
+		/*
+		 * Always sample cur_nice and advance prev_cpu_nice, regardless
+		 * of ignore_nice.  This keeps prev_cpu_nice current so that
+		 * enabling ignore_nice_load via sysfs never produces a
+		 * stale-baseline spike (the delta will be at most one sampling
+		 * interval of accumulated nice time, not since boot).
+		 */
+		cur_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
+		if (ignore_nice)
 			idle_time += div_u64(cur_nice - j_cdbs->prev_cpu_nice, NSEC_PER_USEC);
-			j_cdbs->prev_cpu_nice = cur_nice;
-		}
+
+		j_cdbs->prev_cpu_nice = cur_nice;
 
 		if (unlikely(!time_elapsed)) {
 			/*
@@ -519,7 +531,7 @@ int cpufreq_dbs_governor_start(struct cpufreq_policy *policy)
 	struct dbs_governor *gov = dbs_governor_of(policy);
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
-	unsigned int sampling_rate, ignore_nice, j;
+	unsigned int sampling_rate, j;
 	unsigned int io_busy;
 
 	if (!policy->cur)
@@ -529,7 +541,6 @@ int cpufreq_dbs_governor_start(struct cpufreq_policy *policy)
 	policy_dbs->rate_mult = 1;
 
 	sampling_rate = dbs_data->sampling_rate;
-	ignore_nice = dbs_data->ignore_nice_load;
 
 	mutex_lock(&policy_dbs->update_mutex);
 	io_busy = dbs_data->io_is_busy;
@@ -541,9 +552,7 @@ int cpufreq_dbs_governor_start(struct cpufreq_policy *policy)
 		 * Make the first invocation of dbs_update() compute the load.
 		 */
 		j_cdbs->prev_load = 0;
-
-		if (ignore_nice)
-			j_cdbs->prev_cpu_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
+		j_cdbs->prev_cpu_nice = kcpustat_field(&kcpustat_cpu(j), CPUTIME_NICE, j);
 	}
 	mutex_unlock(&policy_dbs->update_mutex);
 
-- 
2.43.0

Re: [PATCH v3 0/2] governor: Fix races and stale baseline on prev_cpu_nice

[Rafael J. Wysocki]

On Sun, Apr 19, 2026 at 3:27 PM Zhongqiu Han
<zhongqiu.han@oss.qualcomm.com> wrote:
>
> Patch 1 fixes two baseline-consistency issues in the DBS governor
> framework.
>
> gov_update_cpu_data() writes prev_cpu_idle and prev_cpu_nice while
> holding only attr_set->update_lock, whereas dbs_update() reads and
> writes the same fields while holding only policy_dbs->update_mutex.
> Because these are independent locks, the two paths are not mutually
> exclusive.  The fix acquires policy_dbs->update_mutex inside
> gov_update_cpu_data() for each policy.
>
> Additionally, cpufreq_dbs_governor_start() reads io_is_busy before
> acquiring policy_dbs->update_mutex.  A concurrent io_is_busy_store()
> can update io_is_busy and call gov_update_cpu_data(), which writes
> prev_cpu_idle with the new value under the mutex.
> cpufreq_dbs_governor_start() then overwrites prev_cpu_idle with the
> stale value.  The fix reads io_is_busy inside the mutex.
>
> Patch 2 fixes a stale-baseline race on prev_cpu_nice when
> ignore_nice_load is enabled via sysfs.  There is a race window
> between setting ignore_nice_load and gov_update_cpu_data() acquiring
> update_mutex: a concurrent dbs_update() can see the new ignore_nice_load
> value but read a stale prev_cpu_nice, producing an inflated idle_time
> for that sample.
>
> The fix unconditionally advances prev_cpu_nice on every dbs_update()
> call.  With prev_cpu_nice always reflecting the most recent sample,
> enabling ignore_nice_load can never produce a stale-baseline spike.
> To keep prev_cpu_nice handling consistent, gov_update_cpu_data()
> resets prev_cpu_nice unconditionally alongside prev_cpu_idle, and
> cpufreq_dbs_governor_start() initializes prev_cpu_nice unconditionally.
>
> v3:
> - Update linux-next base
> - Fix two additional issues found during sashiko.dev review:
>     - Patch 1: move io_busy read inside policy_dbs->update_mutex in
>       cpufreq_dbs_governor_start() to close a TOCTOU window against
>       concurrent io_is_busy_store().
>     - Patch 2: restore unconditional prev_cpu_nice reset in
>       gov_update_cpu_data() to keep both baselines consistent when
>       io_is_busy changes with ignore_nice_load enabled; update comment.
>     - Patch 2: keep gov_update_cpu_data() in ignore_nice_load_store() to
>       prevent a stale-baseline spike in the governor-start window (when
>       ignore_nice_load is set before the first dbs_update() call); only
>       cpufreq_governor.c is modified.  Update commit message and add one
>       more fixes tag '326c86deaed54a ("[CPUFREQ] Remove unneeded locks").'
>     - Update the commit messages.
> - Link to v2: https://lore.kernel.org/all/20260409111407.9775-1-zhongqiu.han@oss.qualcomm.com/
> - Sashiko v2 comments: https://sashiko.dev/#/patchset/20260409111407.9775-1-zhongqiu.han%40oss.qualcomm.com

So sashiko.dev still has some comments on the first patch in v3:

https://sashiko.dev/#/patchset/20260419132655.3800673-1-zhongqiu.han%40oss.qualcomm.com

Can you please have a look at that and let me know what you think?

> v2:
> - Update linux-next base
> - Based on v1 review, patch 1 is updated to add the missing
>   protection around cpufreq_dbs_governor_start(), and patch 2/2 is added.
> - Link to v1: https://lore.kernel.org/all/20260406110113.3475920-1-zhongqiu.han@oss.qualcomm.com/
>
> Zhongqiu Han (2):
>   cpufreq: governor: Fix data races on per-CPU idle/nice baselines
>   cpufreq: governor: Fix stale prev_cpu_nice spike when enabling
>     ignore_nice_load
>
>  drivers/cpufreq/cpufreq_governor.c | 42 ++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 14 deletions(-)
>
>
> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
> --
> 2.43.0
>
>

Re: [PATCH v3 0/2] governor: Fix races and stale baseline on prev_cpu_nice

[Zhongqiu Han]

On 5/22/2026 11:14 PM, Rafael J. Wysocki wrote:
> On Sun, Apr 19, 2026 at 3:27 PM Zhongqiu Han
> <zhongqiu.han@oss.qualcomm.com> wrote:
>>
>> Patch 1 fixes two baseline-consistency issues in the DBS governor
>> framework.
>>
>> gov_update_cpu_data() writes prev_cpu_idle and prev_cpu_nice while
>> holding only attr_set->update_lock, whereas dbs_update() reads and
>> writes the same fields while holding only policy_dbs->update_mutex.
>> Because these are independent locks, the two paths are not mutually
>> exclusive.  The fix acquires policy_dbs->update_mutex inside
>> gov_update_cpu_data() for each policy.
>>
>> Additionally, cpufreq_dbs_governor_start() reads io_is_busy before
>> acquiring policy_dbs->update_mutex.  A concurrent io_is_busy_store()
>> can update io_is_busy and call gov_update_cpu_data(), which writes
>> prev_cpu_idle with the new value under the mutex.
>> cpufreq_dbs_governor_start() then overwrites prev_cpu_idle with the
>> stale value.  The fix reads io_is_busy inside the mutex.
>>
>> Patch 2 fixes a stale-baseline race on prev_cpu_nice when
>> ignore_nice_load is enabled via sysfs.  There is a race window
>> between setting ignore_nice_load and gov_update_cpu_data() acquiring
>> update_mutex: a concurrent dbs_update() can see the new ignore_nice_load
>> value but read a stale prev_cpu_nice, producing an inflated idle_time
>> for that sample.
>>
>> The fix unconditionally advances prev_cpu_nice on every dbs_update()
>> call.  With prev_cpu_nice always reflecting the most recent sample,
>> enabling ignore_nice_load can never produce a stale-baseline spike.
>> To keep prev_cpu_nice handling consistent, gov_update_cpu_data()
>> resets prev_cpu_nice unconditionally alongside prev_cpu_idle, and
>> cpufreq_dbs_governor_start() initializes prev_cpu_nice unconditionally.
>>
>> v3:
>> - Update linux-next base
>> - Fix two additional issues found during sashiko.dev review:
>>      - Patch 1: move io_busy read inside policy_dbs->update_mutex in
>>        cpufreq_dbs_governor_start() to close a TOCTOU window against
>>        concurrent io_is_busy_store().
>>      - Patch 2: restore unconditional prev_cpu_nice reset in
>>        gov_update_cpu_data() to keep both baselines consistent when
>>        io_is_busy changes with ignore_nice_load enabled; update comment.
>>      - Patch 2: keep gov_update_cpu_data() in ignore_nice_load_store() to
>>        prevent a stale-baseline spike in the governor-start window (when
>>        ignore_nice_load is set before the first dbs_update() call); only
>>        cpufreq_governor.c is modified.  Update commit message and add one
>>        more fixes tag '326c86deaed54a ("[CPUFREQ] Remove unneeded locks").'
>>      - Update the commit messages.
>> - Link to v2: https://lore.kernel.org/all/20260409111407.9775-1-zhongqiu.han@oss.qualcomm.com/
>> - Sashiko v2 comments: https://sashiko.dev/#/patchset/20260409111407.9775-1-zhongqiu.han%40oss.qualcomm.com
> 
> So sashiko.dev still has some comments on the first patch in v3:
> 
> https://sashiko.dev/#/patchset/20260419132655.3800673-1-zhongqiu.han%40oss.qualcomm.com
> 
> Can you please have a look at that and let me know what you think?
> 


Hi Rafael,
Thanks for pointing this out. I've looked at both comments:

Comment 1 (io_is_busy transition window):

The race is real but pre-existing. After Patch 1 the only remaining
window is the gap between the io_is_busy write and gov_update_cpu_data()
acquiring the lock:

CPU0 (io_is_busy_store):    CPU1 (dbs_update):
io_is_busy = 1              mutex_lock()
/* waiting for mutex */     io_busy = 1  /* sees new value */
                             cur_idle = get_cpu_idle_time(io_busy=1)
                             /* prev_cpu_idle still set with io_busy=0 */
                             /* idle_time may be clamped to 0 */
                             /* load inflated for this sample */
                             mutex_unlock()
mutex_lock()
prev_cpu_idle = ../* reset */
mutex_unlock()

The mismatch between the two metrics causes a one-sample anomaly in
either direction: 0→1 tends to inflate the load estimate (frequency
pushed upward), while 1→0 tends to deflate it. The AI's "drives to
minimum" conclusion is wrong for the 0→1 case.

This window is inherent to the design. One possible fix is to add a per
policy io_is_busy copy inside policy_dbs_info and update it under
update_mutex in gov_update_cpu_data(), so dbs_update() always reads a
value consistent with the current prev_cpu_idle baseline. The impact
should be a single-sample frequency spike that self-corrects.


Comment 2 (sampling_rate_store vs. gov_set_update_util):

Also pre-existing. The race:

   cpufreq_dbs_governor_start():     sampling_rate_store():
     mutex_unlock()                    mutex_lock()
     gov->start()                      sample_delay_ns = 0
     gov_set_update_util()             mutex_unlock()
       sample_delay_ns = rate  <--->

If needed, e possible approach would be to move both gov->start() and
gov_set_update_util() inside the mutex would work.


Perhaps both can be evaluated and handled either in this patch or as a
separate follow-up.


>> v2:
>> - Update linux-next base
>> - Based on v1 review, patch 1 is updated to add the missing
>>    protection around cpufreq_dbs_governor_start(), and patch 2/2 is added.
>> - Link to v1: https://lore.kernel.org/all/20260406110113.3475920-1-zhongqiu.han@oss.qualcomm.com/
>>
>> Zhongqiu Han (2):
>>    cpufreq: governor: Fix data races on per-CPU idle/nice baselines
>>    cpufreq: governor: Fix stale prev_cpu_nice spike when enabling
>>      ignore_nice_load
>>
>>   drivers/cpufreq/cpufreq_governor.c | 42 ++++++++++++++++++++----------
>>   1 file changed, 28 insertions(+), 14 deletions(-)
>>
>>
>> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
>> --
>> 2.43.0
>>
>>


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3 0/2] governor: Fix races and stale baseline on prev_cpu_nice

[Rafael J. Wysocki]

On Sun, May 24, 2026 at 7:57 AM Zhongqiu Han
<zhongqiu.han@oss.qualcomm.com> wrote:
>
> On 5/22/2026 11:14 PM, Rafael J. Wysocki wrote:
> > On Sun, Apr 19, 2026 at 3:27 PM Zhongqiu Han
> > <zhongqiu.han@oss.qualcomm.com> wrote:
> >>
> >> Patch 1 fixes two baseline-consistency issues in the DBS governor
> >> framework.
> >>
> >> gov_update_cpu_data() writes prev_cpu_idle and prev_cpu_nice while
> >> holding only attr_set->update_lock, whereas dbs_update() reads and
> >> writes the same fields while holding only policy_dbs->update_mutex.
> >> Because these are independent locks, the two paths are not mutually
> >> exclusive.  The fix acquires policy_dbs->update_mutex inside
> >> gov_update_cpu_data() for each policy.
> >>
> >> Additionally, cpufreq_dbs_governor_start() reads io_is_busy before
> >> acquiring policy_dbs->update_mutex.  A concurrent io_is_busy_store()
> >> can update io_is_busy and call gov_update_cpu_data(), which writes
> >> prev_cpu_idle with the new value under the mutex.
> >> cpufreq_dbs_governor_start() then overwrites prev_cpu_idle with the
> >> stale value.  The fix reads io_is_busy inside the mutex.
> >>
> >> Patch 2 fixes a stale-baseline race on prev_cpu_nice when
> >> ignore_nice_load is enabled via sysfs.  There is a race window
> >> between setting ignore_nice_load and gov_update_cpu_data() acquiring
> >> update_mutex: a concurrent dbs_update() can see the new ignore_nice_load
> >> value but read a stale prev_cpu_nice, producing an inflated idle_time
> >> for that sample.
> >>
> >> The fix unconditionally advances prev_cpu_nice on every dbs_update()
> >> call.  With prev_cpu_nice always reflecting the most recent sample,
> >> enabling ignore_nice_load can never produce a stale-baseline spike.
> >> To keep prev_cpu_nice handling consistent, gov_update_cpu_data()
> >> resets prev_cpu_nice unconditionally alongside prev_cpu_idle, and
> >> cpufreq_dbs_governor_start() initializes prev_cpu_nice unconditionally.
> >>
> >> v3:
> >> - Update linux-next base
> >> - Fix two additional issues found during sashiko.dev review:
> >>      - Patch 1: move io_busy read inside policy_dbs->update_mutex in
> >>        cpufreq_dbs_governor_start() to close a TOCTOU window against
> >>        concurrent io_is_busy_store().
> >>      - Patch 2: restore unconditional prev_cpu_nice reset in
> >>        gov_update_cpu_data() to keep both baselines consistent when
> >>        io_is_busy changes with ignore_nice_load enabled; update comment.
> >>      - Patch 2: keep gov_update_cpu_data() in ignore_nice_load_store() to
> >>        prevent a stale-baseline spike in the governor-start window (when
> >>        ignore_nice_load is set before the first dbs_update() call); only
> >>        cpufreq_governor.c is modified.  Update commit message and add one
> >>        more fixes tag '326c86deaed54a ("[CPUFREQ] Remove unneeded locks").'
> >>      - Update the commit messages.
> >> - Link to v2: https://lore.kernel.org/all/20260409111407.9775-1-zhongqiu.han@oss.qualcomm.com/
> >> - Sashiko v2 comments: https://sashiko.dev/#/patchset/20260409111407.9775-1-zhongqiu.han%40oss.qualcomm.com
> >
> > So sashiko.dev still has some comments on the first patch in v3:
> >
> > https://sashiko.dev/#/patchset/20260419132655.3800673-1-zhongqiu.han%40oss.qualcomm.com
> >
> > Can you please have a look at that and let me know what you think?
> >
>
>
> Hi Rafael,
> Thanks for pointing this out. I've looked at both comments:
>
> Comment 1 (io_is_busy transition window):
>
> The race is real but pre-existing. After Patch 1 the only remaining
> window is the gap between the io_is_busy write and gov_update_cpu_data()
> acquiring the lock:
>
> CPU0 (io_is_busy_store):    CPU1 (dbs_update):
> io_is_busy = 1              mutex_lock()
> /* waiting for mutex */     io_busy = 1  /* sees new value */
>                              cur_idle = get_cpu_idle_time(io_busy=1)
>                              /* prev_cpu_idle still set with io_busy=0 */
>                              /* idle_time may be clamped to 0 */
>                              /* load inflated for this sample */
>                              mutex_unlock()
> mutex_lock()
> prev_cpu_idle = ../* reset */
> mutex_unlock()
>
> The mismatch between the two metrics causes a one-sample anomaly in
> either direction: 0→1 tends to inflate the load estimate (frequency
> pushed upward), while 1→0 tends to deflate it. The AI's "drives to
> minimum" conclusion is wrong for the 0→1 case.
>
> This window is inherent to the design. One possible fix is to add a per
> policy io_is_busy copy inside policy_dbs_info and update it under
> update_mutex in gov_update_cpu_data(), so dbs_update() always reads a
> value consistent with the current prev_cpu_idle baseline. The impact
> should be a single-sample frequency spike that self-corrects.
>
>
> Comment 2 (sampling_rate_store vs. gov_set_update_util):
>
> Also pre-existing. The race:
>
>    cpufreq_dbs_governor_start():     sampling_rate_store():
>      mutex_unlock()                    mutex_lock()
>      gov->start()                      sample_delay_ns = 0
>      gov_set_update_util()             mutex_unlock()
>        sample_delay_ns = rate  <--->
>
> If needed, e possible approach would be to move both gov->start() and
> gov_set_update_util() inside the mutex would work.

Yeah.

So I've applied both patches in the series as 7.2 material.

Please feel free to send a follow-up patch making the change mentioned above.

Thanks!

Re: [PATCH v3 0/2] governor: Fix races and stale baseline on prev_cpu_nice

[Zhongqiu Han]

On 5/27/2026 1:09 AM, Rafael J. Wysocki wrote:
> On Sun, May 24, 2026 at 7:57 AM Zhongqiu Han
> <zhongqiu.han@oss.qualcomm.com> wrote:
>>
>> On 5/22/2026 11:14 PM, Rafael J. Wysocki wrote:
>>> On Sun, Apr 19, 2026 at 3:27 PM Zhongqiu Han
>>> <zhongqiu.han@oss.qualcomm.com> wrote:
>>>>
>>>> Patch 1 fixes two baseline-consistency issues in the DBS governor
>>>> framework.
>>>>
>>>> gov_update_cpu_data() writes prev_cpu_idle and prev_cpu_nice while
>>>> holding only attr_set->update_lock, whereas dbs_update() reads and
>>>> writes the same fields while holding only policy_dbs->update_mutex.
>>>> Because these are independent locks, the two paths are not mutually
>>>> exclusive.  The fix acquires policy_dbs->update_mutex inside
>>>> gov_update_cpu_data() for each policy.
>>>>
>>>> Additionally, cpufreq_dbs_governor_start() reads io_is_busy before
>>>> acquiring policy_dbs->update_mutex.  A concurrent io_is_busy_store()
>>>> can update io_is_busy and call gov_update_cpu_data(), which writes
>>>> prev_cpu_idle with the new value under the mutex.
>>>> cpufreq_dbs_governor_start() then overwrites prev_cpu_idle with the
>>>> stale value.  The fix reads io_is_busy inside the mutex.
>>>>
>>>> Patch 2 fixes a stale-baseline race on prev_cpu_nice when
>>>> ignore_nice_load is enabled via sysfs.  There is a race window
>>>> between setting ignore_nice_load and gov_update_cpu_data() acquiring
>>>> update_mutex: a concurrent dbs_update() can see the new ignore_nice_load
>>>> value but read a stale prev_cpu_nice, producing an inflated idle_time
>>>> for that sample.
>>>>
>>>> The fix unconditionally advances prev_cpu_nice on every dbs_update()
>>>> call.  With prev_cpu_nice always reflecting the most recent sample,
>>>> enabling ignore_nice_load can never produce a stale-baseline spike.
>>>> To keep prev_cpu_nice handling consistent, gov_update_cpu_data()
>>>> resets prev_cpu_nice unconditionally alongside prev_cpu_idle, and
>>>> cpufreq_dbs_governor_start() initializes prev_cpu_nice unconditionally.
>>>>
>>>> v3:
>>>> - Update linux-next base
>>>> - Fix two additional issues found during sashiko.dev review:
>>>>       - Patch 1: move io_busy read inside policy_dbs->update_mutex in
>>>>         cpufreq_dbs_governor_start() to close a TOCTOU window against
>>>>         concurrent io_is_busy_store().
>>>>       - Patch 2: restore unconditional prev_cpu_nice reset in
>>>>         gov_update_cpu_data() to keep both baselines consistent when
>>>>         io_is_busy changes with ignore_nice_load enabled; update comment.
>>>>       - Patch 2: keep gov_update_cpu_data() in ignore_nice_load_store() to
>>>>         prevent a stale-baseline spike in the governor-start window (when
>>>>         ignore_nice_load is set before the first dbs_update() call); only
>>>>         cpufreq_governor.c is modified.  Update commit message and add one
>>>>         more fixes tag '326c86deaed54a ("[CPUFREQ] Remove unneeded locks").'
>>>>       - Update the commit messages.
>>>> - Link to v2: https://lore.kernel.org/all/20260409111407.9775-1-zhongqiu.han@oss.qualcomm.com/
>>>> - Sashiko v2 comments: https://sashiko.dev/#/patchset/20260409111407.9775-1-zhongqiu.han%40oss.qualcomm.com
>>>
>>> So sashiko.dev still has some comments on the first patch in v3:
>>>
>>> https://sashiko.dev/#/patchset/20260419132655.3800673-1-zhongqiu.han%40oss.qualcomm.com
>>>
>>> Can you please have a look at that and let me know what you think?
>>>
>>
>>
>> Hi Rafael,
>> Thanks for pointing this out. I've looked at both comments:
>>
>> Comment 1 (io_is_busy transition window):
>>
>> The race is real but pre-existing. After Patch 1 the only remaining
>> window is the gap between the io_is_busy write and gov_update_cpu_data()
>> acquiring the lock:
>>
>> CPU0 (io_is_busy_store):    CPU1 (dbs_update):
>> io_is_busy = 1              mutex_lock()
>> /* waiting for mutex */     io_busy = 1  /* sees new value */
>>                               cur_idle = get_cpu_idle_time(io_busy=1)
>>                               /* prev_cpu_idle still set with io_busy=0 */
>>                               /* idle_time may be clamped to 0 */
>>                               /* load inflated for this sample */
>>                               mutex_unlock()
>> mutex_lock()
>> prev_cpu_idle = ../* reset */
>> mutex_unlock()
>>
>> The mismatch between the two metrics causes a one-sample anomaly in
>> either direction: 0→1 tends to inflate the load estimate (frequency
>> pushed upward), while 1→0 tends to deflate it. The AI's "drives to
>> minimum" conclusion is wrong for the 0→1 case.
>>
>> This window is inherent to the design. One possible fix is to add a per
>> policy io_is_busy copy inside policy_dbs_info and update it under
>> update_mutex in gov_update_cpu_data(), so dbs_update() always reads a
>> value consistent with the current prev_cpu_idle baseline. The impact
>> should be a single-sample frequency spike that self-corrects.
>>
>>
>> Comment 2 (sampling_rate_store vs. gov_set_update_util):
>>
>> Also pre-existing. The race:
>>
>>     cpufreq_dbs_governor_start():     sampling_rate_store():
>>       mutex_unlock()                    mutex_lock()
>>       gov->start()                      sample_delay_ns = 0
>>       gov_set_update_util()             mutex_unlock()
>>         sample_delay_ns = rate  <--->
>>
>> If needed, e possible approach would be to move both gov->start() and
>> gov_set_update_util() inside the mutex would work.
> 
> Yeah.
> 
> So I've applied both patches in the series as 7.2 material.
> 
> Please feel free to send a follow-up patch making the change mentioned above.
> 
> Thanks!


Thanks Rafael,

Sure, I'll look into that and follow up with a patch accordingly.


-- 
Thx and BRs,
Zhongqiu Han