cpufreq: Allow drivers to receive more information from the governor

From: Rafael J. Wysocki <[email protected]>

Instead of passing util and max between functions while computing the utilization and capacity, store the former in struct sg_cpu (along with the latter and bw_dl).

This will allow the current utilization value to be compared with the one obtained previously (which is requisite for some code changes to follow this one), but also it makes the code look slightly more consistent and clean.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c |   42 ++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +1 -0

@@ -53,6 +53,7 @@ struct sugov_cpu {
 	unsigned int		iowait_boost;
 	u64			last_update;
 
+	unsigned long		util;
 	unsigned long		bw_dl;
 	unsigned long		max;
 

Quoted 28 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Instead of passing util and max between functions while computing the utilization and capacity, store the former in struct sg_cpu (along with the latter and bw_dl).

This will allow the current utilization value to be compared with the one obtained previously (which is requisite for some code changes to follow this one), but also it makes the code look slightly more consistent and clean.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c |   42 ++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +1 -0

@@ -53,6 +53,7 @@ struct sugov_cpu {
      unsigned int            iowait_boost;
      u64                     last_update;
 
+     unsigned long           util;
      unsigned long           bw_dl;
      unsigned long           max;
 

kernel/sched/cpufreq_schedutil.c unsigned long schedutil_cpu_util(int cpu · line 280

Quoted 6 lines

kernel/sched/cpufreq_schedutil.c +1 -1

@@ -276,16 +277,15 @@ unsigned long schedutil_cpu_util(int cpu
      return min(max, util);
 }
 
-static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
+static void sugov_get_util(struct sugov_cpu *sg_cpu)

Maybe name it sugov_update_util() ?

Otherwise,

Acked-by Viresh Kumar <[email protected]>

From: Rafael J. Wysocki <[email protected]>

When avoiding reduction of the frequency after the target CPU has been busy since the previous frequency update, adjust the utilization instead of adjusting the frequency, because doing so is more prudent (it is done to counter a possible utilization deficit after all) and it will allow some code to be shared after a subsequent change.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +1 -2

@@ -437,7 +437,7 @@ static void sugov_update_single(struct u
 {
 	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
 	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
-	unsigned int cached_freq = sg_policy->cached_raw_freq;
+	unsigned long prev_util = sg_cpu->util;
 	unsigned int next_f;
 
 	sugov_iowait_boost(sg_cpu, time, flags);

@@ -451,17 +451,14 @@ static void sugov_update_single(struct u
 	sugov_get_util(sg_cpu);
 	sugov_iowait_apply(sg_cpu, time);
 
-	next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
 	/*
 	 * Do not reduce the frequency if the CPU has not been idle
 	 * recently, as the reduction is likely to be premature then.

Quoted 34 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

When avoiding reduction of the frequency after the target CPU has been busy since the previous frequency update, adjust the utilization instead of adjusting the frequency, because doing so is more prudent (it is done to counter a possible utilization deficit after all) and it will allow some code to be shared after a subsequent change.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +1 -2

@@ -437,7 +437,7 @@ static void sugov_update_single(struct u
 {
      struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
      struct sugov_policy *sg_policy = sg_cpu->sg_policy;
-     unsigned int cached_freq = sg_policy->cached_raw_freq;
+     unsigned long prev_util = sg_cpu->util;
      unsigned int next_f;
 
      sugov_iowait_boost(sg_cpu, time, flags);

@@ -451,17 +451,14 @@ static void sugov_update_single(struct u
      sugov_get_util(sg_cpu);
      sugov_iowait_apply(sg_cpu, time);
 
-     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
      /*
       * Do not reduce the frequency if the CPU has not been idle
       * recently, as the reduction is likely to be premature then.

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct u · line 461

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +3 -5

       */
-     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
-             next_f = sg_policy->next_freq;
+     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
+             sg_cpu->util = prev_util;
 
-             /* Restore cached freq as next_freq has changed */
-             sg_policy->cached_raw_freq = cached_freq;
-     }
+     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);

I don't think we can replace freq comparison by util, or at least it will give us a different final frequency and the behavior is changed.

Lets take an example, lets say current freq is 1 GHz and max is 1024.

Round 1: Lets say util is 1000

next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz

Round 2: Lets say util has come down to 900 here,

before the patch:

next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz

after the patch:

next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz

Or did I make a mistake here ?

Quoted 60 lines · Viresh Kumar

Quoted 34 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

When avoiding reduction of the frequency after the target CPU has been busy since the previous frequency update, adjust the utilization instead of adjusting the frequency, because doing so is more prudent (it is done to counter a possible utilization deficit after all) and it will allow some code to be shared after a subsequent change.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +1 -2

@@ -437,7 +437,7 @@ static void sugov_update_single(struct u
 {
      struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
      struct sugov_policy *sg_policy = sg_cpu->sg_policy;
-     unsigned int cached_freq = sg_policy->cached_raw_freq;
+     unsigned long prev_util = sg_cpu->util;
      unsigned int next_f;
 
      sugov_iowait_boost(sg_cpu, time, flags);

@@ -451,17 +451,14 @@ static void sugov_update_single(struct u
      sugov_get_util(sg_cpu);
      sugov_iowait_apply(sg_cpu, time);
 
-     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
      /*
       * Do not reduce the frequency if the CPU has not been idle
       * recently, as the reduction is likely to be premature then.

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct u · line 461

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +3 -5

       */
-     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
-             next_f = sg_policy->next_freq;
+     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
+             sg_cpu->util = prev_util;
 
-             /* Restore cached freq as next_freq has changed */
-             sg_policy->cached_raw_freq = cached_freq;
-     }
+     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);

I don't think we can replace freq comparison by util, or at least it will give us a different final frequency and the behavior is changed.

Lets take an example, lets say current freq is 1 GHz and max is 1024.

Round 1: Lets say util is 1000

next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz

Round 2: Lets say util has come down to 900 here,

before the patch:

Quoted 7 lines

next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz

after the patch:

next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz

Or did I make a mistake here ?

I think so, if my understanding is correct.

Without the patch, next_f will be reset to the previous value (sq_policy->next_freq) if the CPU has been busy and the (new) next_f is less than that value.

So the "new" next_f before the patch is 1.31 GHz, but because it is less than the previous value (1.45 GHz), it will be reset to that value, unless I'm missing something.

Overall, the patch doesn't change the logic AFAICS and because the util->freq mapping is linear, all of the inequalities map exactly from one to the other (both ways).

Quoted 51 lines · Rafael J. Wysocki

Quoted 50 lines · Viresh Kumar

Quoted 34 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

When avoiding reduction of the frequency after the target CPU has been busy since the previous frequency update, adjust the utilization instead of adjusting the frequency, because doing so is more prudent (it is done to counter a possible utilization deficit after all) and it will allow some code to be shared after a subsequent change.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================

kernel/sched/cpufreq_schedutil.c +1 -2

@@ -437,7 +437,7 @@ static void sugov_update_single(struct u
 {
      struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
      struct sugov_policy *sg_policy = sg_cpu->sg_policy;
-     unsigned int cached_freq = sg_policy->cached_raw_freq;
+     unsigned long prev_util = sg_cpu->util;
      unsigned int next_f;
 
      sugov_iowait_boost(sg_cpu, time, flags);

@@ -451,17 +451,14 @@ static void sugov_update_single(struct u
      sugov_get_util(sg_cpu);
      sugov_iowait_apply(sg_cpu, time);
 
-     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
      /*
       * Do not reduce the frequency if the CPU has not been idle
       * recently, as the reduction is likely to be premature then.

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct u · line 461

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +3 -5

       */
-     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
-             next_f = sg_policy->next_freq;
+     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
+             sg_cpu->util = prev_util;
 
-             /* Restore cached freq as next_freq has changed */
-             sg_policy->cached_raw_freq = cached_freq;
-     }
+     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);

I don't think we can replace freq comparison by util, or at least it will give us a different final frequency and the behavior is changed.

Quoted 1 line

Quoted 1 line

Lets take an example, lets say current freq is 1 GHz and max is 1024.

Quoted 22 lines

Quoted 8 lines

Round 1: Lets say util is 1000

next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz

Round 2: Lets say util has come down to 900 here,

before the patch:

Quoted 7 lines

next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz

after the patch:

next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz

Or did I make a mistake here ?

I think so, if my understanding is correct.

Without the patch, next_f will be reset to the previous value (sq_policy->next_freq) if the CPU has been busy and the (new) next_f is less than that value.

Quoted 3 lines

So the "new" next_f before the patch is 1.31 GHz, but because it is less than the previous value (1.45 GHz), it will be reset to that value, unless I'm missing something.

The prev frequency here was 1.2 GHz (after Round 1). 1.45 GHz is the value we get after this patch, as we take the earlier utilization (1000) into account instead of 900.

So I have misunderstood your example.

In the non-invariant case (which is or shortly will be relevant for everybody interested) cpuinfo.max_freq goes into the calculation instead of the current frequency and the mapping between util and freq is linear. In the freq-dependent case it is not linear, of course.

So I guess the concern is that this changes the behavior in the freq-dependent case which may not be desirable.

Fair enough, but I'm not sure if that is enough of a reason to avoid sharing the code between the "perf" and "freq" paths.

From: Rafael J. Wysocki <[email protected]>

First off, some cpufreq drivers (eg. intel_pstate) can pass hints beyond the current target frequency to the hardware and there are no provisions for doing that in the cpufreq framework. In particular, today the driver has to assume that it should not allow the frequency to fall below the one requested by the governor (or the required capacity may not be provided) which may not be the case and which may lead to excessive energy usage in some scenarios.

Second, the hints passed by these drivers to the hardware need not be in terms of the frequency, so representing the utilization numbers coming from the scheduler as frequency before passing them to those drivers is not really useful.

Address the two points above by adding a special-purpose replacement for the ->fast_switch callback, called ->adjust_perf, allowing the governor to pass abstract performance level (rather than frequency) values for the minimum (required) and target (desired) performance along with the CPU capacity to compare them to.

Also update the schedutil governor to use the new callback instead of ->fast_switch if present.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Changes with respect to the RFC:
- Don't pass "busy" to ->adjust_perf().
- Use a special 'update_util' hook for the ->adjust_perf() case in schedutil (this still requires an additional branch because of the shared common code between this case and the "frequency" one, but IMV this version is cleaner nevertheless).

---
 drivers/cpufreq/cpufreq.c        |   40 ++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h          |   14 +++++++++++
 include/linux/sched/cpufreq.h    |    5 ++++
 kernel/sched/cpufreq_schedutil.c |   48 +++++++++++++++++++++++++++++++--------
 4 files changed, 98 insertions(+), 9 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================

include/linux/cpufreq.h +0 -0

@@ -320,6 +320,15 @@ struct cpufreq_driver {
 					unsigned int index);
 	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,

Quoted 53 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

First off, some cpufreq drivers (eg. intel_pstate) can pass hints beyond the current target frequency to the hardware and there are no provisions for doing that in the cpufreq framework. In particular, today the driver has to assume that it should not allow the frequency to fall below the one requested by the governor (or the required capacity may not be provided) which may not be the case and which may lead to excessive energy usage in some scenarios.

Second, the hints passed by these drivers to the hardware need not be in terms of the frequency, so representing the utilization numbers coming from the scheduler as frequency before passing them to those drivers is not really useful.

Address the two points above by adding a special-purpose replacement for the ->fast_switch callback, called ->adjust_perf, allowing the governor to pass abstract performance level (rather than frequency) values for the minimum (required) and target (desired) performance along with the CPU capacity to compare them to.

Also update the schedutil governor to use the new callback instead of ->fast_switch if present.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Changes with respect to the RFC:
- Don't pass "busy" to ->adjust_perf().
- Use a special 'update_util' hook for the ->adjust_perf() case in schedutil (this still requires an additional branch because of the shared common code between this case and the "frequency" one, but IMV this version is cleaner nevertheless).

---
 drivers/cpufreq/cpufreq.c        |   40 ++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h          |   14 +++++++++++
 include/linux/sched/cpufreq.h    |    5 ++++
 kernel/sched/cpufreq_schedutil.c |   48 +++++++++++++++++++++++++++++++--------
 4 files changed, 98 insertions(+), 9 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================

include/linux/cpufreq.h +4 -0

@@ -320,6 +320,15 @@ struct cpufreq_driver {
 					unsigned int index);
 	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
 				       unsigned int target_freq);
+	/*
+	 * ->fast_switch() replacement for drivers that use an internal
+	 * representation of performance levels and can pass hints other than
+	 * the target performance level to the hardware.

include/linux/cpufreq.h struct cpufreq_driver { · line 331

Quoted 5 lines

include/linux/cpufreq.h +5 -0

+	 */
+	void		(*adjust_perf)(unsigned int cpu,
+				       unsigned long min_perf,
+				       unsigned long target_perf,
+				       unsigned long capacity);

With this callback in place, do we still need to keep the other stuff we introduced recently, like CPUFREQ_NEED_UPDATE_LIMITS ?

Quoted 54 lines · Viresh Kumar

Quoted 53 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

First off, some cpufreq drivers (eg. intel_pstate) can pass hints beyond the current target frequency to the hardware and there are no provisions for doing that in the cpufreq framework. In particular, today the driver has to assume that it should not allow the frequency to fall below the one requested by the governor (or the required capacity may not be provided) which may not be the case and which may lead to excessive energy usage in some scenarios.

Second, the hints passed by these drivers to the hardware need not be in terms of the frequency, so representing the utilization numbers coming from the scheduler as frequency before passing them to those drivers is not really useful.

Address the two points above by adding a special-purpose replacement for the ->fast_switch callback, called ->adjust_perf, allowing the governor to pass abstract performance level (rather than frequency) values for the minimum (required) and target (desired) performance along with the CPU capacity to compare them to.

Also update the schedutil governor to use the new callback instead of ->fast_switch if present.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Changes with respect to the RFC:
- Don't pass "busy" to ->adjust_perf().
- Use a special 'update_util' hook for the ->adjust_perf() case in schedutil (this still requires an additional branch because of the shared common code between this case and the "frequency" one, but IMV this version is cleaner nevertheless).

---
 drivers/cpufreq/cpufreq.c        |   40 ++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h          |   14 +++++++++++
 include/linux/sched/cpufreq.h    |    5 ++++
 kernel/sched/cpufreq_schedutil.c |   48 +++++++++++++++++++++++++++++++--------
 4 files changed, 98 insertions(+), 9 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================

include/linux/cpufreq.h +4 -0

@@ -320,6 +320,15 @@ struct cpufreq_driver {
                                    unsigned int index);
    unsigned int    (*fast_switch)(struct cpufreq_policy *policy,
                                   unsigned int target_freq);
+   /*
+    * ->fast_switch() replacement for drivers that use an internal
+    * representation of performance levels and can pass hints other than
+    * the target performance level to the hardware.

Quoted 8 lines

include/linux/cpufreq.h struct cpufreq_driver { · line 331

Quoted 5 lines

include/linux/cpufreq.h +5 -0

+    */
+   void            (*adjust_perf)(unsigned int cpu,
+                                  unsigned long min_perf,
+                                  unsigned long target_perf,
+                                  unsigned long capacity);

With this callback in place, do we still need to keep the other stuff we introduced recently, like CPUFREQ_NEED_UPDATE_LIMITS ?

Ping

drivers/cpufreq/intel_pstate.c +26 -0

+static void intel_cpufreq_adjust_perf(unsigned int cpunum,
+				      unsigned long min_perf,
+				      unsigned long target_perf,
+				      unsigned long capacity)
+{
+	struct cpudata *cpu = all_cpu_data[cpunum];
+	int old_pstate = cpu->pstate.current_pstate;
+	int cap_pstate, min_pstate, max_pstate, target_pstate;
+
+	update_turbo_state();
+	cap_pstate = global.turbo_disabled ? cpu->pstate.max_pstate :
+					     cpu->pstate.turbo_pstate;
+
+	/* Optimization: Avoid unnecessary divisions. */
+
+	target_pstate = cap_pstate;
+	if (target_perf < capacity)
+		target_pstate = DIV_ROUND_UP(cap_pstate * target_perf, capacity);
+
+	min_pstate = cap_pstate;
+	if (min_perf < capacity)
+		min_pstate = DIV_ROUND_UP(cap_pstate * min_perf, capacity);
+
+	if (min_pstate < cpu->pstate.min_pstate)
+		min_pstate = cpu->pstate.min_pstate;
+

drivers/cpufreq/intel_pstate.c +4 -0

+
+	cpu->pstate.current_pstate = target_pstate;
+	intel_cpufreq_trace(cpu, INTEL_PSTATE_TRACE_FAST_SWITCH, old_pstate);
+}

Quoted 31 lines · Rafael J. Wysocki

drivers/cpufreq/intel_pstate.c +31 -0

+static void intel_cpufreq_adjust_perf(unsigned int cpunum,
+                                   unsigned long min_perf,
+                                   unsigned long target_perf,
+                                   unsigned long capacity)
+{
+     struct cpudata *cpu = all_cpu_data[cpunum];
+     int old_pstate = cpu->pstate.current_pstate;
+     int cap_pstate, min_pstate, max_pstate, target_pstate;
+
+     update_turbo_state();
+     cap_pstate = global.turbo_disabled ? cpu->pstate.max_pstate :
+                                          cpu->pstate.turbo_pstate;
+
+     /* Optimization: Avoid unnecessary divisions. */
+
+     target_pstate = cap_pstate;
+     if (target_perf < capacity)
+             target_pstate = DIV_ROUND_UP(cap_pstate * target_perf, capacity);
+
+     min_pstate = cap_pstate;
+     if (min_perf < capacity)
+             min_pstate = DIV_ROUND_UP(cap_pstate * min_perf, capacity);
+
+     if (min_pstate < cpu->pstate.min_pstate)
+             min_pstate = cpu->pstate.min_pstate;
+
+     if (min_pstate < cpu->min_perf_ratio)
+             min_pstate = cpu->min_perf_ratio;
+
+     max_pstate = min(cap_pstate, cpu->max_perf_ratio);
+     if (max_pstate < min_pstate)

drivers/cpufreq/intel_pstate.c static unsigned int intel_cpufreq_fast_s · line 2675

Quoted 5 lines

drivers/cpufreq/intel_pstate.c +5 -0

+             max_pstate = min_pstate;
+
+     target_pstate = clamp_t(int, target_pstate, min_pstate, max_pstate);
+
+     intel_cpufreq_adjust_hwp(cpu, min_pstate, max_pstate, target_pstate, true);

I'm confused... HWP doesn't do pstate, yet everything here is now called pstate, help?

Quoted 4 lines

drivers/cpufreq/intel_pstate.c +4 -0

+
+     cpu->pstate.current_pstate = target_pstate;
+     intel_cpufreq_trace(cpu, INTEL_PSTATE_TRACE_FAST_SWITCH, old_pstate);
+}

Hi,

This is based on the RFC posted a few days ago:

https://lore.kernel.org/linux-pm/1817571.2o5Kk4Ohv2@kreacher/

The majority of the original cover letter still applies, so let me quote it here:
[...]

Quoted 9 lines · Rafael J. Wysocki

Hi,

This is based on the RFC posted a few days ago:

https://lore.kernel.org/linux-pm/1817571.2o5Kk4Ohv2@kreacher/

The majority of the original cover letter still applies, so let me quote it here:
[...]

Hello Rafael,

I'd like to test this patch, as I have concerns on how it performs against the current intel_pstate passive + HWP + schedutil (which leaves HWP.REQ.DESIRED untouched).

I'll get results within a week. Do you mind holding back the merge until then?

Quoted 11 lines · Giovanni Gherdovich

Quoted 9 lines · Rafael J. Wysocki

Hi,

This is based on the RFC posted a few days ago:

https://lore.kernel.org/linux-pm/1817571.2o5Kk4Ohv2@kreacher/

The majority of the original cover letter still applies, so let me quote it here:
[...]

Quoted 5 lines

Hello Rafael,

I'd like to test this patch, as I have concerns on how it performs against the current intel_pstate passive + HWP + schedutil (which leaves HWP.REQ.DESIRED untouched).

The performance will be worse in some cases, which is to be expected,

but the point here is to counter the problem that in some cases the frequency is excessive now.

Quoted 2 lines

I'll get results within a week. Do you mind holding back the merge until then?

Quoted 7 lines · On 2020.12.08 09:14 Rafael J. Wysocki wrote:

Quoted 4 lines · Giovanni Gherdovich

On Mon, 2020-12-07 at 17:25 +0100, Rafael J. Wysocki wrote:
I'd like to test this patch, as I have concerns on how it performs against the current intel_pstate passive + HWP + schedutil (which leaves HWP.REQ.DESIRED untouched).

The performance will be worse in some cases, which is to be expected,

Agreed. More further below.

Quoted 2 lines

but the point here is to counter the problem that in some cases the frequency is excessive now.

Agreed.

The "hwp" case is misleading. It is really bouncing around at about 0.2 hertz and can't seem to make up its mind.

If nothing else, all the other versions are a little more deterministic in their response.

Quoted 3 lines

Quoted 3 lines

I'll get results within a week. Do you mind holding back the merge until then?

On my system that git "make test" is broken, so I can not do the high PIDs per second test that way. My own PIDs per second test also has issues on this computer.
I am not sure when I'll get these type of tests working, but will try for within a week.

At least on my system, it is most evident for some of the pipe type tests, where the schedutil governor has never really known what to do. This patch set seems to add enough of a downward bias that this version of the schedutil governor now behaves much like the other versions

This is based on the RFC posted a few days ago:

https://lore.kernel.org/linux-pm/1817571.2o5Kk4Ohv2@kreacher/

Using intel_pstate in the passive mode with HWP enabled, in particular under the schedutil governor, is still kind of problematic, because it has to assume that it should not allow the frequency to fall below the one requested by the governor. For this reason, it translates the target frequency into HWP.REQ.MIN which generally causes the processor to run a bit too fast.

Moreover, this allows the HWP algorithm to use any frequency between the target one and HWP.REQ.MAX that corresponds to the policy max limit and some workloads cause it to go for the max turbo frequency prematurely which hurts energy- efficiency without improving performance, even though the schedutil governor itself would not allow the frequency to ramp up so fast.

This patch series attempts to improve the situation by introducing a new driver callback allowing the driver to receive more information from the governor. In particular, this allows the min (required) and target (desired) performance levels to be passed to it and those can be used to give better hints to the hardware.

From: Rafael J. Wysocki <[email protected]>

Make intel_pstate expose the ->adjust_perf() callback when it operates in the passive mode with HWP enabled which causes the schedutil governor to use that callback instead of ->fast_switch().

The minimum and target performance-level values passed by the governor to ->adjust_perf() are converted to HWP.REQ.MIN and HWP.REQ.DESIRED, respectively, which allows the processor to adjust its configuration to maximize energy-efficiency while providing sufficient capacity.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v1 -> v2:
 - No changes.

---
 drivers/cpufreq/intel_pstate.c |   70
+++++++++++++++++++++++++++++++++--------

 1 file changed, 58 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================

drivers/cpufreq/intel_pstate.c +0 -0

@@ -2526,20 +2526,19 @@ static void intel_cpufreq_trace(struct c
                 fp_toint(cpu->iowait_boost * 100));
  }

-static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 target_pstate,
-                                    bool strict, bool fast_switch) +static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 min, u32 max,
+                                    u32 desired, bool fast_switch) {
        u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev;
        value &= ~HWP_MIN_PERF(~0L);
-       value |= HWP_MIN_PERF(target_pstate);
+       value |= HWP_MIN_PERF(min);

-       /*
-        * The entire MSR needs to be updated in order to update the HWP min
-        * field in it, so opportunistically update the max too if needed.
-        */
        value &= ~HWP_MAX_PERF(~0L);
-       value |= HWP_MAX_PERF(strict ? target_pstate : cpu-

Quoted 1 line

max_perf_ratio);

+       value |= HWP_MAX_PERF(max);
+
+       value &= ~HWP_DESIRED_PERF(~0L);
+       value |= HWP_DESIRED_PERF(desired);

        if (value == prev)
                return;

drivers/cpufreq/intel_pstate.c +0 -0

@@ -2569,11 +2568,15 @@ static int intel_cpufreq_update_pstate(s
         int old_pstate = cpu->pstate.current_pstate;

        target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
-       if (hwp_active)
-               intel_cpufreq_adjust_hwp(cpu, target_pstate, -                                        policy->strict_target, fast_switch);
-       else if (target_pstate != old_pstate)
+       if (hwp_active) {
+               int max_pstate = policy->strict_target ? +                                       target_pstate : cpu-

Quoted 1 line

max_perf_ratio;

+
+               intel_cpufreq_adjust_hwp(cpu, target_pstate, max_pstate, 0,
+                                        fast_switch); +       } else if (target_pstate != old_pstate) { intel_cpufreq_adjust_perf_ctl(cpu, target_pstate, fast_switch);
+       }

        cpu->pstate.current_pstate = target_pstate;

drivers/cpufreq/intel_pstate.c +0 -0

@@ -2634,6 +2637,47 @@ static unsigned int intel_cpufreq_fast_s
         return target_pstate * cpu->pstate.scaling;
  }

drivers/cpufreq/intel_pstate.c +12 -0

+static void intel_cpufreq_adjust_perf(unsigned int cpunum,
+                                     unsigned long min_perf,
+                                     unsigned long target_perf,
+                                     unsigned long capacity)
+{
+       struct cpudata *cpu = all_cpu_data[cpunum];
+       int old_pstate = cpu->pstate.current_pstate;
+       int cap_pstate, min_pstate, max_pstate, target_pstate;
+
+       update_turbo_state();
+       cap_pstate = global.turbo_disabled ? cpu->pstate.max_pstate :
+                                            cpu-

Quoted 1 line

pstate.turbo_pstate;

drivers/cpufreq/intel_pstate.c +6 -0

+
+       /* Optimization: Avoid unnecessary divisions. */
+
+       target_pstate = cap_pstate;
+       if (target_perf < capacity)
+               target_pstate = DIV_ROUND_UP(cap_pstate *

target_perf, capacity);
+
+       min_pstate = cap_pstate;
+       if (min_perf < capacity)
+               min_pstate = DIV_ROUND_UP(cap_pstate * min_perf, capacity);

drivers/cpufreq/intel_pstate.c +12 -0

+
+       if (min_pstate < cpu->pstate.min_pstate)
+               min_pstate = cpu->pstate.min_pstate;
+
+       if (min_pstate < cpu->min_perf_ratio)
+               min_pstate = cpu->min_perf_ratio;
+
+       max_pstate = min(cap_pstate, cpu->max_perf_ratio);
+       if (max_pstate < min_pstate)
+               max_pstate = min_pstate;
+
+       target_pstate = clamp_t(int, target_pstate, min_pstate,

max_pstate);
+
+       intel_cpufreq_adjust_hwp(cpu, min_pstate, max_pstate, target_pstate, true);
+
+       cpu->pstate.current_pstate = target_pstate;
+       intel_cpufreq_trace(cpu, INTEL_PSTATE_TRACE_FAST_SWITCH, old_pstate);

drivers/cpufreq/intel_pstate.c +2 -0

+}
+

 static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy) {
        int max_state, turbo_max, min_freq, max_freq, ret;

drivers/cpufreq/intel_pstate.c +0 -0

@@ -3032,6 +3076,8 @@ static int __init intel_pstate_init(void
                         intel_pstate.attr = hwp_cpufreq_attrs;
                         intel_cpufreq.attr = hwp_cpufreq_attrs;

                        intel_cpufreq.flags |= CPUFREQ_NEED_UPDATE_LIMITS;
+                       intel_cpufreq.fast_switch = NULL; +                       intel_cpufreq.adjust_perf = intel_cpufreq_adjust_perf;
                        if (!default_driver) default_driver = &intel_pstate;

Hi,

The timing of this is not perfect (sorry about that), but here's a refresh of this series.

The majority of the previous cover letter still applies:

Quoted 17 lines · CET Rafael J. Wysocki

This is based on the RFC posted a few days ago:

https://lore.kernel.org/linux-pm/1817571.2o5Kk4Ohv2@kreacher/

Using intel_pstate in the passive mode with HWP enabled, in particular under the schedutil governor, is still kind of problematic, because it has to assume that it should not allow the frequency to fall below the one requested by the governor. For this reason, it translates the target frequency into HWP.REQ.MIN which generally causes the processor to run a bit too fast.

Moreover, this allows the HWP algorithm to use any frequency between the target one and HWP.REQ.MAX that corresponds to the policy max limit and some workloads cause it to go for the max turbo frequency prematurely which hurts energy- efficiency without improving performance, even though the schedutil governor itself would not allow the frequency to ramp up so fast.

Quoted 5 lines

This patch series attempts to improve the situation by introducing a new driver callback allowing the driver to receive more information from the governor. In particular, this allows the min (required) and target (desired) performance levels to be passed to it and those can be used to give better hints to the hardware.

In this second revision there are three patches (one preparatory patch for schedutil that hasn't changed since the v1, the introduction of the new callback and schedutil changes in patch [2/3] and the intel_pstate changes in patch [3/3] that are the same as before.

Please see patch changelogs for details.

Hi,

Quoted 1 line · On 2020.12.14 12:02 Rafael J. Wysocki wrote:

Hi,

Hi Rafael,

V2 test results below are new, other results are partially re-stated:

For readers that do not want to read on, I didn't find anything different than with the other versions. This was more just due diligence.

Legend:

hwp: Kernel 5.10-rc6, HWP enabled; intel_cpufreq
rfc (or rjw): Kernel 5.10-rc6 + this patch set, HWP enabled; intel_cpu-freq; schedutil
no-hwp: Kernel 5.10-rc6, HWP disabled; intel_cpu-freq
acpi (or acpi-cpufreq): Kernel 5.10-rc6, HWP disabled; acpi-cpufreq; schedutil
patch: Kernel 5.10-rc7 + V1 patch set, HWP enabled; intel_cpu-freq; schedutil
v2: Kernel 5.10-rc7 + V2 patch set, HWP enabled; intel_cpu-freq; schedutil

Fixed work packet, fixed period, periodic workflow, load sweep up/down:

load work/sleep frequency: 73 Hertz:

hwp: Average: 12.00822 watts
rjw: Average: 10.18089 watts
no-hwp: Average: 10.21947 watts
acpi-cpufreq: Average: 9.06585 watts
patch: Average: 10.26060 watts
v2: Average: 10.50444

load work/sleep frequency: 113 Hertz:

hwp: Average: 12.01056
rjw: Average: 10.12303
no-hwp: Average: 10.08228
acpi-cpufreq: Average: 9.02215
patch: Average: 10.27055
v2: Average: 10.31097

load work/sleep frequency: 211 Hertz:

hwp: Average: 12.16067
rjw: Average: 10.24413
no-hwp: Average: 10.12463
acpi-cpufreq: Average: 9.19175
patch: Average: 10.33000
v2: Average: 10.39811

load work/sleep frequency: 347 Hertz:

hwp: Average: 12.34169
rjw: Average: 10.79980
no-hwp: Average: 10.57296
acpi-cpufreq: Average: 9.84709
patch: Average: 10.67029
v2: Average: 10.93143

load work/sleep frequency: 401 Hertz:

hwp: Average: 12.42562
rjw: Average: 11.12465
no-hwp: Average: 11.24203
acpi-cpufreq: Average: 10.78670
patch: Average: 10.94514
v2: Average: 11.50324

Serialized single threaded via PIDs per second method:
A.K.A fixed work packet, variable period Results:

Execution times (seconds. Less is better):

no-hwp:

performance: Samples: 382 ; Average: 10.54450 ; Stand Deviation: 0.01564 ; Maximum: 10.61000 ; Minimum: 10.50000

schedutil: Samples: 293 ; Average: 13.73416 ; Stand Deviation: 0.73395 ; Maximum: 15.46000 ; Minimum: 11.68000
acpi: Samples: 253 ; Average: 15.94889 ; Stand Deviation: 1.28219 ; Maximum: 18.66000 ; Minimum: 12.04000

hwp:

schedutil: Samples: 380 ; Average: 10.58287 ; Stand Deviation: 0.01864 ; Maximum: 10.64000 ; Minimum: 10.54000
patch: Samples: 276 ; Average: 14.57029 ; Stand Deviation: 0.89771 ; Maximum: 16.04000 ; Minimum: 11.68000
rfc: Samples: 271 ; Average: 14.86037 ; Stand Deviation: 0.84164 ; Maximum: 16.04000 ; Minimum: 12.21000
v2: Samples: 274 ; Average: 14.67978 ; Stand Deviation: 1.03378 ; Maximum: 16.07000 ; Minimum: 11.43000

Power (watts. More indicates higher CPU frequency and better performance. Sample time = 1 second.):

no-hwp:

performance: Samples: 4000 ; Average: 25.41355 ; Stand Deviation: 0.22156 ; Maximum: 26.01996 ; Minimum: 24.08807

schedutil: Samples: 4000 ; Average: 12.58863 ; Stand Deviation: 5.48600 ; Maximum: 25.50934 ; Minimum: 7.54559
acpi: Samples: 4000 ; Average: 9.57924 ; Stand Deviation: 5.41157 ; Maximum: 25.06366 ; Minimum: 5.51129

hwp:

schedutil: Samples: 4000 ; Average: 25.24245 ; Stand Deviation: 0.19539 ; Maximum: 25.93671 ; Minimum: 24.14746
patch: Samples: 4000 ; Average: 11.07225 ; Stand Deviation: 5.63142 ; Maximum: 24.99493 ; Minimum: 3.67548
rfc: Samples: 4000 ; Average: 10.35842 ; Stand Deviation: 4.77915 ; Maximum: 24.95953 ; Minimum: 7.26202
v2: Samples: 4000 ; Average: 10.98284 ; Stand Deviation: 5.48859 ; Maximum: 25.76331 ; Minimum: 7.53790

Hi,

The timing of this is not perfect (sorry about that), but here's a refresh of this series.

The majority of the previous cover letter still applies:
[...]

Quoted 7 lines · Rafael J. Wysocki

Hi,

The timing of this is not perfect (sorry about that), but here's a refresh of this series.

The majority of the previous cover letter still applies:
[...]

Hello,

the series is tested using

-> tbench (packets processing with loopback networking, measures throughput) -> dbench (filesystem operations, measures average latency)
-> kernbench (kernel compilation, elapsed time)
-> and gitsource (long-running shell script, elapsed time)

These are chosen because none of them is bound by compute and all are sensitive to freq scaling decisions. The machines are a Cascade Lake based server, a client Skylake and a Coffee Lake laptop.

What's being compared:

sugov-HWP.desired : the present series; intel_pstate=passive, governor=schedutil
sugov-HWP.min : mainline; intel_pstate=passive, governor=schedutil
powersave-HWP : mainline; intel_pstate=active, governor=powersave
perfgov-HWP : mainline; intel_pstate=active, governor=performance
sugov-no-HWP : HWP disabled; intel_pstate=passive, governor=schedutil

Dbench and Kernbench have neutral results, but Tbench has sugov-HWP.desired lose in both performance and performance-per-watt, while Gitsource show the series as faster in raw performance but again worse than the competition in efficiency.

1. SUMMARY BY BENCHMARK
1.1. TBENCH
1.2. DBENCH
1.3. KERNBENCH
1.4. GITSOURCE
2. SUMMARY BY USER PROFILE
2.1. PERFORMANCE USER: what if I switch pergov -> schedutil? 2.2. DEFAULT USER: what if I switch powersave -> schedutil? 2.3. DEVELOPER: what if I switch sugov-HWP.min -> sugov-HWP.desired?
3. RESULTS TABLES
PERFORMANCE RATIOS
PERFORMANCE-PER-WATT RATIOS

1. SUMMARY BY BENCHMARK
~~~~~~~~~~~~~~~~~~~~~~~

Tbench: sugov-HWP.desired is the worst performance on all three
machines. sugov-HWP.min is between 20% and 90% better. The baseline sugov-HWP.desired offers a lower throughput, but does it increase efficiency? It actually doesn't: on two out of three machines the incumbent code (current sugov, or intel_pstate=active) has 10% to 35% better efficiency. In other word, the status quo is both faster and more efficient than the proposed series on this benchmark.
The absolute power consumption is lower, but the delivered performance is "even more lower", and that's why performance-per-watt shows a net loss.

Dbench: generally neutral, in both performance and efficiency. Powersave is
occasionally behind the pack in performance, 5% to 15%. A 15% performance loss on the Coffe Lake is compensated by an 80% improved efficiency. To be noted that on the same Coffee Lake sugov-no-HWP is 20% ahead of the pack in efficiency.

Kernbench: neutral, in both performance and efficiency. powersave looses 14%
to the pack in performance on the Cascade Lake.

Gitsource: this test show the most compelling case against the
sugov-HWP.desired series: on the Cascade Lake sugov-HWP.desired is 10% faster than sugov-HWP.min (it was expected to be slower!) and 35% less efficient (we expected more performance-per-watt, not less).

2. SUMMARY BY USER PROFILE
~~~~~~~~~~~~~~~~~~~~~~~~~~

If I was a perfgov-HWP user, I would be 20%-90% faster than with other governors on tbench an gitsource. This speed gap comes with an unexpected efficiency bonus on both test. Since dbench and kernbench have a flat profile across the board, there is no incentive to try another governor.

If I was a powersave-HWP user, I'd be the slower of the bunch. The lost performance is not, in general, balanced by better efficiency. This only happens on Coffee Lake, which is a CPU for the mobile market and possibly HWP has efficiency-oriented tuning there. Any flavor of schedutil would be an improvement.

From a developer perspective, the obstacles to move from HWP.min to HWP.desired are tbench, where HWP.desired is worse than having no HWP support at all, and gitsource, where HWP.desired has the opposite properties than those advertised (it's actually faster but less efficient).

3. RESULTS TABLES
~~~~~~~~~~~~~~~~~

Tilde (~) means the result is the same as baseline (or, the ratio is close to 1). The double asterisk (**) is a visual aid and means the result is better than baseline (higher or lower depending on the case).

| 80x_CASCADELAKE_NUMA: Intel Cascade Lake, 40 cores / 80 threads, NUMA, SATA SSD storage
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| sugov-HWP.des sugov-HWP.min powersave-HWP perfgov-HWP sugov-no-HWP better if
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

|                                         PERFORMANCE RATIOS
| tbench         1.00           1.89**         1.88**        1.89**        1.17**       higher
| dbench         1.00           ~              1.06          ~             ~            lower
| kernbench      1.00           ~              1.14          ~             ~            lower
| gitsource      1.00           1.11           2.70          0.80**        ~            lower
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|                                    PERFORMANCE-PER-WATT RATIOS
| tbench         1.00           1.36**         1.38**        1.33**        1.04**       higher
| dbench         1.00           ~              ~             ~             ~            higher
| kernbench      1.00           ~              ~             ~             ~            higher
| gitsource      1.00           1.36**         0.63          1.22**        1.02**       higher

| 8x_COFFEELAKE_UMA: Intel Coffee Lake, 4 cores / 8 threads, UMA, NVMe SSD storage
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| sugov-HWP.des sugov-HWP.min powersave-HWP perfgov-HWP sugov-no-HWP better if
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

|                                         PERFORMANCE RATIOS
| tbench         1.00           1.27**         1.30**        1.30**        1.31**       higher
| dbench         1.00           ~              1.15          ~             ~            lower
| kernbench      1.00           ~              ~             ~             ~            lower
| gitsource      1.00           ~              2.09          ~             ~            lower
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|                                    PERFORMANCE-PER-WATT RATIOS
| tbench         1.00           ~              ~             ~             ~            higher
| dbench         1.00           ~              1.82**        ~             1.22**       higher
| kernbench      1.00           ~              ~             ~             ~            higher
| gitsource      1.00           ~              1.56**        ~             1.17**       higher

| 8x_SKYLAKE_UMA: Intel Skylake (client), 4 cores / 8 threads, UMA, SATA SSD storage
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| sugov-HWP.des sugov-HWP.min powersave-HWP perfgov-HWP sugov-no-HWP better if
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

|                                         PERFORMANCE RATIOS
| tbench         1.00           1.21**         1.22**        1.20**        1.06**       higher
| dbench         1.00           ~              ~             ~             ~            lower
| kernbench      1.00           ~              ~             ~             ~            lower
| gitsource      1.00           ~              1.71          0.96**        ~            lower
+ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|                                    PERFORMANCE-PER-WATT RATIOS
| tbench         1.00           1.11**         1.12**        1.10**        1.03**       higher
| dbench         1.00           ~              ~             ~             ~            higher
| kernbench      1.00           ~              ~             ~             ~            higher
| gitsource      1.00           ~              0.75          ~             ~            higher

Hi,

Quoted 5 lines · Giovanni Gherdovich

Gitsource: this test show the most compelling case against the
sugov-HWP.desired series: on the Cascade Lake sugov-HWP.desired is 10% faster than sugov-HWP.min (it was expected to be slower!) and 35% less efficient (we expected more performance-per-watt, not less).

This is a bit counter-intuitive, so it is good to try to understand what's going on instead of drawing conclusions right away from pure numbers.

My interpretation of the available data is that gitsource benefits from the "race-to-idle" effect in terms of energy-efficiency which also causes it to suffer in terms of performance. Namely, completing the given piece of work faster causes some CPU idle time to become available and that effectively reduces power, but it also increases the response time (by the idle state exit latency) which causes performance to drop. Whether or not this effect can be present depends on what CPU idle states are available etc. and it may be a pure coincidence.

[snip]

There is a whole broad category of workloads involving periodic tasks that do the same amount of work in every period regardless of the frequency they run at (as long as the frequency is sufficient to avoid "overrunning" the period) and they almost never benefit from "race-to-idle".There is zero benefit from running them too fast and the energy-efficiency goes down the sink when that happens.

Now the problem is that with sugov-HWP.min the users who care about these workloads don't even have an option to use the task utilization history recorded by the scheduler to bias the frequency towards the "sufficient" level, because sugov-HWP.min only sets a lower bound on the frequency selection to improve the situation, so the choice between it and sugov-HWP.desired boils down to whether or not to give that option to them and my clear preference is for that option to exist. Sorry about that. [Note that it really is an option, though, because "pure" HWP is still the default for HWP-enabled systems.]

It may be possible to restore some "race-to-idle" benefits by tweaking HWP_REQ.EPP in the future, but that needs to be investigated.

BTW, what EPP value was there on the system where you saw better performance under sugov-HWP.desired? If it was greater than zero, it would be useful to decrease EPP (by adjusting the energy_performance_preference attributes in sysfs for all CPUs) and see what happens to the performance difference then.

Quoted 23 lines · Rafael J. Wysocki

Hi,

Quoted 5 lines · Giovanni Gherdovich

Gitsource: this test show the most compelling case against the
sugov-HWP.desired series: on the Cascade Lake sugov-HWP.desired is 10% faster than sugov-HWP.min (it was expected to be slower!) and 35% less efficient (we expected more performance-per-watt, not less).

This is a bit counter-intuitive, so it is good to try to understand what's going on instead of drawing conclusions right away from pure numbers.

My interpretation of the available data is that gitsource benefits from the "race-to-idle" effect in terms of energy-efficiency which also causes it to suffer in terms of performance. Namely, completing the given piece of work faster causes some CPU idle time to become available and that effectively reduces power, but it also increases the response time (by the idle state exit latency) which causes performance to drop. Whether or not this effect can be present depends on what CPU idle states are available etc. and it may be a pure coincidence.

Quoted 1 line

[snip]

Right, race-to-idle might explain the increased efficiency of HWP.MIN. As you note, increased exit latencies from idle can also explain the overall performance difference.

Quoted 7 lines

There is a whole broad category of workloads involving periodic tasks that do the same amount of work in every period regardless of the frequency they run at (as long as the frequency is sufficient to avoid "overrunning" the period) and they almost never benefit from "race-to-idle".There is zero benefit from running them too fast and the energy-efficiency goes down the sink when that happens.

Quoted 9 lines

Now the problem is that with sugov-HWP.min the users who care about these workloads don't even have an option to use the task utilization history recorded by the scheduler to bias the frequency towards the "sufficient" level, because sugov-HWP.min only sets a lower bound on the frequency selection to improve the situation, so the choice between it and sugov-HWP.desired boils down to whether or not to give that option to them and my clear preference is for that option to exist. Sorry about that. [Note that it really is an option, though, because "pure" HWP is still the default for HWP-enabled systems.]

Sure, the periodic workloads benefit from this patch, Doug's test shows that.

I guess I'm still confused by the difference between setting HWP.DESIRED and disabling HWP completely. The Intel manual says that a non-zero HWP.DESIRED "effectively disabl[es] HW autonomous selection", but then continues with "The Desired_Performance input is non-constraining in terms of Performance and Energy optimizations, which are independently controlled". The first statement sounds like HWP is out of the picture (no more autonomous frequency selections) but the latter part implies there are other optimizations still available. I'm not sure how to reason about that.

Quoted 8 lines

It may be possible to restore some "race-to-idle" benefits by tweaking HWP_REQ.EPP in the future, but that needs to be investigated.

BTW, what EPP value was there on the system where you saw better performance under sugov-HWP.desired? If it was greater than zero, it would be useful to decrease EPP (by adjusting the energy_performance_preference attributes in sysfs for all CPUs) and see what happens to the performance difference then.

For sugov-HWP.desired the EPP was 0x80 (the default value).