cpufreq: intel_pstate: Use most recent guaranteed performance values
diff mbox series

Message ID 4600970.KDbqdY64fy@kreacher
State Accepted
Commit e40ad84c26b4deeee46666492ec66b9a534b8e59
Headers show
Series
  • cpufreq: intel_pstate: Use most recent guaranteed performance values
Related show

Commit Message

Rafael J. Wysocki Dec. 17, 2020, 7:17 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When turbo has been disabled by the BIOS, but HWP_CAP.GUARANTEED is
changed later, user space may want to take advantage of this increased
guaranteed performance.

HWP_CAP.GUARANTEED is not a static value.  It can be adjusted by an
out-of-band agent or during an Intel Speed Select performance level
change.  The HWP_CAP.MAX is still the maximum achievable performance
with turbo disabled by the BIOS, so HWP_CAP.GUARANTEED can still
change as long as it remains less than or equal to HWP_CAP.MAX.

When HWP_CAP.GUARANTEED is changed, the sysfs base_frequency
attribute shows the most recent guaranteed frequency value. This
attribute can be used by user space software to update the scaling
min/max limits of the CPU.

Currently, the ->setpolicy() callback already uses the latest
HWP_CAP values when setting HWP_REQ, but the ->verify() callback will
restrict the user settings to the to old guaranteed performance value
which prevents user space from making use of the extra CPU capacity
theoretically available to it after increasing HWP_CAP.GUARANTEED.

To address this, read HWP_CAP in intel_pstate_verify_cpu_policy()
to obtain the maximum P-state that can be used and use that to
confine the policy max limit instead of using the cached and
possibly stale pstate.max_freq value for this purpose.

For consistency, update intel_pstate_update_perf_limits() to use the
maximum available P-state returned by intel_pstate_get_hwp_max() to
compute the maximum frequency instead of using the return value of
intel_pstate_get_max_freq() which, again, may be stale.

