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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <jhjmu4xcqyk.mognet@arm.com>
Date:   Sat, 20 Jun 2020 23:28:19 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Ionela Voinescu <ionela.voinescu@....com>
Cc:     Vincent Guittot <vincent.guittot@...aro.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        "open list\:THERMAL" <linux-pm@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Amit Daniel Kachhap <amit.kachhap@...il.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Russell King <linux@...linux.org.uk>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...nel.org>,
        LAK <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition


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@....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@...eup.net

> Thanks,
> Ionela.