Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition | Date | Sat, 20 Jun 2020 23:28:19 +0100 |
| |
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.
| |