[PATCH v2 0/3] sched, arch_topology: Thermal pressure configuration cleanup

[PATCH v2 0/3] sched, arch_topology: Thermal pressure configuration cleanup

[Valentin Schneider]

Hi folks,

This stems from this thread [1] on the list. TL;DR: the thermal pressure config
has no helpful documentation, and figuring out if the right dependencies are in
place is not easy for a regular user. 

The current landscape also paints an odd picture: arch_set_thermal_pressure() is
hardcoded in sched/core.c, and is *not* architecture-definable, while
arch_get_thermal_pressure() is. Patch 1 is tackling this, the rest is Kconfig
stuff.

Cheers,
Valentin

[1]: https://lkml.kernel.org/r/20200603173150.GB1551@shell.armlinux.org.uk

Revisions
=========

v1 -> v2
--------

o Remove cpufreq_cooling.c weak function; use #define stub in sched/topology.h
  (Vincent)
o Hinge arm SCHED_THERMAL_PRESSURE selection on ARM_CPU_TOPOLOGY

Valentin Schneider (3):
  arch_topology, sched/core: Cleanup thermal pressure definition
  sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry
  arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE

 arch/arm/Kconfig                  |  1 +
 arch/arm/include/asm/topology.h   |  3 ++-
 arch/arm64/Kconfig                |  1 +
 arch/arm64/include/asm/topology.h |  3 ++-
 drivers/base/arch_topology.c      | 11 +++++++++++
 include/linux/arch_topology.h     |  4 ++--
 include/linux/sched/topology.h    |  7 +++++++
 init/Kconfig                      | 15 ++++++++++++++-
 kernel/sched/core.c               | 11 -----------
 9 files changed, 40 insertions(+), 16 deletions(-)

--
2.27.0

[PATCH v2 1/3] arch_topology, sched/core: Cleanup thermal pressure definition

[Valentin Schneider]

The following commit:

  14533a16c46d ("thermal/cpu-cooling, sched/core: Move the arch_set_thermal_pressure() API to generic scheduler code")

moved the definition of arch_set_thermal_pressure() to sched/core.c, but
kept its declaration in linux/arch_topology.h. When building e.g. an x86
kernel with CONFIG_SCHED_THERMAL_PRESSURE=y, cpufreq_cooling.c ends up
getting the declaration of arch_set_thermal_pressure() from
include/linux/arch_topology.h, which is somewhat awkward.

On top of this, sched/core.c unconditionally defines
o The thermal_pressure percpu variable
o arch_set_thermal_pressure()

while arch_scale_thermal_pressure() does nothing unless redefined by the
architecture.

arch_*() functions are meant to be defined by architectures, so revert the
aforementioned commit and re-implement it in a way that keeps
arch_set_thermal_pressure() architecture-definable, and doesn't define the
thermal pressure percpu variable for kernels that don't need
it (CONFIG_SCHED_THERMAL_PRESSURE=n).

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 arch/arm/include/asm/topology.h   |  3 ++-
 arch/arm64/include/asm/topology.h |  3 ++-
 drivers/base/arch_topology.c      | 11 +++++++++++
 include/linux/arch_topology.h     |  4 ++--
 include/linux/sched/topology.h    |  7 +++++++
 kernel/sched/core.c               | 11 -----------
 6 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index 435aba289fc5..e0593cf095d0 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -16,8 +16,9 @@
 /* Enable topology flag updates */
 #define arch_update_cpu_topology topology_update_cpu_topology
 
-/* Replace task scheduler's default thermal pressure retrieve API */
+/* Replace task scheduler's default thermal pressure API */
 #define arch_scale_thermal_pressure topology_get_thermal_pressure
+#define arch_set_thermal_pressure   topology_set_thermal_pressure
 
 #else
 
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 0cc835ddfcd1..e042f6527981 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -34,8 +34,9 @@ void topology_scale_freq_tick(void);
 /* Enable topology flag updates */
 #define arch_update_cpu_topology topology_update_cpu_topology
 
-/* Replace task scheduler's default thermal pressure retrieve API */
+/* Replace task scheduler's default thermal pressure API */
 #define arch_scale_thermal_pressure topology_get_thermal_pressure
+#define arch_set_thermal_pressure   topology_set_thermal_pressure
 
 #include <asm-generic/topology.h>
 
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 4d0a0038b476..d14cab7dfa3c 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -54,6 +54,17 @@ void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
 	per_cpu(cpu_scale, cpu) = capacity;
 }
 
