| Message ID | 20200614010755.9129-2-valentin.schneider@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sched, arch_topology: Thermal pressure configuration cleanup | expand |
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
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
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
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
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 >
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.
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.
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.
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.
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.
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.
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.
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(-)