This issue is a side-effect of fixing the scaling frequency limits in
commit eacc9c5a927e ("cpufreq: intel_pstate: Fix intel_pstate_get_hwp_max()
for turbo disabled") which currected the setting of the reduced scaling
frequency values, but caused stale HWP_CAP.GUARANTEED to be used in
the case at hand.

Fixes: eacc9c5a927e ("cpufreq: intel_pstate: Fix intel_pstate_get_hwp_max() for turbo disabled")
Reported-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: 5.8+ <stable@vger.kernel.org> # 5.8+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Srinivas Pandruvada Dec. 19, 2020, 5:19 a.m. UTC | #1
On Thu, 2020-12-17 at 20:17 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> When turbo has been disabled by the BIOS, but HWP_CAP.GUARANTEED is
> changed later, user space may want to take advantage of this
> increased
> guaranteed performance.
> 
> HWP_CAP.GUARANTEED is not a static value.  It can be adjusted by an
> out-of-band agent or during an Intel Speed Select performance level
> change.  The HWP_CAP.MAX is still the maximum achievable performance
> with turbo disabled by the BIOS, so HWP_CAP.GUARANTEED can still
> change as long as it remains less than or equal to HWP_CAP.MAX.
> 
> When HWP_CAP.GUARANTEED is changed, the sysfs base_frequency
> attribute shows the most recent guaranteed frequency value. This
> attribute can be used by user space software to update the scaling
> min/max limits of the CPU.
> 
> Currently, the ->setpolicy() callback already uses the latest
> HWP_CAP values when setting HWP_REQ, but the ->verify() callback will
> restrict the user settings to the to old guaranteed performance value
> which prevents user space from making use of the extra CPU capacity
> theoretically available to it after increasing HWP_CAP.GUARANTEED.
> 
> To address this, read HWP_CAP in intel_pstate_verify_cpu_policy()
> to obtain the maximum P-state that can be used and use that to
> confine the policy max limit instead of using the cached and
> possibly stale pstate.max_freq value for this purpose.
> 
> For consistency, update intel_pstate_update_perf_limits() to use the
> maximum available P-state returned by intel_pstate_get_hwp_max() to
> compute the maximum frequency instead of using the return value of
> intel_pstate_get_max_freq() which, again, may be stale.
> 
> This issue is a side-effect of fixing the scaling frequency limits in
> commit eacc9c5a927e ("cpufreq: intel_pstate: Fix
> intel_pstate_get_hwp_max()
> for turbo disabled") which currected 
corrected

Thanks,
Srinivas

> the setting of the reduced scaling
> frequency values, but caused stale HWP_CAP.GUARANTEED to be used in
> the case at hand.
> 
> Fixes: eacc9c5a927e ("cpufreq: intel_pstate: Fix
> intel_pstate_get_hwp_max() for turbo disabled")
> Reported-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com
> >
> Tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: 5.8+ <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2207,9 +2207,9 @@ static void intel_pstate_update_perf_lim
>  					    unsigned int policy_min,
>  					    unsigned int policy_max)
>  {
> -	int max_freq = intel_pstate_get_max_freq(cpu);
>  	int32_t max_policy_perf, min_policy_perf;
>  	int max_state, turbo_max;
> +	int max_freq;
>  
>  	/*
>  	 * HWP needs some special consideration, because on BDX the
> @@ -2223,6 +2223,7 @@ static void intel_pstate_update_perf_lim
>  			cpu->pstate.max_pstate : cpu-
> >pstate.turbo_pstate;
>  		turbo_max = cpu->pstate.turbo_pstate;
>  	}
> +	max_freq = max_state * cpu->pstate.scaling;
>  
>  	max_policy_perf = max_state * policy_max / max_freq;
>  	if (policy_max == policy_min) {
> @@ -2325,9 +2326,18 @@ static void intel_pstate_adjust_policy_m
>  static void intel_pstate_verify_cpu_policy(struct cpudata *cpu,
>  					   struct cpufreq_policy_data
> *policy)
>  {
> +	int max_freq;
> +
>  	update_turbo_state();
> -	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> -				     intel_pstate_get_max_freq(cpu));
> +	if (hwp_active) {
> +		int max_state, turbo_max;
> +
> +		intel_pstate_get_hwp_max(cpu->cpu, &turbo_max,
> &max_state);
> +		max_freq = max_state * cpu->pstate.scaling;
> +	} else {
> +		max_freq = intel_pstate_get_max_freq(cpu);
> +	}
> +	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> max_freq);
>  
>  	intel_pstate_adjust_policy_max(cpu, policy);
>  }
> 
> 
>
Rafael J. Wysocki Dec. 20, 2020, 4:41 p.m. UTC | #2
On Sat, Dec 19, 2020 at 6:21 AM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Thu, 2020-12-17 at 20:17 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > When turbo has been disabled by the BIOS, but HWP_CAP.GUARANTEED is
> > changed later, user space may want to take advantage of this
> > increased
> > guaranteed performance.
> >
> > HWP_CAP.GUARANTEED is not a static value.  It can be adjusted by an
> > out-of-band agent or during an Intel Speed Select performance level
> > change.  The HWP_CAP.MAX is still the maximum achievable performance
> > with turbo disabled by the BIOS, so HWP_CAP.GUARANTEED can still
> > change as long as it remains less than or equal to HWP_CAP.MAX.
> >
> > When HWP_CAP.GUARANTEED is changed, the sysfs base_frequency
> > attribute shows the most recent guaranteed frequency value. This
> > attribute can be used by user space software to update the scaling
> > min/max limits of the CPU.
> >
> > Currently, the ->setpolicy() callback already uses the latest
> > HWP_CAP values when setting HWP_REQ, but the ->verify() callback will
> > restrict the user settings to the to old guaranteed performance value
> > which prevents user space from making use of the extra CPU capacity
> > theoretically available to it after increasing HWP_CAP.GUARANTEED.
> >
> > To address this, read HWP_CAP in intel_pstate_verify_cpu_policy()
> > to obtain the maximum P-state that can be used and use that to
> > confine the policy max limit instead of using the cached and
> > possibly stale pstate.max_freq value for this purpose.
> >
> > For consistency, update intel_pstate_update_perf_limits() to use the
> > maximum available P-state returned by intel_pstate_get_hwp_max() to
> > compute the maximum frequency instead of using the return value of
> > intel_pstate_get_max_freq() which, again, may be stale.
> >
> > This issue is a side-effect of fixing the scaling frequency limits in
> > commit eacc9c5a927e ("cpufreq: intel_pstate: Fix
> > intel_pstate_get_hwp_max()
> > for turbo disabled") which currected
> corrected

Right, thanks!

Patch
diff mbox series

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2207,9 +2207,9 @@  static void intel_pstate_update_perf_lim
 					    unsigned int policy_min,
 					    unsigned int policy_max)
 {
-	int max_freq = intel_pstate_get_max_freq(cpu);
 	int32_t max_policy_perf, min_policy_perf;
 	int max_state, turbo_max;
+	int max_freq;
 
 	/*
 	 * HWP needs some special consideration, because on BDX the
@@ -2223,6 +2223,7 @@  static void intel_pstate_update_perf_lim
 			cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
 		turbo_max = cpu->pstate.turbo_pstate;
 	}
+	max_freq = max_state * cpu->pstate.scaling;
 
 	max_policy_perf = max_state * policy_max / max_freq;
 	if (policy_max == policy_min) {
@@ -2325,9 +2326,18 @@  static void intel_pstate_adjust_policy_m
 static void intel_pstate_verify_cpu_policy(struct cpudata *cpu,
 					   struct cpufreq_policy_data *policy)
 {
+	int max_freq;
+
 	update_turbo_state();
-	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
-				     intel_pstate_get_max_freq(cpu));
+	if (hwp_active) {
+		int max_state, turbo_max;
+
+		intel_pstate_get_hwp_max(cpu->cpu, &turbo_max, &max_state);
+		max_freq = max_state * cpu->pstate.scaling;
+	} else {
+		max_freq = intel_pstate_get_max_freq(cpu);
+	}
+	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq, max_freq);
 
 	intel_pstate_adjust_policy_max(cpu, policy);
 }