On Sun, 21 Jun 2020 at 00:28, Valentin Schneider
<valentin.schnei...@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.schnei...@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-curroje...@riseup.net
>
> > Thanks,
> > Ionela.
