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

[PATCH 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

Valentin Schneider (3):
  thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition
  sched: Cleanup SCHED_THERMAL_PRESSURE setup
  arm, arm64: Select CONFIG_SCHED_THERMAL_PRESSURE

 arch/arm/Kconfig                  |  1 +
 arch/arm64/Kconfig                |  1 +
 drivers/base/arch_topology.c      | 11 +++++++++++
 drivers/thermal/cpufreq_cooling.c |  5 +++++
 include/linux/arch_topology.h     |  3 ---
 init/Kconfig                      | 15 ++++++++++++++-
 kernel/sched/core.c               | 11 -----------
 7 files changed, 32 insertions(+), 15 deletions(-)

--
2.27.0

[PATCH 1/3] thermal/cpu-cooling, 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, the public setter, arch_set_thermal_pressure(), is defined
unconditionally in sched/core.c while the public getter,
arch_scale_thermal_pressure(), is hardcoded to return 0 unless it has been
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.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 drivers/base/arch_topology.c      | 11 +++++++++++
 drivers/thermal/cpufreq_cooling.c |  5 +++++
 include/linux/arch_topology.h     |  3 ---
 kernel/sched/core.c               | 11 -----------
 4 files changed, 16 insertions(+), 14 deletions(-)

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/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
index e297e135c031..a1efd379b683 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
 	return 0;
 }
 
