[PATCH v1 0/6] intel_pstate: Turbo disabled handling rework

[PATCH v1 0/6] intel_pstate: Turbo disabled handling rework

[Rafael J. Wysocki]

Hi Everyone,

This series reworks the handling of disabling turbo in intel_pstate
on top of the previous series of cleanups

https://lore.kernel.org/linux-pm/12409658.O9o76ZdvQC@kreacher/

The underlying problem is that disabling turbo is handled quite consistently
in intel_pstate and basically it can get disabled at any time (through
MSR_IA32_MISC_ENABLE_TURBO_DISABLE) without much coordination with the
cpufreq core or anything else.

Disabling turbo through the "no_turbo" sysfs attribute is more consistent,
but it has issues too (for example, if turbo is disabled via "no_turbo",
the frequency-invariance code gets notified on the turbo status change,
but the actual maximum frequency of the CPU is only updated if the
MSR_IA32_MISC_ENABLE_TURBO_DISABLE value changes either, which need not
happen at the same time or even at all).

The first patch is not really related to the rest of the series, it's
just a cleanup and can be applied separately.

Patch [2/6] uses the observation that it should be necessary to read
MSR_IA32_MISC_ENABLE_TURBO_DISABLE after driver initialization to remove
in-flight reads on that MSR and turbo state updates related to them.

Patch [3/6] builds on top of the previous one to adjust the "no_turbo"
attribute "store" and "show" callbacks.

Patch [4/6] adds READ_ONCE() annotations to global.no_turbo accesses and
makes some related simplifications.

Patch [5/6] replaces the cached MSR_IA32_MISC_ENABLE_TURBO_DISABLE
value in some checks with global.no_turbo for consistency.

Patch [6/6] makes all of the code paths where the maximum CPU frequency
can change to do that consistently by using the same set of functions.

Details are described in the individual patch changelogs.

Thanks!

[PATCH v1 1/6] cpufreq: intel_pstate: Fold intel_pstate_max_within_limits() into caller

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Fold intel_pstate_max_within_limits() into its only caller.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2012,14 +2012,6 @@ static void intel_pstate_set_min_pstate(
 	intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate);
 }
 
