Messages in this thread |  | | Subject | Re: [Patch v9 7/8] sched/fair: Enable tuning of decay period | From | Thara Gopinath <> | Date | Fri, 7 Feb 2020 17:42:20 -0500 |
| |
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: >>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >>>>> index e35b28e..be4147b 100644 >>>>> --- a/Documentation/admin-guide/kernel-parameters.txt >>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt >>>>> @@ -4376,6 +4376,11 @@ >>>>> incurs a small amount of overhead in the scheduler >>>>> but is useful for debugging and performance tuning. >>>>> >>>>> + sched_thermal_decay_shift= >>>>> + [KNL, SMP] Set decay shift for thermal pressure signal. >>>>> + Format: integer between 0 and 10 >>>>> + Default is 0. >>>>> + >>>> >>>> That tells an admin [or any reader] almost nothing about this kernel parameter >>>> or what it does. And nothing about what unit the value is in. >>>> Does the value 0 disable this feature? >>> >>> Thanks for the review. 0 does not disable "thermal pressure" feature. 0 >>> means the default decay period for averaging PELT signals (which is >>> usually 32 but configurable) will also be applied for thermal pressure >>> signal. A shift will shift the default decay period. >>> >>> You are right. It needs more explanation here. I will fix it and send v10. >> >> Or just send an update for this patch? I'm thinking most of this is >> looking good. > > 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@arm.com/
Having said that if everyone feel the same about naming of this function, I can change it one last time.
> > (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.
>
-- Warm Regards Thara
|  |