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

quoted

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 [off-list ref] 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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, 5 Jul 2020 at 16:19, Valentin Schneider
[off-list ref] 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 [off-list ref] 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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help