+DEFINE_PER_CPU(unsigned long, thermal_pressure);
+
+void arch_set_thermal_pressure(const struct cpumask *cpus,
+			       unsigned long th_pressure)
+{
+	int cpu;
+
+	for_each_cpu(cpu, cpus)
+		WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
+}
+
 static ssize_t cpu_capacity_show(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 0566cb3314ef..69b1dabe39dc 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -39,8 +39,8 @@ static inline unsigned long topology_get_thermal_pressure(int cpu)
 	return per_cpu(thermal_pressure, cpu);
 }
 
-void arch_set_thermal_pressure(struct cpumask *cpus,
-			       unsigned long th_pressure);
+void topology_set_thermal_pressure(const struct cpumask *cpus,
+				   unsigned long th_pressure);
 
 struct cpu_topology {
 	int thread_id;
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index fb11091129b3..764222d637b7 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -232,6 +232,13 @@ unsigned long arch_scale_thermal_pressure(int cpu)
 }
 #endif
 
+#ifndef arch_set_thermal_pressure
+static __always_inline
+void arch_set_thermal_pressure(const struct cpumask *cpus,
+			       unsigned long th_pressure)
+{ }
+#endif
+
 static inline int task_node(const struct task_struct *p)
 {
 	return cpu_to_node(task_cpu(p));
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ff0519551188..90b44f3840e4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3731,17 +3731,6 @@ unsigned long long task_sched_runtime(struct task_struct *p)
 	return ns;
 }
 