-static void intel_pstate_max_within_limits(struct cpudata *cpu)
-{
-	int pstate = max(cpu->pstate.min_pstate, cpu->max_perf_ratio);
-
-	update_turbo_state();
-	intel_pstate_set_pstate(cpu, pstate);
-}
-
 static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
 {
 	int perf_ctl_max_phys = pstate_funcs.get_max_physical(cpu->cpu);
@@ -2594,12 +2586,15 @@ static int intel_pstate_set_policy(struc
 	intel_pstate_update_perf_limits(cpu, policy->min, policy->max);
 
 	if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
+		int pstate = max(cpu->pstate.min_pstate, cpu->max_perf_ratio);
+
 		/*
 		 * NOHZ_FULL CPUs need this as the governor callback may not
 		 * be invoked on them.
 		 */
 		intel_pstate_clear_update_util_hook(policy->cpu);
-		intel_pstate_max_within_limits(cpu);
+		update_turbo_state();
+		intel_pstate_set_pstate(cpu, pstate);
 	} else {
 		intel_pstate_set_update_util_hook(policy->cpu);
 	}

[PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The global.turbo_disabled is updated quite often, especially in the
passive mode in which case it is updated every time the scheduler calls
into the driver.  However, this is generally not necessary and it adds
MSR read overhead to scheduler code paths (and that particular MSR is
slow to read).

For this reason, make the driver read MSR_IA32_MISC_ENABLE_TURBO_DISABLE
just once at the cpufreq driver registration time and remove all of the
in-flight updates of global.turbo_disabled.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   51 ++++++-----------------------------------
 1 file changed, 8 insertions(+), 43 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -173,7 +173,6 @@ struct vid_data {
  *			based on the MSR_IA32_MISC_ENABLE value and whether or
  *			not the maximum reported turbo P-state is different from
  *			the maximum reported non-turbo one.
- * @turbo_disabled_mf:	The @turbo_disabled value reflected by cpuinfo.max_freq.
  * @min_perf_pct:	Minimum capacity limit in percent of the maximum turbo
  *			P-state capacity.
  * @max_perf_pct:	Maximum capacity limit in percent of the maximum turbo
@@ -182,7 +181,6 @@ struct vid_data {
 struct global_params {
 	bool no_turbo;
 	bool turbo_disabled;
-	bool turbo_disabled_mf;
 	int max_perf_pct;
 	int min_perf_pct;
 };
@@ -594,12 +592,13 @@ static void intel_pstate_hybrid_hwp_adju
 	cpu->pstate.min_pstate = intel_pstate_freq_to_hwp(cpu, freq);
 }
 
-static inline void update_turbo_state(void)
+static bool turbo_is_disabled(void)
 {
 	u64 misc_en;
 
 	rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
-	global.turbo_disabled = misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE;
+
+	return !!(misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE);
 }
 
 static int min_perf_pct_min(void)
@@ -1154,40 +1153,16 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
-	policy->cpuinfo.max_freq = global.turbo_disabled_mf ?
+	policy->cpuinfo.max_freq = global.turbo_disabled ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
 	refresh_frequency_limits(policy);
 }
 
-static void intel_pstate_update_max_freq(unsigned int cpu)
-{
-	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
-
-	if (!policy)
-		return;
-
-	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
-
-	cpufreq_cpu_release(policy);
-}
-
 static void intel_pstate_update_limits(unsigned int cpu)
 {
 	mutex_lock(&intel_pstate_driver_lock);
 
-	update_turbo_state();
-	/*
-	 * If turbo has been turned on or off globally, policy limits for
-	 * all CPUs need to be updated to reflect that.
-	 */
-	if (global.turbo_disabled_mf != global.turbo_disabled) {
-		global.turbo_disabled_mf = global.turbo_disabled;
-		arch_set_max_freq_ratio(global.turbo_disabled);
-		for_each_possible_cpu(cpu)
-			intel_pstate_update_max_freq(cpu);
-	} else {
-		cpufreq_update_policy(cpu);
-	}
+	cpufreq_update_policy(cpu);
 
 	mutex_unlock(&intel_pstate_driver_lock);
 }
@@ -1287,7 +1262,6 @@ static ssize_t show_no_turbo(struct kobj
 		return -EAGAIN;
 	}
 
-	update_turbo_state();
 	if (global.turbo_disabled)
 		ret = sprintf(buf, "%u\n", global.turbo_disabled);
 	else
@@ -1317,7 +1291,6 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_lock(&intel_pstate_limits_lock);
 
-	update_turbo_state();
 	if (global.turbo_disabled) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
 		mutex_unlock(&intel_pstate_limits_lock);
@@ -2281,8 +2254,6 @@ static void intel_pstate_adjust_pstate(s
 	struct sample *sample;
 	int target_pstate;
 
-	update_turbo_state();
-
 	target_pstate = get_target_pstate(cpu);
 	target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
 	trace_cpu_frequency(target_pstate * cpu->pstate.scaling, cpu->cpu);
@@ -2593,7 +2564,6 @@ static int intel_pstate_set_policy(struc
 		 * be invoked on them.
 		 */
 		intel_pstate_clear_update_util_hook(policy->cpu);
-		update_turbo_state();
 		intel_pstate_set_pstate(cpu, pstate);
 	} else {
 		intel_pstate_set_update_util_hook(policy->cpu);
@@ -2637,7 +2607,6 @@ static void intel_pstate_verify_cpu_poli
 {
 	int max_freq;
 
-	update_turbo_state();
 	if (hwp_active) {
 		intel_pstate_get_hwp_cap(cpu);
 		max_freq = global.no_turbo || global.turbo_disabled ?
@@ -2734,8 +2703,6 @@ static int __intel_pstate_cpu_init(struc
 
 	/* cpuinfo and default policy values */
 	policy->cpuinfo.min_freq = cpu->pstate.min_freq;
-	update_turbo_state();
-	global.turbo_disabled_mf = global.turbo_disabled;
 	policy->cpuinfo.max_freq = global.turbo_disabled ?
 			cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 
@@ -2901,8 +2868,6 @@ static int intel_cpufreq_target(struct c
 	struct cpufreq_freqs freqs;
 	int target_pstate;
 
-	update_turbo_state();
-
 	freqs.old = policy->cur;
 	freqs.new = target_freq;
 
@@ -2924,8 +2889,6 @@ static unsigned int intel_cpufreq_fast_s
 	struct cpudata *cpu = all_cpu_data[policy->cpu];
 	int target_pstate;
 
-	update_turbo_state();
-
 	target_pstate = intel_pstate_freq_to_hwp(cpu, target_freq);
 
 	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, true);
@@ -2943,7 +2906,6 @@ static void intel_cpufreq_adjust_perf(un
 	int old_pstate = cpu->pstate.current_pstate;
 	int cap_pstate, min_pstate, max_pstate, target_pstate;
 
-	update_turbo_state();
 	cap_pstate = global.turbo_disabled ? HWP_GUARANTEED_PERF(hwp_cap) :
 					     HWP_HIGHEST_PERF(hwp_cap);
 
@@ -3131,6 +3093,9 @@ static int intel_pstate_register_driver(
 
 	memset(&global, 0, sizeof(global));
 	global.max_perf_pct = 100;
+	global.turbo_disabled = turbo_is_disabled();
+
+	arch_set_max_freq_ratio(global.turbo_disabled);
 
 	intel_pstate_driver = driver;
 	ret = cpufreq_register_driver(intel_pstate_driver);

[PATCH v1 3/6] cpufreq: intel_pstate: Rearrange show_no_turbo() and store_no_turbo()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Now that global.turbo_disabled can only change at the cpufreq driver
registration time, initialize global.no_turbo at that time too so they
are in sync to start with (if the former is set, the latter cannot be
updated later anyway).

That allows show_no_turbo() to be simlified because it does not need
to check global.turbo_disabled and store_no_turbo() can be rearranged
to avoid doing anything if the new value of global.no_turbo is equal
to the current one and only return an error on attempts to clear
global.no_turbo when global.turbo_disabled.

While at it, eliminate the redundant ret variable from store_no_turbo().

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1262,10 +1262,7 @@ static ssize_t show_no_turbo(struct kobj
 		return -EAGAIN;
 	}
 
-	if (global.turbo_disabled)
-		ret = sprintf(buf, "%u\n", global.turbo_disabled);
-	else
-		ret = sprintf(buf, "%u\n", global.no_turbo);
+	ret = sprintf(buf, "%u\n", global.no_turbo);
 
 	mutex_unlock(&intel_pstate_driver_lock);
 
@@ -1276,31 +1273,34 @@ static ssize_t store_no_turbo(struct kob
 			      const char *buf, size_t count)
 {
 	unsigned int input;
-	int ret;
+	bool no_turbo;
 
-	ret = sscanf(buf, "%u", &input);
-	if (ret != 1)
+	if (sscanf(buf, "%u", &input) != 1)
 		return -EINVAL;
 
 	mutex_lock(&intel_pstate_driver_lock);
 
 	if (!intel_pstate_driver) {
-		mutex_unlock(&intel_pstate_driver_lock);
-		return -EAGAIN;
+		count = -EAGAIN;
+		goto unlock_driver;
 	}
 
-	mutex_lock(&intel_pstate_limits_lock);
+	no_turbo = !!clamp_t(int, input, 0, 1);
+
+	if (no_turbo == global.no_turbo)
+		goto unlock_driver;
 
 	if (global.turbo_disabled) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
-		mutex_unlock(&intel_pstate_limits_lock);
-		mutex_unlock(&intel_pstate_driver_lock);
-		return -EPERM;
+		count = -EPERM;
+		goto unlock_driver;
 	}
 
-	global.no_turbo = clamp_t(int, input, 0, 1);
+	global.no_turbo = no_turbo;
+
+	mutex_lock(&intel_pstate_limits_lock);
 
-	if (global.no_turbo) {
+	if (no_turbo) {
 		struct cpudata *cpu = all_cpu_data[0];
 		int pct = cpu->pstate.max_pstate * 100 / cpu->pstate.turbo_pstate;
 
@@ -1312,8 +1312,9 @@ static ssize_t store_no_turbo(struct kob
 	mutex_unlock(&intel_pstate_limits_lock);
 
 	intel_pstate_update_policies();
-	arch_set_max_freq_ratio(global.no_turbo);
+	arch_set_max_freq_ratio(no_turbo);
 
+unlock_driver:
 	mutex_unlock(&intel_pstate_driver_lock);
 
 	return count;
@@ -3094,6 +3095,7 @@ static int intel_pstate_register_driver(
 	memset(&global, 0, sizeof(global));
 	global.max_perf_pct = 100;
 	global.turbo_disabled = turbo_is_disabled();
+	global.no_turbo = global.turbo_disabled;
 
 	arch_set_max_freq_ratio(global.turbo_disabled);
 

[PATCH v1 4/6] cpufreq: intel_pstate: Read global.no_turbo under READ_ONCE()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because global.no_turbo is generally not read under intel_pstate_driver_lock
make store_no_turbo() use WRITE_ONCE() for updating it (this is the only
place at which it is updated except for the initialization) and make the
majority of places reading it use READ_ONCE().

Also remove redundant global.turbo_disabled checks from places that
depend on the 'true' value of global.no_turbo because it can only be
'true' if global.turbo_disabled is also 'true'.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1296,7 +1296,7 @@ static ssize_t store_no_turbo(struct kob
 		goto unlock_driver;
 	}
 
-	global.no_turbo = no_turbo;
+	WRITE_ONCE(global.no_turbo, no_turbo);
 
 	mutex_lock(&intel_pstate_limits_lock);
 
@@ -1748,7 +1748,7 @@ static u64 atom_get_val(struct cpudata *
 	u32 vid;
 
 	val = (u64)pstate << 8;
-	if (global.no_turbo && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
 		val |= (u64)1 << 32;
 
 	vid_fp = cpudata->vid.min + mul_fp(
@@ -1913,7 +1913,7 @@ static u64 core_get_val(struct cpudata *
 	u64 val;
 
 	val = (u64)pstate << 8;
-	if (global.no_turbo && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
 		val |= (u64)1 << 32;
 
 	return val;
@@ -2211,7 +2211,7 @@ static inline int32_t get_target_pstate(
 
 	sample->busy_scaled = busy_frac * 100;
 
-	target = global.no_turbo || global.turbo_disabled ?
+	target = READ_ONCE(global.no_turbo) ?
 			cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
 	target += target >> 2;
 	target = mul_fp(target, busy_frac);
@@ -2473,7 +2473,7 @@ static void intel_pstate_clear_update_ut
 
 static int intel_pstate_get_max_freq(struct cpudata *cpu)
 {
-	return global.turbo_disabled || global.no_turbo ?
+	return READ_ONCE(global.no_turbo) ?
 			cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 }
 
@@ -2610,7 +2610,7 @@ static void intel_pstate_verify_cpu_poli
 
 	if (hwp_active) {
 		intel_pstate_get_hwp_cap(cpu);
-		max_freq = global.no_turbo || global.turbo_disabled ?
+		max_freq = READ_ONCE(global.no_turbo) ?
 				cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 	} else {
 		max_freq = intel_pstate_get_max_freq(cpu);

[PATCH v1 5/6] cpufreq: intel_pstate: Replace three global.turbo_disabled checks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Replace the global.turbo_disabled in __intel_pstate_update_max_freq() with
a global.no_turbo one to make store_no_turbo() actually update the maximum
CPU frequency on the trubo preference changes, which needs to be consistent
with arch_set_max_freq_ratio() called from there.

For more consistency, replace the global.turbo_disabled checks in
__intel_pstate_cpu_init() and intel_cpufreq_adjust_perf() with
global.no_turbo checks either.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1153,7 +1153,7 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
-	policy->cpuinfo.max_freq = global.turbo_disabled ?
+	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
 	refresh_frequency_limits(policy);
 }
@@ -2704,7 +2704,7 @@ static int __intel_pstate_cpu_init(struc
 
 	/* cpuinfo and default policy values */
 	policy->cpuinfo.min_freq = cpu->pstate.min_freq;
-	policy->cpuinfo.max_freq = global.turbo_disabled ?
+	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 
 	policy->min = policy->cpuinfo.min_freq;
@@ -2907,8 +2907,9 @@ static void intel_cpufreq_adjust_perf(un
 	int old_pstate = cpu->pstate.current_pstate;
 	int cap_pstate, min_pstate, max_pstate, target_pstate;
 
-	cap_pstate = global.turbo_disabled ? HWP_GUARANTEED_PERF(hwp_cap) :
-					     HWP_HIGHEST_PERF(hwp_cap);
+	cap_pstate = READ_ONCE(global.no_turbo) ?
+					HWP_GUARANTEED_PERF(hwp_cap) :
+					HWP_HIGHEST_PERF(hwp_cap);
 
 	/* Optimization: Avoid unnecessary divisions. */
 

[PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There are 3 places at which the maximum CPU frequency may change,
store_no_turbo(), intel_pstate_update_limits() (when called by the
cpufreq core) and intel_pstate_notify_work() (when handling a HWP
change notification).  Currently, cpuinfo.max_freq is only updated by
store_no_turbo(), although it principle it may be necessary to update
it at the other 2 places too.

Make all of them mutually consistent.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
+	intel_pstate_get_hwp_cap(cpudata);
+
 	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
+
 	refresh_frequency_limits(policy);
 }
 
 static void intel_pstate_update_limits(unsigned int cpu)
 {
-	mutex_lock(&intel_pstate_driver_lock);
+	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
 
-	cpufreq_update_policy(cpu);
+	if (!policy)
+		return;
 
-	mutex_unlock(&intel_pstate_driver_lock);
+	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
+
+	cpufreq_cpu_release(policy);
+}
+
+static void intel_pstate_update_limits_for_all(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		intel_pstate_update_limits(cpu);
 }
 
 /************************** sysfs begin ************************/
@@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	intel_pstate_update_policies();
+	intel_pstate_update_limits_for_all();
 	arch_set_max_freq_ratio(no_turbo);
 
 unlock_driver:
@@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
 	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata->cpu);
 
 	if (policy) {
-		intel_pstate_get_hwp_cap(cpudata);
 		__intel_pstate_update_max_freq(cpudata, policy);
 
 		cpufreq_cpu_release(policy);

Re: [PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:06 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There are 3 places at which the maximum CPU frequency may change,
> store_no_turbo(), intel_pstate_update_limits() (when called by the
> cpufreq core) and intel_pstate_notify_work() (when handling a HWP
> change notification).  Currently, cpuinfo.max_freq is only updated by
> store_no_turbo(), although it principle it may be necessary to update
> it at the other 2 places too.

It also works for intel_pstate_notify_work() path as is without this
change.

To start with:

$ sudo rdmsr 0x771
6080c14
$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
2000000
800000
0

Now trigger a max frequency change via SST. intel_pstate_notify_work()
called because guaranteed also changed. We didn't subscribe for max
change only (although we should).

Max changed from 2GHz to 1.9 GHz.

$ sudo rdmsr 0x771
6080e13
[labuser@gnr-bkc ~]$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
1900000
800000
0

Now trigger SST to change to max frequency to 2GHz.

sudo rdmsr 0x771
6080c14

cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
2000000
800000
0

May be you mean something else.

Thanks,
Srinivas


> 
> Make all of them mutually consistent.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
>  static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
>                                            struct cpufreq_policy
> *policy)
>  {
> +       intel_pstate_get_hwp_cap(cpudata);
> +
>         policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
>                         cpudata->pstate.max_freq : cpudata-
> >pstate.turbo_freq;
> +
>         refresh_frequency_limits(policy);
>  }
>  
>  static void intel_pstate_update_limits(unsigned int cpu)
>  {
> -       mutex_lock(&intel_pstate_driver_lock);
> +       struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
>  
> -       cpufreq_update_policy(cpu);
> +       if (!policy)
> +               return;
>  
> -       mutex_unlock(&intel_pstate_driver_lock);
> +       __intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
> +
> +       cpufreq_cpu_release(policy);
> +}
> +
> +static void intel_pstate_update_limits_for_all(void)
> +{
> +       int cpu;
> +
> +       for_each_possible_cpu(cpu)
> +               intel_pstate_update_limits(cpu);
>  }
>  
>  /************************** sysfs begin ************************/
> @@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
>  
>         mutex_unlock(&intel_pstate_limits_lock);
>  
> -       intel_pstate_update_policies();
> +       intel_pstate_update_limits_for_all();
>         arch_set_max_freq_ratio(no_turbo);
>  
>  unlock_driver:
> @@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
>         struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata-
> >cpu);
>  
>         if (policy) {
> -               intel_pstate_get_hwp_cap(cpudata);
>                 __intel_pstate_update_max_freq(cpudata, policy);
>  
>                 cpufreq_cpu_release(policy);
> 
> 
> 

Re: [PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[Rafael J. Wysocki]

On Wed, Mar 27, 2024 at 7:08 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Mon, 2024-03-25 at 18:06 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There are 3 places at which the maximum CPU frequency may change,
> > store_no_turbo(), intel_pstate_update_limits() (when called by the
> > cpufreq core) and intel_pstate_notify_work() (when handling a HWP
> > change notification).  Currently, cpuinfo.max_freq is only updated by
> > store_no_turbo(), although it principle it may be necessary to update
> > it at the other 2 places too.
>
> It also works for intel_pstate_notify_work() path as is without this
> change.
>
> To start with:
>
> $ sudo rdmsr 0x771
> 6080c14
> $ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
> 2000000
> 800000
> 0
>
> Now trigger a max frequency change via SST. intel_pstate_notify_work()
> called because guaranteed also changed. We didn't subscribe for max
> change only (although we should).
>
> Max changed from 2GHz to 1.9 GHz.
>
> $ sudo rdmsr 0x771
> 6080e13
> [labuser@gnr-bkc ~]$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
> 1900000
> 800000
> 0
>
> Now trigger SST to change to max frequency to 2GHz.
>
> sudo rdmsr 0x771
> 6080c14
>
> cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
> 2000000
> 800000
> 0
>
> May be you mean something else.

No, I don't, and you are right.

When I was writing the changelog, I somehow forgot that
intel_pstate_notify_work() called __intel_pstate_update_max_freq(),
even though the code changes in the patch obviously take that into
account (I can't really explain what happened :-/).

I'll fix the changelog.

Cheers,
Rafael

[PATCH v1.1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There are 3 places at which the maximum CPU frequency may change,
store_no_turbo(), intel_pstate_update_limits() (when called by the
cpufreq core) and intel_pstate_notify_work() (when handling a HWP
change notification).  Currently, cpuinfo.max_freq is only updated by
store_no_turbo() and intel_pstate_notify_work(), although it principle
it may be necessary to update it in intel_pstate_update_limits() either.

Make all of them mutually consistent.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v1.1: Fix changelog (Srinivas).

---
 drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
+	intel_pstate_get_hwp_cap(cpudata);
+
 	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
+
 	refresh_frequency_limits(policy);
 }
 
 static void intel_pstate_update_limits(unsigned int cpu)
 {
-	mutex_lock(&intel_pstate_driver_lock);
+	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
 
-	cpufreq_update_policy(cpu);
+	if (!policy)
+		return;
 
-	mutex_unlock(&intel_pstate_driver_lock);
+	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
+
+	cpufreq_cpu_release(policy);
+}
+
+static void intel_pstate_update_limits_for_all(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		intel_pstate_update_limits(cpu);
 }
 
 /************************** sysfs begin ************************/
@@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	intel_pstate_update_policies();
+	intel_pstate_update_limits_for_all();
 	arch_set_max_freq_ratio(no_turbo);
 
 unlock_driver:
@@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
 	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata->cpu);
 
 	if (policy) {
-		intel_pstate_get_hwp_cap(cpudata);
 		__intel_pstate_update_max_freq(cpudata, policy);
 
 		cpufreq_cpu_release(policy);

Re: [PATCH v1 1/6] cpufreq: intel_pstate: Fold intel_pstate_max_within_limits() into caller

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:01 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold intel_pstate_max_within_limits() into its only caller.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/cpufreq/intel_pstate.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2012,14 +2012,6 @@ static void intel_pstate_set_min_pstate(
>         intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate);
>  }
>  
> -static void intel_pstate_max_within_limits(struct cpudata *cpu)
> -{
> -       int pstate = max(cpu->pstate.min_pstate, cpu-
> >max_perf_ratio);
> -
> -       update_turbo_state();
> -       intel_pstate_set_pstate(cpu, pstate);
> -}
> -
>  static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
>  {
>         int perf_ctl_max_phys = pstate_funcs.get_max_physical(cpu-
> >cpu);
> @@ -2594,12 +2586,15 @@ static int intel_pstate_set_policy(struc
>         intel_pstate_update_perf_limits(cpu, policy->min, policy-
> >max);
>  
>         if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
> +               int pstate = max(cpu->pstate.min_pstate, cpu-
> >max_perf_ratio);
> +
>                 /*
>                  * NOHZ_FULL CPUs need this as the governor callback
> may not
>                  * be invoked on them.
>                  */
>                 intel_pstate_clear_update_util_hook(policy->cpu);
> -               intel_pstate_max_within_limits(cpu);
> +               update_turbo_state();
> +               intel_pstate_set_pstate(cpu, pstate);
>         } else {
>                 intel_pstate_set_update_util_hook(policy->cpu);
>         }
> 
> 
> 

Re: [PATCH v1 0/6] intel_pstate: Turbo disabled handling rework

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:00 +0100, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This series reworks the handling of disabling turbo in intel_pstate
> on top of the previous series of cleanups
> 
> https://lore.kernel.org/linux-pm/12409658.O9o76ZdvQC@kreacher/
> 
> The underlying problem is that disabling turbo is handled quite
> consistently
> in intel_pstate and basically it can get disabled at any time
> (through
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE) without much coordination with
> the
> cpufreq core or anything else.
> 
> Disabling turbo through the "no_turbo" sysfs attribute is more
> consistent,
> but it has issues too (for example, if turbo is disabled via
> "no_turbo",
> the frequency-invariance code gets notified on the turbo status
> change,
> but the actual maximum frequency of the CPU is only updated if the
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE value changes either, which need
> not
> happen at the same time or even at all).
> 
> The first patch is not really related to the rest of the series, it's
> just a cleanup and can be applied separately.
> 
> Patch [2/6] uses the observation that it should be necessary to read
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE after driver initialization to
> remove
> in-flight reads on that MSR and turbo state updates related to them.
> 
> Patch [3/6] builds on top of the previous one to adjust the
> "no_turbo"
> attribute "store" and "show" callbacks.
> 
> Patch [4/6] adds READ_ONCE() annotations to global.no_turbo accesses
> and
> makes some related simplifications.
> 
> Patch [5/6] replaces the cached MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> value in some checks with global.no_turbo for consistency.
> 
> Patch [6/6] makes all of the code paths where the maximum CPU
> frequency
> can change to do that consistently by using the same set of
> functions.
> 
> Details are described in the individual patch changelogs.
> 

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

> Thanks!
> 
> 
> 
>