Re: [Patch v9 7/8] sched/fair: Enable tuning of decay period

[Thara Gopinath]

On 02/10/2020 06:59 AM, Dietmar Eggemann wrote:
> On 07/02/2020 23:42, Thara Gopinath wrote:
>> On 02/04/2020 03:39 AM, Dietmar Eggemann wrote:
>>> On 03/02/2020 16:55, Peter Zijlstra wrote:
>>>> On Mon, Feb 03, 2020 at 07:07:57AM -0500, Thara Gopinath wrote:
>>>>> On 01/28/2020 06:56 PM, Randy Dunlap wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 1/28/20 2:36 PM, Thara Gopinath wrote:
> 
> [...]
> 
>>> I do agree. IMHO, there are just two little things outstanding:
>>>
>>> (1) arch_scale_thermal_pressure() instead  of
>>>     arch_cpu_thermal_pressure() in v8 4/7
>>
>> The "scale_" part was discussed in v6. Ionela had suggested that having
>> "scale" is not suited for this function because "thermal pressure" is
>> not exactly scaled but subtracted. I actually agree with that.
>>
>> https://lore.kernel.org/lkml/20191223175005.GA31446@xxxxxxx/
>>
>> Having said that if everyone feel the same about naming of this
>> function, I can change it one last time.
> 
> I'm still in favor for this. And Vincent seems to be OK as well:
> 
> https://lore.kernel.org/r/CAKfTPtBzoLnvAJ7sjPogMYS=WwBbdzWO07Kj=KDFVpO4=Su5ow@xxxxxxxxxxxxxx
> 
> The 'v6->v7' change note:
> 
> - Renamed arch_scale_thermal_capacity to arch_cpu_thermal_pressure
>   as per review comments from Peter, Dietmar and Ionela.
> 
> is really not saying from which review comment the individual changes in
> the function name are coming from. And I don't see an answer to Ionela's
> email saying that her proposal will manifest in a particular part of
> this change.
Hi Dietmar,

Like I said, don't want to argue on name. It is trivial for me. I have
v10 prepped with the name change. Will send it out shortly.

> 
>>> (2) guarding of thermal pressure code in Arm's arch_topology driver  w/
>>>     CONFIG_HAVE_SCHED_THERMAL_PRESSURE plus disabling it by default for
>>>     Arm64.
>> It was enabled by default as per your suggestion in v9.
>>
>> The patch can be dropped.
>>
>> I don't understand the need to guard arch_topology with
>> CONFIG_HAVE_SCHED_THERMAL_PRESSURE. CONFIG_HAVE_SCHED_THERMAL_PRESSURE
>> is for scheduler to enable/disable averaging of thermal pressure. We
>> wanted to separate updating and retrieving of instantaneous thermal
>> pressure from scheduler. Guarding it with
>> CONFIG_HAVE_SCHED_THERMAL_PRESSURE is to me equivalent to putting back
>> this whole code in the scheduler framework. I am against it. I also do
>> not see other arch_ functions guarded similarly.
> 
> Cpu-invariant accounting can't be guarded with a kernel CONFIG switch.
> Frequency-invariant accounting could be with CONFIG_CPU_FREQ but this is
> enabled by default by Arm64 defconfig.
> Thermal pressure (accounting) (CONFIG_HAVE_SCHED_THERMAL_PRESSURE) is
> disabled by default so why should a per-cpu thermal_pressure be
> maintained on such a system (CONFIG_CPU_THERMAL=y by default)?

I agree that there is no need for per-cpu thermal pressure to be
maintained if no averaging is happening in the scheduler, today. I don't
know if there will ever be an use for it.
My issue has to do with using a config option meant for internal
scheduler code being used else where. To me, once this happens, the
entire work done to separate out reading and writing of instantaneous
thermal pressure to arch_topology makes no sense. We could have kept it
in scheduler itself.
Another way I think about this whole thermal pressure framework  is that
it is the job of cooling device or cpufreq or any other entity to update
a throttle in maximum pressure to the scheduler. It should be
independent of what scheduler does with it. Scheduler can choose to
ignore it.

> 


-- 
Warm Regards
Thara