-DEFINE_PER_CPU(unsigned long, thermal_pressure);
-
-void arch_set_thermal_pressure(struct cpumask *cpus,
-			       unsigned long th_pressure)
-{
-	int cpu;
-
-	for_each_cpu(cpu, cpus)
-		WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
-}
-
 /*
  * This function gets called by the timer code, with HZ frequency.
  * We call it with interrupts disabled.
-- 
2.27.0

[PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Valentin Schneider]

As Russell pointed out [1], this option is severely lacking in the
documentation department, and figuring out if one has the required
dependencies to benefit from turning it on is not straightforward.

Make it non user-visible, and add a bit of help to it. While at it, make it
depend on CPU_FREQ_THERMAL.

[1]: https://lkml.kernel.org/r/20200603173150.GB1551@shell.armlinux.org.uk

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 init/Kconfig | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/init/Kconfig b/init/Kconfig
index 0498af567f70..0a97d85568b2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -492,8 +492,21 @@ config HAVE_SCHED_AVG_IRQ
 	depends on SMP
 
 config SCHED_THERMAL_PRESSURE
-	bool "Enable periodic averaging of thermal pressure"
+	bool
 	depends on SMP
+	depends on CPU_FREQ_THERMAL
+	help
+	  Select this option to enable thermal pressure accounting in the
+	  scheduler. Thermal pressure is the value conveyed to the scheduler
+	  that reflects the reduction in CPU compute capacity resulted from
+	  thermal throttling. Thermal throttling occurs when the performance of
+	  a CPU is capped due to high operating temperatures.
+
+	  If selected, the scheduler will be able to balance tasks accordingly,
+	  i.e. put less load on throttled CPUs than on non/less throttled ones.
+
+	  This requires the architecture to implement
+	  arch_set_thermal_pressure() and arch_get_thermal_pressure().
 
 config BSD_PROCESS_ACCT
 	bool "BSD Process Accounting"
-- 
2.27.0

[PATCH v2 3/3] arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE

[Valentin Schneider]

This option now correctly depends on CPU_FREQ_THERMAL, so select it on the
architectures that implement the required functions,
arch_set_thermal_pressure() and arch_get_thermal_pressure().

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 arch/arm/Kconfig   | 1 +
 arch/arm64/Kconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 2ac74904a3ce..939c4d6bbc2e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -46,6 +46,7 @@ config ARM
 	select EDAC_ATOMIC_SCRUB
 	select GENERIC_ALLOCATOR
 	select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
+	select SCHED_THERMAL_PRESSURE if ARM_CPU_TOPOLOGY
 	select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
 	select GENERIC_CPU_AUTOPROBE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 66dc41fd49f2..96d478fb7a2e 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -100,6 +100,7 @@ config ARM64
 	select FRAME_POINTER
 	select GENERIC_ALLOCATOR
 	select GENERIC_ARCH_TOPOLOGY
+	select SCHED_THERMAL_PRESSURE
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_CLOCKEVENTS_BROADCAST
 	select GENERIC_CPU_AUTOPROBE
-- 
2.27.0

Re: [PATCH v2 3/3] arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE

[Catalin Marinas]

On Sun, Jul 12, 2020 at 05:59:17PM +0100, Valentin Schneider wrote:
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 66dc41fd49f2..96d478fb7a2e 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -100,6 +100,7 @@ config ARM64
>  	select FRAME_POINTER
>  	select GENERIC_ALLOCATOR
>  	select GENERIC_ARCH_TOPOLOGY
> +	select SCHED_THERMAL_PRESSURE
>  	select GENERIC_CLOCKEVENTS
>  	select GENERIC_CLOCKEVENTS_BROADCAST
>  	select GENERIC_CPU_AUTOPROBE

We tend to keep these in alphabetical order. Otherwise,

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 3/3] arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE

[Valentin Schneider]

On 13/07/20 11:03, Catalin Marinas wrote:
> On Sun, Jul 12, 2020 at 05:59:17PM +0100, Valentin Schneider wrote:
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 66dc41fd49f2..96d478fb7a2e 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -100,6 +100,7 @@ config ARM64
>>      select FRAME_POINTER
>>      select GENERIC_ALLOCATOR
>>      select GENERIC_ARCH_TOPOLOGY
>> +	select SCHED_THERMAL_PRESSURE
>>      select GENERIC_CLOCKEVENTS
>>      select GENERIC_CLOCKEVENTS_BROADCAST
>>      select GENERIC_CPU_AUTOPROBE
>
> We tend to keep these in alphabetical order. Otherwise,
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks! Will reorder that in v3.

Re: [PATCH v2 0/3] sched, arch_topology: Thermal pressure configuration cleanup

[Peter Zijlstra]

On Sun, Jul 12, 2020 at 05:59:14PM +0100, Valentin Schneider wrote:
> Valentin Schneider (3):
>   arch_topology, sched/core: Cleanup thermal pressure definition
>   sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry
>   arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE

Thanks!

Re: [PATCH v2 0/3] sched, arch_topology: Thermal pressure configuration cleanup

[Vincent Guittot]

On Sun, 12 Jul 2020 at 18:59, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> Hi folks,
>
> This stems from this thread [1] on the list. TL;DR: the thermal pressure config
> has no helpful documentation, and figuring out if the right dependencies are in
> place is not easy for a regular user.
>
> The current landscape also paints an odd picture: arch_set_thermal_pressure() is
> hardcoded in sched/core.c, and is *not* architecture-definable, while
> arch_get_thermal_pressure() is. Patch 1 is tackling this, the rest is Kconfig
> stuff.
>
> Cheers,
> Valentin
>
> [1]: https://lkml.kernel.org/r/20200603173150.GB1551@shell.armlinux.org.uk
>
> Revisions
> =========
>
> v1 -> v2
> --------
>
> o Remove cpufreq_cooling.c weak function; use #define stub in sched/topology.h
>   (Vincent)

Looks good to me.

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

> o Hinge arm SCHED_THERMAL_PRESSURE selection on ARM_CPU_TOPOLOGY
>
> Valentin Schneider (3):
>   arch_topology, sched/core: Cleanup thermal pressure definition
>   sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry
>   arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE
>
>  arch/arm/Kconfig                  |  1 +
>  arch/arm/include/asm/topology.h   |  3 ++-
>  arch/arm64/Kconfig                |  1 +
>  arch/arm64/include/asm/topology.h |  3 ++-
>  drivers/base/arch_topology.c      | 11 +++++++++++
>  include/linux/arch_topology.h     |  4 ++--
>  include/linux/sched/topology.h    |  7 +++++++
>  init/Kconfig                      | 15 ++++++++++++++-
>  kernel/sched/core.c               | 11 -----------
>  9 files changed, 40 insertions(+), 16 deletions(-)
>
> --
> 2.27.0
>

Re: [PATCH v2 1/3] arch_topology, sched/core: Cleanup thermal pressure definition

[Thara Gopinath]

On 7/12/20 12:59 PM, Valentin Schneider wrote:
> The following commit:
> 
>    14533a16c46d ("thermal/cpu-cooling, sched/core: Move the arch_set_thermal_pressure() API to generic scheduler code")
> 
> moved the definition of arch_set_thermal_pressure() to sched/core.c, but
> kept its declaration in linux/arch_topology.h. When building e.g. an x86
> kernel with CONFIG_SCHED_THERMAL_PRESSURE=y, cpufreq_cooling.c ends up
> getting the declaration of arch_set_thermal_pressure() from
> include/linux/arch_topology.h, which is somewhat awkward.
> 
> On top of this, sched/core.c unconditionally defines
> o The thermal_pressure percpu variable
> o arch_set_thermal_pressure()
> 
> while arch_scale_thermal_pressure() does nothing unless redefined by the
> architecture.
> 
> arch_*() functions are meant to be defined by architectures, so revert the
> aforementioned commit and re-implement it in a way that keeps
> arch_set_thermal_pressure() architecture-definable, and doesn't define the
> thermal pressure percpu variable for kernels that don't need
> it (CONFIG_SCHED_THERMAL_PRESSURE=n).
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---

Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>

>   arch/arm/include/asm/topology.h   |  3 ++-
>   arch/arm64/include/asm/topology.h |  3 ++-
>   drivers/base/arch_topology.c      | 11 +++++++++++
>   include/linux/arch_topology.h     |  4 ++--
>   include/linux/sched/topology.h    |  7 +++++++
>   kernel/sched/core.c               | 11 -----------
>   6 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 435aba289fc5..e0593cf095d0 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -16,8 +16,9 @@
>   /* Enable topology flag updates */
>   #define arch_update_cpu_topology topology_update_cpu_topology
>   
> -/* Replace task scheduler's default thermal pressure retrieve API */
> +/* Replace task scheduler's default thermal pressure API */
>   #define arch_scale_thermal_pressure topology_get_thermal_pressure
> +#define arch_set_thermal_pressure   topology_set_thermal_pressure
>   
>   #else
>   
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index 0cc835ddfcd1..e042f6527981 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -34,8 +34,9 @@ void topology_scale_freq_tick(void);
>   /* Enable topology flag updates */
>   #define arch_update_cpu_topology topology_update_cpu_topology
>   
> -/* Replace task scheduler's default thermal pressure retrieve API */
> +/* Replace task scheduler's default thermal pressure API */
>   #define arch_scale_thermal_pressure topology_get_thermal_pressure
> +#define arch_set_thermal_pressure   topology_set_thermal_pressure
>   
>   #include <asm-generic/topology.h>
>   
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 4d0a0038b476..d14cab7dfa3c 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -54,6 +54,17 @@ void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
>   	per_cpu(cpu_scale, cpu) = capacity;
>   }
>   
> +DEFINE_PER_CPU(unsigned long, thermal_pressure);
> +
> +void arch_set_thermal_pressure(const struct cpumask *cpus,
> +			       unsigned long th_pressure)
> +{
> +	int cpu;
> +
> +	for_each_cpu(cpu, cpus)
> +		WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
> +}
> +
>   static ssize_t cpu_capacity_show(struct device *dev,
>   				 struct device_attribute *attr,
>   				 char *buf)
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 0566cb3314ef..69b1dabe39dc 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -39,8 +39,8 @@ static inline unsigned long topology_get_thermal_pressure(int cpu)
>   	return per_cpu(thermal_pressure, cpu);
>   }
>   
> -void arch_set_thermal_pressure(struct cpumask *cpus,
> -			       unsigned long th_pressure);
> +void topology_set_thermal_pressure(const struct cpumask *cpus,
> +				   unsigned long th_pressure);
>   
>   struct cpu_topology {
>   	int thread_id;
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index fb11091129b3..764222d637b7 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -232,6 +232,13 @@ unsigned long arch_scale_thermal_pressure(int cpu)
>   }
>   #endif
>   
> +#ifndef arch_set_thermal_pressure
> +static __always_inline
> +void arch_set_thermal_pressure(const struct cpumask *cpus,
> +			       unsigned long th_pressure)
> +{ }
> +#endif
> +
>   static inline int task_node(const struct task_struct *p)
>   {
>   	return cpu_to_node(task_cpu(p));
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index ff0519551188..90b44f3840e4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3731,17 +3731,6 @@ unsigned long long task_sched_runtime(struct task_struct *p)
>   	return ns;
>   }
>   
> -DEFINE_PER_CPU(unsigned long, thermal_pressure);
> -
> -void arch_set_thermal_pressure(struct cpumask *cpus,
> -			       unsigned long th_pressure)
> -{
> -	int cpu;
> -
> -	for_each_cpu(cpu, cpus)
> -		WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
> -}
> -
>   /*
>    * This function gets called by the timer code, with HZ frequency.
>    * We call it with interrupts disabled.
> 

-- 
Warm Regards
Thara

Re: [PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Qian Cai]

On Sun, Jul 12, 2020 at 05:59:16PM +0100, Valentin Schneider wrote:
> As Russell pointed out [1], this option is severely lacking in the
> documentation department, and figuring out if one has the required
> dependencies to benefit from turning it on is not straightforward.
> 
> Make it non user-visible, and add a bit of help to it. While at it, make it
> depend on CPU_FREQ_THERMAL.
> 
> [1]: https://lkml.kernel.org/r/20200603173150.GB1551@shell.armlinux.org.uk
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  init/Kconfig | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 0498af567f70..0a97d85568b2 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -492,8 +492,21 @@ config HAVE_SCHED_AVG_IRQ
>  	depends on SMP
>  
>  config SCHED_THERMAL_PRESSURE
> -	bool "Enable periodic averaging of thermal pressure"
> +	bool
>  	depends on SMP
> +	depends on CPU_FREQ_THERMAL
> +	help
> +	  Select this option to enable thermal pressure accounting in the
> +	  scheduler. Thermal pressure is the value conveyed to the scheduler
> +	  that reflects the reduction in CPU compute capacity resulted from
> +	  thermal throttling. Thermal throttling occurs when the performance of
> +	  a CPU is capped due to high operating temperatures.
> +
> +	  If selected, the scheduler will be able to balance tasks accordingly,
> +	  i.e. put less load on throttled CPUs than on non/less throttled ones.
> +
> +	  This requires the architecture to implement
> +	  arch_set_thermal_pressure() and arch_get_thermal_pressure().
>  
>  config BSD_PROCESS_ACCT
>  	bool "BSD Process Accounting"
> -- 

On arm64 linux-next (20200727),

https://gitlab.com/cailca/linux-mm/-/blob/master/arm64.config

WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
  Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
  Selected by [y]:
  - ARM64 [=y]

Re: [PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Dietmar Eggemann]

On 27/07/2020 16:18, Qian Cai wrote:
> On Sun, Jul 12, 2020 at 05:59:16PM +0100, Valentin Schneider wrote:
>> As Russell pointed out [1], this option is severely lacking in the
>> documentation department, and figuring out if one has the required
>> dependencies to benefit from turning it on is not straightforward.
>>
>> Make it non user-visible, and add a bit of help to it. While at it, make it
>> depend on CPU_FREQ_THERMAL.
>>
>> [1]: https://lkml.kernel.org/r/20200603173150.GB1551@shell.armlinux.org.uk
>>
>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>> ---
>>  init/Kconfig | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 0498af567f70..0a97d85568b2 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -492,8 +492,21 @@ config HAVE_SCHED_AVG_IRQ
>>  	depends on SMP
>>  
>>  config SCHED_THERMAL_PRESSURE
>> -	bool "Enable periodic averaging of thermal pressure"
>> +	bool
>>  	depends on SMP
>> +	depends on CPU_FREQ_THERMAL
>> +	help
>> +	  Select this option to enable thermal pressure accounting in the
>> +	  scheduler. Thermal pressure is the value conveyed to the scheduler
>> +	  that reflects the reduction in CPU compute capacity resulted from
>> +	  thermal throttling. Thermal throttling occurs when the performance of
>> +	  a CPU is capped due to high operating temperatures.
>> +
>> +	  If selected, the scheduler will be able to balance tasks accordingly,
>> +	  i.e. put less load on throttled CPUs than on non/less throttled ones.
>> +
>> +	  This requires the architecture to implement
>> +	  arch_set_thermal_pressure() and arch_get_thermal_pressure().
>>  
>>  config BSD_PROCESS_ACCT
>>  	bool "BSD Process Accounting"
>> -- 
> 
> On arm64 linux-next (20200727),
> 
> https://gitlab.com/cailca/linux-mm/-/blob/master/arm64.config
> 
> WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
>   Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
>   Selected by [y]:
>   - ARM64 [=y]

Not sure, but:

(1) do we wan to let people enable SCHED_THERMAL_PRESSURE for arm64 so
arm64 can potentially run w/o a CPU freq cooling device?

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 2d4abbc9f8d0..baffe8b66da2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -192,7 +192,6 @@ config ARM64
        select PCI_SYSCALL if PCI
        select POWER_RESET
        select POWER_SUPPLY
-       select SCHED_THERMAL_PRESSURE
        select SPARSE_IRQ
        select SWIOTLB
        select SYSCTL_EXCEPTION_TRACE
diff --git a/init/Kconfig b/init/Kconfig
index 37b089f87804..8b36e07fb230 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -492,7 +492,7 @@ config HAVE_SCHED_AVG_IRQ
        depends on SMP

 config SCHED_THERMAL_PRESSURE
-       bool
+       bool "Thermal pressure accounting"
        depends on SMP
        depends on CPU_FREQ_THERMAL
        help

Or

(2) should SCHED_THERMAL_PRESSURE for arm64 be enabled by default?

But then it makes no sense to allow the removal of CONFIG_CPU_FREQ_THERMAL.

linux-next/master$ make ARCH=arm64 defconfig

// Remove CONFIG_CPU_FREQ_THERMAL
linux-next/master$ grep CPU_FREQ_THERMAL .config
# CONFIG_CPU_FREQ_THERMAL is not set

linux-next/master$ make
scripts/kconfig/conf  --syncconfig Kconfig

WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
  Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
  Selected by [y]:
  - ARM64 [=y]

WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
  Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
  Selected by [y]:
  - ARM64 [=y]

WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
  Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
  Selected by [y]:
  - ARM64 [=y]
  HOSTCC  scripts/dtc/dtc.o

---

There is a similar issue with arm.

I would prefer for (1).

Re: [PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Valentin Schneider]

Hi,

On 27/07/20 18:45, Dietmar Eggemann wrote:
> On 27/07/2020 16:18, Qian Cai wrote:
>> On Sun, Jul 12, 2020 at 05:59:16PM +0100, Valentin Schneider wrote:
>>> As Russell pointed out [1], this option is severely lacking in the
>>> documentation department, and figuring out if one has the required
>>> dependencies to benefit from turning it on is not straightforward.
>>>
>>> Make it non user-visible, and add a bit of help to it. While at it, make it
>>> depend on CPU_FREQ_THERMAL.
>>>
>>> [1]: https://lkml.kernel.org/r/20200603173150.GB1551@shell.armlinux.org.uk
>>>
>>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>>> ---
>>>  init/Kconfig | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/init/Kconfig b/init/Kconfig
>>> index 0498af567f70..0a97d85568b2 100644
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -492,8 +492,21 @@ config HAVE_SCHED_AVG_IRQ
>>>     depends on SMP
>>>
>>>  config SCHED_THERMAL_PRESSURE
>>> -	bool "Enable periodic averaging of thermal pressure"
>>> +	bool
>>>     depends on SMP
>>> +	depends on CPU_FREQ_THERMAL
>>> +	help
>>> +	  Select this option to enable thermal pressure accounting in the
>>> +	  scheduler. Thermal pressure is the value conveyed to the scheduler
>>> +	  that reflects the reduction in CPU compute capacity resulted from
>>> +	  thermal throttling. Thermal throttling occurs when the performance of
>>> +	  a CPU is capped due to high operating temperatures.
>>> +
>>> +	  If selected, the scheduler will be able to balance tasks accordingly,
>>> +	  i.e. put less load on throttled CPUs than on non/less throttled ones.
>>> +
>>> +	  This requires the architecture to implement
>>> +	  arch_set_thermal_pressure() and arch_get_thermal_pressure().
>>>
>>>  config BSD_PROCESS_ACCT
>>>     bool "BSD Process Accounting"
>>> --
>>
>> On arm64 linux-next (20200727),
>>
>> https://gitlab.com/cailca/linux-mm/-/blob/master/arm64.config
>>
>> WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
>>   Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
>>   Selected by [y]:
>>   - ARM64 [=y]
>
> Not sure, but:
>
> (1) do we wan to let people enable SCHED_THERMAL_PRESSURE for arm64 so
> arm64 can potentially run w/o a CPU freq cooling device?
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 2d4abbc9f8d0..baffe8b66da2 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -192,7 +192,6 @@ config ARM64
>         select PCI_SYSCALL if PCI
>         select POWER_RESET
>         select POWER_SUPPLY
> -       select SCHED_THERMAL_PRESSURE
>         select SPARSE_IRQ
>         select SWIOTLB
>         select SYSCTL_EXCEPTION_TRACE
> diff --git a/init/Kconfig b/init/Kconfig
> index 37b089f87804..8b36e07fb230 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -492,7 +492,7 @@ config HAVE_SCHED_AVG_IRQ
>         depends on SMP
>
>  config SCHED_THERMAL_PRESSURE
> -       bool
> +       bool "Thermal pressure accounting"
>         depends on SMP
>         depends on CPU_FREQ_THERMAL
>         help
>
> Or
>
> (2) should SCHED_THERMAL_PRESSURE for arm64 be enabled by default?
>
> But then it makes no sense to allow the removal of CONFIG_CPU_FREQ_THERMAL.
>
> linux-next/master$ make ARCH=arm64 defconfig
>
> // Remove CONFIG_CPU_FREQ_THERMAL
> linux-next/master$ grep CPU_FREQ_THERMAL .config
> # CONFIG_CPU_FREQ_THERMAL is not set
>
> linux-next/master$ make
> scripts/kconfig/conf  --syncconfig Kconfig
>
> WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
>   Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
>   Selected by [y]:
>   - ARM64 [=y]
>
> WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
>   Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
>   Selected by [y]:
>   - ARM64 [=y]
>
> WARNING: unmet direct dependencies detected for SCHED_THERMAL_PRESSURE
>   Depends on [n]: SMP [=y] && CPU_FREQ_THERMAL [=n]
>   Selected by [y]:
>   - ARM64 [=y]
>   HOSTCC  scripts/dtc/dtc.o
>
> ---
>
> There is a similar issue with arm.
>
> I would prefer for (1).

I went for having SCHED_THERMAL_PRESSURE in arm64/Kconfig because of where
the discussion went in the original thread ([1] in the changelog).

One point is that selecting this option requires having the right
infrastructure in place (arch_{set, scale}_thermal_pressure() must be
redefined by the architecture), which cannot be easily expressed in Kconfig
terms. Russell's point was that this is difficult for a lambda user to make
sense of, and Vincent argued that this option should simply be selected at
architecture level, which, given the context, makes sense IMO.

We could change the arch Kconfig into

  select SCHED_THERMAL_PRESSURE if CPU_FREQ_THERMAL

but that seems redundant; this dependency is already expressed in
SCHED_THERMAL_PRESSURE's definition. Is there a proper pattern to select
some Kconfig option only if all of its dependencies are met?

Re: [PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Dietmar Eggemann]

On 28/07/2020 18:16, Valentin Schneider wrote:
> 
> Hi,
> 
> On 27/07/20 18:45, Dietmar Eggemann wrote:
>> On 27/07/2020 16:18, Qian Cai wrote:
>>> On Sun, Jul 12, 2020 at 05:59:16PM +0100, Valentin Schneider wrote:

[...]

> I went for having SCHED_THERMAL_PRESSURE in arm64/Kconfig because of where
> the discussion went in the original thread ([1] in the changelog).
> 
> One point is that selecting this option requires having the right
> infrastructure in place (arch_{set, scale}_thermal_pressure() must be
> redefined by the architecture), which cannot be easily expressed in Kconfig
> terms. Russell's point was that this is difficult for a lambda user to make
> sense of, and Vincent argued that this option should simply be selected at
> architecture level, which, given the context, makes sense IMO.
> 
> We could change the arch Kconfig into
> 
>   select SCHED_THERMAL_PRESSURE if CPU_FREQ_THERMAL
> 
> but that seems redundant; this dependency is already expressed in
> SCHED_THERMAL_PRESSURE's definition. Is there a proper pattern to select
> some Kconfig option only if all of its dependencies are met?

The warning when disabling CPU_FREQ_THERMAL after make defconfig disappears, so
this should be OK.

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 939c4d6bbc2e..a677e71b3d5f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -46,7 +46,7 @@ config ARM
        select EDAC_ATOMIC_SCRUB
        select GENERIC_ALLOCATOR
        select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
-       select SCHED_THERMAL_PRESSURE if ARM_CPU_TOPOLOGY
+       select SCHED_THERMAL_PRESSURE if ARM_CPU_TOPOLOGY && CPU_FREQ_THERMAL
        select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
        select GENERIC_CLOCKEVENTS_BROADCAST if SMP
        select GENERIC_CPU_AUTOPROBE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c403e6f5db86..59ae16f8b941 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -192,7 +192,7 @@ config ARM64
        select PCI_SYSCALL if PCI
        select POWER_RESET
        select POWER_SUPPLY
-       select SCHED_THERMAL_PRESSURE
+       select SCHED_THERMAL_PRESSURE if CPU_FREQ_THERMAL
        select SPARSE_IRQ
        select SWIOTLB
        select SYSCTL_EXCEPTION_TRAC

Re: [PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Quentin Perret]

On Tuesday 28 Jul 2020 at 17:16:57 (+0100), Valentin Schneider wrote:
> We could change the arch Kconfig into
> 
>   select SCHED_THERMAL_PRESSURE if CPU_FREQ_THERMAL
> 
> but that seems redundant; this dependency is already expressed in
> SCHED_THERMAL_PRESSURE's definition. Is there a proper pattern to select
> some Kconfig option only if all of its dependencies are met?

How about something like this (totally untested):

---8<---
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 939c4d6bbc2e..2ac74904a3ce 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -46,7 +46,6 @@ config ARM
 	select EDAC_ATOMIC_SCRUB
 	select GENERIC_ALLOCATOR
 	select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
-	select SCHED_THERMAL_PRESSURE if ARM_CPU_TOPOLOGY
 	select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
 	select GENERIC_CPU_AUTOPROBE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c403e6f5db86..66dc41fd49f2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -192,7 +192,6 @@ config ARM64
 	select PCI_SYSCALL if PCI
 	select POWER_RESET
 	select POWER_SUPPLY
-	select SCHED_THERMAL_PRESSURE
 	select SPARSE_IRQ
 	select SWIOTLB
 	select SYSCTL_EXCEPTION_TRACE
diff --git a/init/Kconfig b/init/Kconfig
index 0a97d85568b2..c2e1f3ac527e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -493,6 +493,7 @@ config HAVE_SCHED_AVG_IRQ
 
 config SCHED_THERMAL_PRESSURE
 	bool
+	default y if ARM64 || ARM_CPU_TOPOLOGY
 	depends on SMP
 	depends on CPU_FREQ_THERMAL
 	help
--->8---

Thanks,
Quentin

Re: [PATCH v2 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE kconfig entry

[Valentin Schneider]

On 29/07/20 14:09, Quentin Perret wrote:
> On Tuesday 28 Jul 2020 at 17:16:57 (+0100), Valentin Schneider wrote:
>> We could change the arch Kconfig into
>>
>>   select SCHED_THERMAL_PRESSURE if CPU_FREQ_THERMAL
>>
>> but that seems redundant; this dependency is already expressed in
>> SCHED_THERMAL_PRESSURE's definition. Is there a proper pattern to select
>> some Kconfig option only if all of its dependencies are met?
>
> How about something like this (totally untested):
>
> ---8<---
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 939c4d6bbc2e..2ac74904a3ce 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -46,7 +46,6 @@ config ARM
>       select EDAC_ATOMIC_SCRUB
>       select GENERIC_ALLOCATOR
>       select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
> -	select SCHED_THERMAL_PRESSURE if ARM_CPU_TOPOLOGY
>       select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
>       select GENERIC_CLOCKEVENTS_BROADCAST if SMP
>       select GENERIC_CPU_AUTOPROBE
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index c403e6f5db86..66dc41fd49f2 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -192,7 +192,6 @@ config ARM64
>       select PCI_SYSCALL if PCI
>       select POWER_RESET
>       select POWER_SUPPLY
> -	select SCHED_THERMAL_PRESSURE
>       select SPARSE_IRQ
>       select SWIOTLB
>       select SYSCTL_EXCEPTION_TRACE
> diff --git a/init/Kconfig b/init/Kconfig
> index 0a97d85568b2..c2e1f3ac527e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -493,6 +493,7 @@ config HAVE_SCHED_AVG_IRQ
>
>  config SCHED_THERMAL_PRESSURE
>       bool
> +	default y if ARM64 || ARM_CPU_TOPOLOGY
>       depends on SMP
>       depends on CPU_FREQ_THERMAL
>       help
> --->8---
>

That does seem to do just what I was looking for, thanks!

> Thanks,
> Quentin