+__weak void
+arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
+{
+}
+
 /**
  * cpufreq_set_cur_state - callback function to set the current cooling state.
  * @cdev: thermal cooling device pointer.
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 0566cb3314ef..81bd1c627195 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -39,9 +39,6 @@ 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);
-
 struct cpu_topology {
 	int thread_id;
 	int core_id;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43ba2d4a8eca..7861d21f3c2b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3628,17 +3628,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 2/3] sched: Cleanup SCHED_THERMAL_PRESSURE setup

[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 74a5ac65644f..052c9f4531c2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -439,8 +439,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 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 c77c93c485a0..f80bc12dabed 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 GENERIC_ARCH_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 5d513f461957..8bfe9221309f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -93,6 +93,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 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]

Hi Valentin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on driver-core/driver-core-testing tip/sched/core arm/for-next arm64/for-next/core soc/for-next linus/master v5.7 next-20200613]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Valentin-Schneider/sched-arch_topology-Thermal-pressure-configuration-cleanup/20200614-091051
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8dc697d75c13ee2901d1a40f1d7d58163048c204
config: arm64-randconfig-r013-20200614 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project cb5072d1877b38c972f95092db2cedbcddb81da6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/base/arch_topology.c:59:6: warning: no previous prototype for function 'arch_set_thermal_pressure' [-Wmissing-prototypes]
void arch_set_thermal_pressure(const struct cpumask *cpus,
^
drivers/base/arch_topology.c:59:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void arch_set_thermal_pressure(const struct cpumask *cpus,
^
static
1 warning generated.

vim +/arch_set_thermal_pressure +59 drivers/base/arch_topology.c

    58	
  > 59	void arch_set_thermal_pressure(const struct cpumask *cpus,
    60				       unsigned long th_pressure)
    61	{
    62		int cpu;
    63	
    64		for_each_cpu(cpu, cpus)
    65			WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
    66	}
    67	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36464 bytes --]

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

Hi Valentin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on driver-core/driver-core-testing tip/sched/core arm/for-next arm64/for-next/core soc/for-next linus/master v5.7 next-20200613]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Valentin-Schneider/sched-arch_topology-Thermal-pressure-configuration-cleanup/20200614-091051
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8dc697d75c13ee2901d1a40f1d7d58163048c204
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/base/arch_topology.c:59:6: warning: no previous prototype for 'arch_set_thermal_pressure' [-Wmissing-prototypes]
59 | void arch_set_thermal_pressure(const struct cpumask *cpus,
|      ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/arch_set_thermal_pressure +59 drivers/base/arch_topology.c

    58	
  > 59	void arch_set_thermal_pressure(const struct cpumask *cpus,
    60				       unsigned long th_pressure)
    61	{
    62		int cpu;
    63	
    64		for_each_cpu(cpu, cpus)
    65			WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
    66	}
    67	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64515 bytes --]

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

Hi Valentin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on driver-core/driver-core-testing tip/sched/core arm/for-next arm64/for-next/core soc/for-next linus/master v5.7 next-20200613]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Valentin-Schneider/sched-arch_topology-Thermal-pressure-configuration-cleanup/20200614-091051
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8dc697d75c13ee2901d1a40f1d7d58163048c204
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/thermal/cpufreq_cooling.c:421:1: warning: no previous prototype for 'arch_set_thermal_pressure' [-Wmissing-prototypes]
421 | arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
| ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/arch_set_thermal_pressure +421 drivers/thermal/cpufreq_cooling.c

   419	
   420	__weak void
 > 421	arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
   422	{
   423	}
   424	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65591 bytes --]

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Valentin Schneider]

On 14/06/20 08:39, kernel test robot wrote:
> Hi Valentin,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on tip/auto-latest]
> [also build test WARNING on driver-core/driver-core-testing tip/sched/core arm/for-next arm64/for-next/core soc/for-next linus/master v5.7 next-20200613]
> [cannot apply to linux/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Valentin-Schneider/sched-arch_topology-Thermal-pressure-configuration-cleanup/20200614-091051
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8dc697d75c13ee2901d1a40f1d7d58163048c204
> config: arm64-randconfig-r013-20200614 (attached as .config)
> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project cb5072d1877b38c972f95092db2cedbcddb81da6)
> reproduce (this is a W=1 build):

Ah, W=1! I thought I was going nuts.

If desired, I can add a declaration in cpu_cooling.h, similar to what we
have for the arch_set_freq_scale() stub.

>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install arm64 cross compiling tool for clang build
>         # apt-get install binutils-aarch64-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
>>> drivers/base/arch_topology.c:59:6: warning: no previous prototype for function 'arch_set_thermal_pressure' [-Wmissing-prototypes]
> void arch_set_thermal_pressure(const struct cpumask *cpus,
> ^
> drivers/base/arch_topology.c:59:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> void arch_set_thermal_pressure(const struct cpumask *cpus,
> ^
> static
> 1 warning generated.
>
> vim +/arch_set_thermal_pressure +59 drivers/base/arch_topology.c
>
>     58
>   > 59	void arch_set_thermal_pressure(const struct cpumask *cpus,
>     60				       unsigned long th_pressure)
>     61	{
>     62		int cpu;
>     63
>     64		for_each_cpu(cpu, cpus)
>     65			WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
>     66	}
>     67
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Vincent Guittot]

On Sun, 14 Jun 2020 at 03:10, Valentin Schneider
<valentin.schneider@arm.com> 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, the public setter, arch_set_thermal_pressure(), is defined
> unconditionally in sched/core.c while the public getter,
> arch_scale_thermal_pressure(), is hardcoded to return 0 unless it has been
> 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.
>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  drivers/base/arch_topology.c      | 11 +++++++++++
>  drivers/thermal/cpufreq_cooling.c |  5 +++++
>  include/linux/arch_topology.h     |  3 ---
>  kernel/sched/core.c               | 11 -----------
>  4 files changed, 16 insertions(+), 14 deletions(-)
>
> 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/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> index e297e135c031..a1efd379b683 100644
> --- a/drivers/thermal/cpufreq_cooling.c
> +++ b/drivers/thermal/cpufreq_cooling.c
> @@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
>         return 0;
>  }
>
> +__weak void
> +arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
> +{
> +}

Having this weak function declared in cpufreq_cooling is weird. This
means that we will have to do so for each one that wants to use it.

Can't you declare an empty function in a common header file ?

> +
>  /**
>   * cpufreq_set_cur_state - callback function to set the current cooling state.
>   * @cdev: thermal cooling device pointer.
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 0566cb3314ef..81bd1c627195 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -39,9 +39,6 @@ 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);
> -
>  struct cpu_topology {
>         int thread_id;
>         int core_id;
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 43ba2d4a8eca..7861d21f3c2b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3628,17 +3628,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
>

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Ionela Voinescu]

Hi Vincent,

On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
> On Sun, 14 Jun 2020 at 03:10, Valentin Schneider
> <valentin.schneider@arm.com> wrote:
[..]
> > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> > index e297e135c031..a1efd379b683 100644
> > --- a/drivers/thermal/cpufreq_cooling.c
> > +++ b/drivers/thermal/cpufreq_cooling.c
> > @@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> >         return 0;
> >  }
> >
> > +__weak void
> > +arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
> > +{
> > +}
> 
> Having this weak function declared in cpufreq_cooling is weird. This
> means that we will have to do so for each one that wants to use it.
> 
> Can't you declare an empty function in a common header file ?

Do we expect anyone other than cpufreq_cooling to call
arch_set_thermal_pressure()?

I'm not against any of the options, either having it here as a week
default definition (same as done for arch_set_freq_scale() in cpufreq.c)
or in a common header (as done for arch_scale_freq_capacity() in sched.h).

But for me, Valentin's implementation seems more natural as setters are
usually only called from within the framework that does the control
(throttling for thermal or frequency setting for cpufreq) and we
probably want to think twice if we want to call them from other places.

Thanks,
Ionela.

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Valentin Schneider]

On 20/06/20 18:49, Ionela Voinescu wrote:
> Hi Vincent,
>
> On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
>> On Sun, 14 Jun 2020 at 03:10, Valentin Schneider
>> <valentin.schneider@arm.com> wrote:
> [..]
>> > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
>> > index e297e135c031..a1efd379b683 100644
>> > --- a/drivers/thermal/cpufreq_cooling.c
>> > +++ b/drivers/thermal/cpufreq_cooling.c
>> > @@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
>> >         return 0;
>> >  }
>> >
>> > +__weak void
>> > +arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
>> > +{
>> > +}
>>
>> Having this weak function declared in cpufreq_cooling is weird. This
>> means that we will have to do so for each one that wants to use it.
>>
>> Can't you declare an empty function in a common header file ?
>
> Do we expect anyone other than cpufreq_cooling to call
> arch_set_thermal_pressure()?
>
> I'm not against any of the options, either having it here as a week
> default definition (same as done for arch_set_freq_scale() in cpufreq.c)
> or in a common header (as done for arch_scale_freq_capacity() in sched.h).
>

Same thoughts here; I was going for the arch_set_freq_scale() way.

> But for me, Valentin's implementation seems more natural as setters are
> usually only called from within the framework that does the control
> (throttling for thermal or frequency setting for cpufreq) and we
> probably want to think twice if we want to call them from other places.
>

Well TBH I was tempted to go the other way and keep the definition in
core.c, given a simple per-cpu value is fairly generic. More precisely, it
seems somewhat awkward that architectures have to redefine those interfaces
when, given what cpufreq_cooling is doing, they'll have to go for per-cpu
storage in some way or another.

I ultimately decided against it, seeing as it isn't too difficult to come
up with other drivers of thermal pressure. There was that TDP-bound thing
[1], where IIUC you could end up with throttling not because of thermal but
because of power constraints. And then there's always FW that can cap stuff
as a last resort, and some architectures will want to inform the scheduler
of that when/if they'll be able to query FW for that.

[1]: 20200428032258.2518-1-currojerez@riseup.net

> Thanks,
> Ionela.

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Vincent Guittot]

On Sat, 20 Jun 2020 at 19:49, Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> Hi Vincent,
>
> On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
> > On Sun, 14 Jun 2020 at 03:10, Valentin Schneider
> > <valentin.schneider@arm.com> wrote:
> [..]
> > > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> > > index e297e135c031..a1efd379b683 100644
> > > --- a/drivers/thermal/cpufreq_cooling.c
> > > +++ b/drivers/thermal/cpufreq_cooling.c
> > > @@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> > >         return 0;
> > >  }
> > >
> > > +__weak void
> > > +arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
> > > +{
> > > +}
> >
> > Having this weak function declared in cpufreq_cooling is weird. This
> > means that we will have to do so for each one that wants to use it.
> >
> > Can't you declare an empty function in a common header file ?
>
> Do we expect anyone other than cpufreq_cooling to call
> arch_set_thermal_pressure()?

Yes, cpufreq cooling device is only 1 possible way to do thermal mitigation

>
> I'm not against any of the options, either having it here as a week
> default definition (same as done for arch_set_freq_scale() in cpufreq.c)
> or in a common header (as done for arch_scale_freq_capacity() in sched.h).
>
> But for me, Valentin's implementation seems more natural as setters are
> usually only called from within the framework that does the control
> (throttling for thermal or frequency setting for cpufreq) and we
> probably want to think twice if we want to call them from other places.
>
> Thanks,
> Ionela.

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Vincent Guittot]

On Sun, 21 Jun 2020 at 00:28, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
>
> On 20/06/20 18:49, Ionela Voinescu wrote:
> > Hi Vincent,
> >
> > On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
> >> On Sun, 14 Jun 2020 at 03:10, Valentin Schneider
> >> <valentin.schneider@arm.com> wrote:
> > [..]
> >> > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> >> > index e297e135c031..a1efd379b683 100644
> >> > --- a/drivers/thermal/cpufreq_cooling.c
> >> > +++ b/drivers/thermal/cpufreq_cooling.c
> >> > @@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> >> >         return 0;
> >> >  }
> >> >
> >> > +__weak void
> >> > +arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
> >> > +{
> >> > +}
> >>
> >> Having this weak function declared in cpufreq_cooling is weird. This
> >> means that we will have to do so for each one that wants to use it.
> >>
> >> Can't you declare an empty function in a common header file ?
> >
> > Do we expect anyone other than cpufreq_cooling to call
> > arch_set_thermal_pressure()?
> >
> > I'm not against any of the options, either having it here as a week
> > default definition (same as done for arch_set_freq_scale() in cpufreq.c)
> > or in a common header (as done for arch_scale_freq_capacity() in sched.h).
> >
>
> Same thoughts here; I was going for the arch_set_freq_scale() way.
>
> > But for me, Valentin's implementation seems more natural as setters are
> > usually only called from within the framework that does the control
> > (throttling for thermal or frequency setting for cpufreq) and we
> > probably want to think twice if we want to call them from other places.
> >
>
> Well TBH I was tempted to go the other way and keep the definition in
> core.c, given a simple per-cpu value is fairly generic. More precisely, it

Having all definitions in the same place is my main concern here.
If topology.c defines arch_set_thermal_pressure it should also provide
the empty function when the feature is not available or possible
instead of relying of each user of the interface to define a weak
function just in case.

> seems somewhat awkward that architectures have to redefine those interfaces
> when, given what cpufreq_cooling is doing, they'll have to go for per-cpu
> storage in some way or another.
>
> I ultimately decided against it, seeing as it isn't too difficult to come
> up with other drivers of thermal pressure. There was that TDP-bound thing
> [1], where IIUC you could end up with throttling not because of thermal but
> because of power constraints. And then there's always FW that can cap stuff
> as a last resort, and some architectures will want to inform the scheduler
> of that when/if they'll be able to query FW for that.
>
> [1]: 20200428032258.2518-1-currojerez@riseup.net
>
> > Thanks,
> > Ionela.

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Valentin Schneider]

Sorry for getting back to this only now;

On 22/06/20 09:37, Vincent Guittot wrote:
> On Sun, 21 Jun 2020 at 00:28, Valentin Schneider <valentin.schneider@arm.com> wrote:
>> On 20/06/20 18:49, Ionela Voinescu wrote:
>> > On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
>> >> Having this weak function declared in cpufreq_cooling is weird. This
>> >> means that we will have to do so for each one that wants to use it.
>> >>
>> >> Can't you declare an empty function in a common header file ?
>> >
>> > Do we expect anyone other than cpufreq_cooling to call
>> > arch_set_thermal_pressure()?
>> >
>> > I'm not against any of the options, either having it here as a week
>> > default definition (same as done for arch_set_freq_scale() in cpufreq.c)
>> > or in a common header (as done for arch_scale_freq_capacity() in sched.h).
>> >
>>
>> Same thoughts here; I was going for the arch_set_freq_scale() way.
>>
>> > But for me, Valentin's implementation seems more natural as setters are
>> > usually only called from within the framework that does the control
>> > (throttling for thermal or frequency setting for cpufreq) and we
>> > probably want to think twice if we want to call them from other places.
>> >
>>
>> Well TBH I was tempted to go the other way and keep the definition in
>> core.c, given a simple per-cpu value is fairly generic. More precisely, it
>
> Having all definitions in the same place is my main concern here.
> If topology.c defines arch_set_thermal_pressure it should also provide
> the empty function when the feature is not available or possible
> instead of relying of each user of the interface to define a weak
> function just in case.
>

include/linux/sched/topology.h already defines a stub for
arch_scale_thermal_pressure(), I suppose we could have one for
arch_set_thermal_pressure() there.

That would require having something like

#define arch_set_thermal_pressure topology_set_thermal_pressure

in the arm & arm64 include/asm/topology.h headers, with
topology_set_thermal_pressure() being what arch_set_thermal_pressure()
currently is in this patchset.


This would set an odd precedent in that so far we only ever had to #define
getter functions, the setters being either:
- entirely contained within arch_topology. (for the CPU scale)
- defined in arch_topology, declared in cpufreq and contained there (for
  the freq scale).

It made the most sense to me to follow the arch_set_freq_scale() pattern
and contain the thermal pressure setter within cpufreq_cooling, especially
since I didn't see a strong point in breaking the current patterns.

Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

[Vincent Guittot]

On Sun, 5 Jul 2020 at 16:19, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
>
> Sorry for getting back to this only now;
>
> On 22/06/20 09:37, Vincent Guittot wrote:
> > On Sun, 21 Jun 2020 at 00:28, Valentin Schneider <valentin.schneider@arm.com> wrote:
> >> On 20/06/20 18:49, Ionela Voinescu wrote:
> >> > On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
> >> >> Having this weak function declared in cpufreq_cooling is weird. This
> >> >> means that we will have to do so for each one that wants to use it.
> >> >>
> >> >> Can't you declare an empty function in a common header file ?
> >> >
> >> > Do we expect anyone other than cpufreq_cooling to call
> >> > arch_set_thermal_pressure()?
> >> >
> >> > I'm not against any of the options, either having it here as a week
> >> > default definition (same as done for arch_set_freq_scale() in cpufreq.c)
> >> > or in a common header (as done for arch_scale_freq_capacity() in sched.h).
> >> >
> >>
> >> Same thoughts here; I was going for the arch_set_freq_scale() way.
> >>
> >> > But for me, Valentin's implementation seems more natural as setters are
> >> > usually only called from within the framework that does the control
> >> > (throttling for thermal or frequency setting for cpufreq) and we
> >> > probably want to think twice if we want to call them from other places.
> >> >
> >>
> >> Well TBH I was tempted to go the other way and keep the definition in
> >> core.c, given a simple per-cpu value is fairly generic. More precisely, it
> >
> > Having all definitions in the same place is my main concern here.
> > If topology.c defines arch_set_thermal_pressure it should also provide
> > the empty function when the feature is not available or possible
> > instead of relying of each user of the interface to define a weak
> > function just in case.
> >
>
> include/linux/sched/topology.h already defines a stub for
> arch_scale_thermal_pressure(), I suppose we could have one for
> arch_set_thermal_pressure() there.
>
> That would require having something like
>
> #define arch_set_thermal_pressure topology_set_thermal_pressure
>
> in the arm & arm64 include/asm/topology.h headers, with
> topology_set_thermal_pressure() being what arch_set_thermal_pressure()
> currently is in this patchset.

That looks like a better solution IMO. At least everything is gathered
in the same place:
topology_get/set_thermal_pressure are in arch_topology.c
and arch_scale_thermal_pressure/arch_set_thermal_pressure in the
respective topology.h

>
>
> This would set an odd precedent in that so far we only ever had to #define
> getter functions, the setters being either:
> - entirely contained within arch_topology. (for the CPU scale)
> - defined in arch_topology, declared in cpufreq and contained there (for
>   the freq scale).
>
> It made the most sense to me to follow the arch_set_freq_scale() pattern
> and contain the thermal pressure setter within cpufreq_cooling, especially
> since I didn't see a strong point in breaking the current patterns.