linux-kernel - Re: [Patch v5 2/6] sched/fair: Add infrastructure to store and update instantaneous thermal pressure

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]

Date:   Tue, 5 Nov 2019 21:53:41 +0000
From:   Ionela Voinescu <ionela.voinescu@....com>
To:     Thara Gopinath <thara.gopinath@...aro.org>
Cc:     mingo@...hat.com, peterz@...radead.org, vincent.guittot@...aro.org,
        rui.zhang@...el.com, edubezval@...il.com, qperret@...gle.com,
        linux-kernel@...r.kernel.org, amit.kachhap@...il.com,
        javi.merino@...nel.org, daniel.lezcano@...aro.org
Subject: Re: [Patch v5 2/6] sched/fair: Add infrastructure to store and
 update instantaneous thermal pressure

On Tuesday 05 Nov 2019 at 16:29:32 (-0500), Thara Gopinath wrote:
> On 11/05/2019 04:15 PM, Ionela Voinescu wrote:
> > On Tuesday 05 Nov 2019 at 16:02:00 (-0500), Thara Gopinath wrote:
> >> On 11/05/2019 03:21 PM, Ionela Voinescu wrote:
> >>> Hi Thara,
> >>>
> >>> On Tuesday 05 Nov 2019 at 13:49:42 (-0500), Thara Gopinath wrote:
> >>> [...]
> >>>> +static void trigger_thermal_pressure_average(struct rq *rq)
> >>>> +{
> >>>> +#ifdef CONFIG_SMP
> >>>> +	update_thermal_load_avg(rq_clock_task(rq), rq,
> >>>> +				per_cpu(thermal_pressure, cpu_of(rq)));
> >>>> +#endif
> >>>> +}
> >>>
> >>> Why did you decide to keep trigger_thermal_pressure_average and not
> >>> call update_thermal_load_avg directly?
> >>>
> >>> For !CONFIG_SMP you already have an update_thermal_load_avg function
> >>> that does nothing, in kernel/sched/pelt.h, so you don't need that
> >>> ifdef. 
> >> Hi,
> >>
> >> Yes you are right. But later with the shift option added, I shift
> >> rq_clock_task(rq) by the shift. I thought it is better to contain it in
> >> a function that replicate it in three different places. I can remove the
> >> CONFIG_SMP in the next version.
> > 
> > You could still keep that in one place if you shift the now argument of
> > ___update_load_sum instead.
> 
> No. I cannot do this. The authors of the pelt framework  prefers not to
> include a shift parameter there. I had discussed this with Vincent earlier.
> 

Right! I missed Vincent's last comment on this in v4. 

I would argue that it's more of a PELT parameter than a CFS parameter
:), where it's currently being used. I would also argue that's more of a
PELT parameter than a thermal parameter. It controls the PELT time
progression for the thermal signal, but it seems more to configure the
PELT algorithm, rather than directly characterize thermals.

In any case, it only seemed to me that adding a wrapper function for
this purpose only was not worth doing.

Thank you for clarifying,
Ionela.

> > 
> > To me that trigger_thermal_pressure_average function seems more code
> > than it's worth for this.
> > 
> > Thanks,
> > Ionela.
> > 
> >>>
> >>> Thanks,
> >>> Ionela.
> >>>
> >>>> +
> >>>>  /*
> >>>>   * All the scheduling class methods:
> >>>>   */
> >>>> -- 
> >>>> 2.1.4
> >>>>
> >>
> >>
> >> -- 
> >> Warm Regards
> >> Thara
> 
> 
> -- 
> Warm Regards
> Thara