Thermal governors can respond to an overheat event of a cpu by
capping the cpu's maximum possible frequency. This in turn
means that the maximum available compute capacity of the
cpu is restricted. But today in the kernel, task scheduler is
not notified of capping of maximum frequency of a cpu.
In other words, scheduler is unaware of maximum capacity
restrictions placed on a cpu due to thermal activity.
This patch series attempts to address this issue.
The benefits identified are better task placement among available
cpus in event of overheating which in turn leads to better
performance numbers.
The reduction in the maximum possible capacity of a cpu due to a
thermal event can be considered as thermal pressure. Instantaneous
thermal pressure is hard to record and can sometime be erroneous
as there can be mismatch between the actual capping of capacity
and scheduler recording it. Thus solution is to have a weighted
average per cpu value for thermal pressure over time.
The weight reflects the amount of time the cpu has spent at a
capped maximum frequency. Since thermal pressure is recorded as
an average, it must be decayed periodically. Exisiting algorithm
in the kernel scheduler pelt framework is re-used to calculate
the weighted average. This patch series also defines a sysctl
inerface to allow for a configurable decay period.
Regarding testing, basic build, boot and sanity testing have been
performed on db845c platform with debian file system.
Further, dhrystone and hackbench tests have been
run with the thermal pressure algorithm. During testing, due to
constraints of step wise governor in dealing with big little systems,
trip point 0 temperature was made assymetric between cpus in little
cluster and big cluster; the idea being that
big core will heat up and cpu cooling device will throttle the
frequency of the big cores faster, there by limiting the maximum available
capacity and the scheduler will spread out tasks to little cores as well.
Test Results
Hackbench: 1 group , 30000 loops, 10 runs
Result SD
(Secs) (% of mean)
No Thermal Pressure 14.03 2.69%
Thermal Pressure PELT Algo. Decay : 32 ms 13.29 0.56%
Thermal Pressure PELT Algo. Decay : 64 ms 12.57 1.56%
Thermal Pressure PELT Algo. Decay : 128 ms 12.71 1.04%
Thermal Pressure PELT Algo. Decay : 256 ms 12.29 1.42%
Thermal Pressure PELT Algo. Decay : 512 ms 12.42 1.15%
Dhrystone Run Time : 20 threads, 3000 MLOOPS
Result SD
(Secs) (% of mean)
No Thermal Pressure 9.452 4.49%
Thermal Pressure PELT Algo. Decay : 32 ms 8.793 5.30%
Thermal Pressure PELT Algo. Decay : 64 ms 8.981 5.29%
Thermal Pressure PELT Algo. Decay : 128 ms 8.647 6.62%
Thermal Pressure PELT Algo. Decay : 256 ms 8.774 6.45%
Thermal Pressure PELT Algo. Decay : 512 ms 8.603 5.41%
A Brief History
The first version of this patch-series was posted with resuing
PELT algorithm to decay thermal pressure signal. The discussions
that followed were around whether intanteneous thermal pressure
solution is better and whether a stand-alone algortihm to accumulate
and decay thermal pressure is more appropriate than re-using the
PELT framework.
Tests on Hikey960 showed the stand-alone algorithm performing slightly
better than resuing PELT algorithm and V2 was posted with the stand
alone algorithm. Test results were shared as part of this series.
Discussions were around re-using PELT algorithm and running
further tests with more granular decay period.
For some time after this development was impeded due to hardware
unavailability, some other unforseen and possibly unfortunate events.
For this version, h/w was switched from hikey960 to db845c.
Also Instantaneous thermal pressure was never tested as part of this
cycle as it is clear that weighted average is a better implementation.
The non-PELT algorithm never gave any conclusive results to prove that it
is better than reusing PELT algorithm, in this round of testing.
Also reusing PELT algorithm means thermal pressure tracks the
other utilization signals in the scheduler.
v3->v4:
- "Patch 3/7:sched: Initialize per cpu thermal pressure structure"
is dropped as it is no longer needed following changes in other
other patches.
- rest of the change log mentioned in specific patches.
Thara Gopinath (6):
sched/pelt.c: Add support to track thermal pressure
sched/fair: Add infrastructure to store and update instantaneous
thermal pressure
sched/fair: Enable periodic update of thermal pressure
sched/fair: update cpu_capcity to reflect thermal pressure
thermal/cpu-cooling: Update thermal pressure in case of a maximum
frequency capping
sched/fair: Enable tuning of decay period
Documentation/admin-guide/kernel-parameters.txt | 5 ++
drivers/thermal/cpu_cooling.c | 36 ++++++++++++-
include/linux/sched.h | 9 ++++
kernel/sched/fair.c | 69 +++++++++++++++++++++++++
kernel/sched/pelt.c | 13 +++++
kernel/sched/pelt.h | 7 +++
kernel/sched/sched.h | 1 +
7 files changed, 138 insertions(+), 2 deletions(-)
--
2.1.4
Add interface APIs to initialize, update/average, track, accumulate
and decay thermal pressure per cpu basis. A per cpu variable
thermal_pressure is introduced to keep track of instantaneous per
cpu thermal pressure. Thermal pressure is the delta between maximum
capacity and capped capacity due to a thermal event.
API trigger_thermal_pressure_average is called for periodic accumulate
and decay of the thermal pressure.This API passes on the instantaneous
thermal pressure of a cpu to update_thermal_load_avg to do the necessary
accumulate, decay and average.
API update_thermal_pressure is for the system to update the thermal
pressure by providing a capped maximum capacity.
Considering, trigger_thermal_pressure_average reads thermal_pressure and
update_thermal_pressure writes into thermal_pressure, one can argue for
some sort of locking mechanism to avoid a stale value.
But considering trigger_thermal_pressure_average can be called from a
system critical path like scheduler tick function, a locking mechanism
is not ideal. This means that it is possible the thermal_pressure value
used to calculate average thermal pressure for a cpu can be
stale for upto 1 tick period.
Signed-off-by: Thara Gopinath <[email protected]>
---
v3->v4:
- Dropped per cpu max_capacity_info struct and instead added a per
delta_capacity variable to store the delta between maximum
capacity and capped capacity. The delta is now calculated when
thermal pressure is updated and not every tick.
- Dropped populate_max_capacity_info api as only per cpu delta
capacity is stored.
- Renamed update_periodic_maxcap to
trigger_thermal_pressure_average and update_maxcap_capacity to
update_thermal_pressure.
v4->v5:
- As per Peter's review comments folded thermal.c into fair.c.
- As per Ionela's review comments revamped update_thermal_pressure
to take maximum available capacity as input instead of maximum
capped frequency ration.
---
include/linux/sched.h | 9 +++++++++
kernel/sched/fair.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 263cf08..3c31084 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1993,6 +1993,15 @@ static inline void rseq_syscall(struct pt_regs *regs)
#endif
+#ifdef CONFIG_SMP
+void update_thermal_pressure(int cpu, unsigned long capped_capacity);
+#else
+static inline void
+update_thermal_pressure(int cpu, unsigned long capped_capacity)
+{
+}
+#endif
+
const struct sched_avg *sched_trace_cfs_rq_avg(struct cfs_rq *cfs_rq);
char *sched_trace_cfs_rq_path(struct cfs_rq *cfs_rq, char *str, int len);
int sched_trace_cfs_rq_cpu(struct cfs_rq *cfs_rq);
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 682a754..2e907cc 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -86,6 +86,12 @@ static unsigned int normalized_sysctl_sched_wakeup_granularity = 1000000UL;
const_debug unsigned int sysctl_sched_migration_cost = 500000UL;
+/*
+ * Per-cpu instantaneous delta between maximum capacity
+ * and maximum available capacity due to thermal events.
+ */
+static DEFINE_PER_CPU(unsigned long, thermal_pressure);
+
#ifdef CONFIG_SMP
/*
* For asym packing, by default the lower numbered CPU has higher priority.
@@ -10401,6 +10407,37 @@ static unsigned int get_rr_interval_fair(struct rq *rq, struct task_struct *task
return rr_interval;
}
+#ifdef CONFIG_SMP
+/**
+ * update_thermal_pressure: Update thermal pressure
+ * @cpu: the cpu for which thermal pressure is to be updated for
+ * @capped_capacity: maximum capacity of the cpu after the capping
+ * due to thermal event.
+ *
+ * Delta between the arch_scale_cpu_capacity and capped max capacity is
+ * stored in per cpu thermal_pressure variable.
+ */
+void update_thermal_pressure(int cpu, unsigned long capped_capacity)
+{
+ unsigned long delta;
+
+ delta = arch_scale_cpu_capacity(cpu) - capped_capacity;
+ per_cpu(thermal_pressure, cpu) = delta;
+}
+#endif
+
+/**
+ * trigger_thermal_pressure_average: Trigger the thermal pressure accumulate
+ * and average algorithm
+ */
+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
+}
+
/*
* All the scheduling class methods:
*/
--
2.1.4
cpu_capacity relflects the maximum available capacity of a cpu. Thermal
pressure on a cpu means this maximum available capacity is reduced. This
patch reduces the average thermal pressure for a cpu from its maximum
available capacity so that cpu_capacity reflects the actual
available capacity.
Other signals that are deducted from cpu_capacity to reflect the actual
capacity available are rt and dl util_avg. util_avg tracks a binary signal
and uses the weights 1024 and 0. Whereas thermal pressure is tracked
using load_avg. load_avg uses the actual "delta" capacity as the weight.
Signed-off-by: Thara Gopinath <[email protected]>
---
kernel/sched/fair.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 9fb0494..5f6c371 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)
used = READ_ONCE(rq->avg_rt.util_avg);
used += READ_ONCE(rq->avg_dl.util_avg);
+ used += READ_ONCE(rq->avg_thermal.load_avg);
if (unlikely(used >= max))
return 1;
--
2.1.4
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.
Thanks,
Ionela.
> +
> /*
> * All the scheduling class methods:
> */
> --
> 2.1.4
>
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
>
> Thanks,
> Ionela.
>
>> +
>> /*
>> * All the scheduling class methods:
>> */
>> --
>> 2.1.4
>>
--
Warm Regards
Thara
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.
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
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.
>
> 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
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
Hi Thara,
On Tue, 5 Nov 2019 at 19:49, Thara Gopinath <[email protected]> wrote:
>
> Add interface APIs to initialize, update/average, track, accumulate
> and decay thermal pressure per cpu basis. A per cpu variable
> thermal_pressure is introduced to keep track of instantaneous per
> cpu thermal pressure. Thermal pressure is the delta between maximum
> capacity and capped capacity due to a thermal event.
> API trigger_thermal_pressure_average is called for periodic accumulate
> and decay of the thermal pressure.This API passes on the instantaneous
> thermal pressure of a cpu to update_thermal_load_avg to do the necessary
> accumulate, decay and average.
> API update_thermal_pressure is for the system to update the thermal
> pressure by providing a capped maximum capacity.
> Considering, trigger_thermal_pressure_average reads thermal_pressure and
> update_thermal_pressure writes into thermal_pressure, one can argue for
> some sort of locking mechanism to avoid a stale value.
> But considering trigger_thermal_pressure_average can be called from a
> system critical path like scheduler tick function, a locking mechanism
> is not ideal. This means that it is possible the thermal_pressure value
> used to calculate average thermal pressure for a cpu can be
> stale for upto 1 tick period.
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
>
> v3->v4:
> - Dropped per cpu max_capacity_info struct and instead added a per
> delta_capacity variable to store the delta between maximum
> capacity and capped capacity. The delta is now calculated when
> thermal pressure is updated and not every tick.
> - Dropped populate_max_capacity_info api as only per cpu delta
> capacity is stored.
> - Renamed update_periodic_maxcap to
> trigger_thermal_pressure_average and update_maxcap_capacity to
> update_thermal_pressure.
> v4->v5:
> - As per Peter's review comments folded thermal.c into fair.c.
> - As per Ionela's review comments revamped update_thermal_pressure
> to take maximum available capacity as input instead of maximum
> capped frequency ration.
>
> ---
> include/linux/sched.h | 9 +++++++++
> kernel/sched/fair.c | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 263cf08..3c31084 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1993,6 +1993,15 @@ static inline void rseq_syscall(struct pt_regs *regs)
>
> #endif
>
> +#ifdef CONFIG_SMP
> +void update_thermal_pressure(int cpu, unsigned long capped_capacity);
> +#else
> +static inline void
> +update_thermal_pressure(int cpu, unsigned long capped_capacity)
> +{
> +}
> +#endif
> +
> const struct sched_avg *sched_trace_cfs_rq_avg(struct cfs_rq *cfs_rq);
> char *sched_trace_cfs_rq_path(struct cfs_rq *cfs_rq, char *str, int len);
> int sched_trace_cfs_rq_cpu(struct cfs_rq *cfs_rq);
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 682a754..2e907cc 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -86,6 +86,12 @@ static unsigned int normalized_sysctl_sched_wakeup_granularity = 1000000UL;
>
> const_debug unsigned int sysctl_sched_migration_cost = 500000UL;
>
> +/*
> + * Per-cpu instantaneous delta between maximum capacity
> + * and maximum available capacity due to thermal events.
> + */
> +static DEFINE_PER_CPU(unsigned long, thermal_pressure);
> +
> #ifdef CONFIG_SMP
> /*
> * For asym packing, by default the lower numbered CPU has higher priority.
> @@ -10401,6 +10407,37 @@ static unsigned int get_rr_interval_fair(struct rq *rq, struct task_struct *task
> return rr_interval;
> }
>
> +#ifdef CONFIG_SMP
> +/**
> + * update_thermal_pressure: Update thermal pressure
> + * @cpu: the cpu for which thermal pressure is to be updated for
> + * @capped_capacity: maximum capacity of the cpu after the capping
> + * due to thermal event.
> + *
> + * Delta between the arch_scale_cpu_capacity and capped max capacity is
> + * stored in per cpu thermal_pressure variable.
> + */
> +void update_thermal_pressure(int cpu, unsigned long capped_capacity)
> +{
> + unsigned long delta;
> +
> + delta = arch_scale_cpu_capacity(cpu) - capped_capacity;
> + per_cpu(thermal_pressure, cpu) = delta;
use WRITE_ONCE
> +}
> +#endif
> +
> +/**
> + * trigger_thermal_pressure_average: Trigger the thermal pressure accumulate
> + * and average algorithm
> + */
> +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
> +}
> +
> /*
> * All the scheduling class methods:
> */
> --
> 2.1.4
>
On 05/11/2019 22:53, Ionela Voinescu wrote:
> 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.
Coming back to the v4 discussion
https://lore.kernel.org/r/[email protected]
There is no API between pelt.c and other parts of the scheduler/kernel
so why should we keep an unnecessary parameter and wrapper functions?
There is also no abstraction, update_thermal_load_avg() in pelt.c even
carries '_thermal_' in its name.
So why not define this shift value '[sched_|pelt_]thermal_decay_shift'
there as well? It belongs to update_thermal_load_avg().
All call sites of update_thermal_load_avg() use 'rq_clock_task(rq) >>
sched_thermal_decay_shift' so I don't see the need to pass it in.
IMHO, preparing for eventual code changes (e.g. parsing different now
values) is not a good practice in the kernel. Keeping the code small and
tidy is.
On 11/05/19 13:49, Thara Gopinath wrote:
> cpu_capacity relflects the maximum available capacity of a cpu. Thermal
> pressure on a cpu means this maximum available capacity is reduced. This
> patch reduces the average thermal pressure for a cpu from its maximum
> available capacity so that cpu_capacity reflects the actual
> available capacity.
>
> Other signals that are deducted from cpu_capacity to reflect the actual
> capacity available are rt and dl util_avg. util_avg tracks a binary signal
> and uses the weights 1024 and 0. Whereas thermal pressure is tracked
> using load_avg. load_avg uses the actual "delta" capacity as the weight.
I think you intended to put this as comment...
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> kernel/sched/fair.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 9fb0494..5f6c371 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)
>
> used = READ_ONCE(rq->avg_rt.util_avg);
> used += READ_ONCE(rq->avg_dl.util_avg);
> + used += READ_ONCE(rq->avg_thermal.load_avg);
... here?
I find the explanation hard to parse too. Do you think you can rephrase it?
Something based on what you wrote here would be more understandable IMHO:
https://lore.kernel.org/lkml/[email protected]/
Thanks!
--
Qais Yousef
>
> if (unlikely(used >= max))
> return 1;
> --
> 2.1.4
>
Hi Vincent,
Thanks for the review
On 11/06/2019 03:27 AM, Vincent Guittot wrote:
> Hi Thara,
>
>
> On Tue, 5 Nov 2019 at 19:49, Thara Gopinath <[email protected]> wrote:
>>
>> Add interface APIs to initialize, update/average, track, accumulate
>> and decay thermal pressure per cpu basis. A per cpu variable
>> thermal_pressure is introduced to keep track of instantaneous per
>> cpu thermal pressure. Thermal pressure is the delta between maximum
>> capacity and capped capacity due to a thermal event.
>> API trigger_thermal_pressure_average is called for periodic accumulate
>> and decay of the thermal pressure.This API passes on the instantaneous
>> thermal pressure of a cpu to update_thermal_load_avg to do the necessary
>> accumulate, decay and average.
>> API update_thermal_pressure is for the system to update the thermal
>> pressure by providing a capped maximum capacity.
>> Considering, trigger_thermal_pressure_average reads thermal_pressure and
>> update_thermal_pressure writes into thermal_pressure, one can argue for
>> some sort of locking mechanism to avoid a stale value.
>> But considering trigger_thermal_pressure_average can be called from a
>> system critical path like scheduler tick function, a locking mechanism
>> is not ideal. This means that it is possible the thermal_pressure value
>> used to calculate average thermal pressure for a cpu can be
>> stale for upto 1 tick period.
>>
>> Signed-off-by: Thara Gopinath <[email protected]>
>> ---
>>
>> v3->v4:
>> - Dropped per cpu max_capacity_info struct and instead added a per
>> delta_capacity variable to store the delta between maximum
>> capacity and capped capacity. The delta is now calculated when
>> thermal pressure is updated and not every tick.
>> - Dropped populate_max_capacity_info api as only per cpu delta
>> capacity is stored.
>> - Renamed update_periodic_maxcap to
>> trigger_thermal_pressure_average and update_maxcap_capacity to
>> update_thermal_pressure.
>> v4->v5:
>> - As per Peter's review comments folded thermal.c into fair.c.
>> - As per Ionela's review comments revamped update_thermal_pressure
>> to take maximum available capacity as input instead of maximum
>> capped frequency ration.
>>
>> ---
>> include/linux/sched.h | 9 +++++++++
>> kernel/sched/fair.c | 37 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 46 insertions(+)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 263cf08..3c31084 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1993,6 +1993,15 @@ static inline void rseq_syscall(struct pt_regs *regs)
>>
>> #endif
>>
>> +#ifdef CONFIG_SMP
>> +void update_thermal_pressure(int cpu, unsigned long capped_capacity);
>> +#else
>> +static inline void
>> +update_thermal_pressure(int cpu, unsigned long capped_capacity)
>> +{
>> +}
>> +#endif
>> +
>> const struct sched_avg *sched_trace_cfs_rq_avg(struct cfs_rq *cfs_rq);
>> char *sched_trace_cfs_rq_path(struct cfs_rq *cfs_rq, char *str, int len);
>> int sched_trace_cfs_rq_cpu(struct cfs_rq *cfs_rq);
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 682a754..2e907cc 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -86,6 +86,12 @@ static unsigned int normalized_sysctl_sched_wakeup_granularity = 1000000UL;
>>
>> const_debug unsigned int sysctl_sched_migration_cost = 500000UL;
>>
>> +/*
>> + * Per-cpu instantaneous delta between maximum capacity
>> + * and maximum available capacity due to thermal events.
>> + */
>> +static DEFINE_PER_CPU(unsigned long, thermal_pressure);
>> +
>> #ifdef CONFIG_SMP
>> /*
>> * For asym packing, by default the lower numbered CPU has higher priority.
>> @@ -10401,6 +10407,37 @@ static unsigned int get_rr_interval_fair(struct rq *rq, struct task_struct *task
>> return rr_interval;
>> }
>>
>> +#ifdef CONFIG_SMP
>> +/**
>> + * update_thermal_pressure: Update thermal pressure
>> + * @cpu: the cpu for which thermal pressure is to be updated for
>> + * @capped_capacity: maximum capacity of the cpu after the capping
>> + * due to thermal event.
>> + *
>> + * Delta between the arch_scale_cpu_capacity and capped max capacity is
>> + * stored in per cpu thermal_pressure variable.
>> + */
>> +void update_thermal_pressure(int cpu, unsigned long capped_capacity)
>> +{
>> + unsigned long delta;
>> +
>> + delta = arch_scale_cpu_capacity(cpu) - capped_capacity;
>> + per_cpu(thermal_pressure, cpu) = delta;
>
> use WRITE_ONCE
Will do.
>
>> +}
>> +#endif
>> +
>> +/**
>> + * trigger_thermal_pressure_average: Trigger the thermal pressure accumulate
>> + * and average algorithm
>> + */
>> +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
>> +}
>> +
>> /*
>> * All the scheduling class methods:
>> */
>> --
>> 2.1.4
>>
--
Warm Regards
Thara
On 11/06/2019 11:56 AM, Qais Yousef wrote:
> On 11/05/19 13:49, Thara Gopinath wrote:
>> cpu_capacity relflects the maximum available capacity of a cpu. Thermal
>> pressure on a cpu means this maximum available capacity is reduced. This
>> patch reduces the average thermal pressure for a cpu from its maximum
>> available capacity so that cpu_capacity reflects the actual
>> available capacity.
>>
>> Other signals that are deducted from cpu_capacity to reflect the actual
>> capacity available are rt and dl util_avg. util_avg tracks a binary signal
>> and uses the weights 1024 and 0. Whereas thermal pressure is tracked
>> using load_avg. load_avg uses the actual "delta" capacity as the weight.
>
> I think you intended to put this as comment...
>
>>
>> Signed-off-by: Thara Gopinath <[email protected]>
>> ---
>> kernel/sched/fair.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 9fb0494..5f6c371 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)
>>
>> used = READ_ONCE(rq->avg_rt.util_avg);
>> used += READ_ONCE(rq->avg_dl.util_avg);
>> + used += READ_ONCE(rq->avg_thermal.load_avg);
>
> ... here?
I did not! But I can.
>
> I find the explanation hard to parse too. Do you think you can rephrase it?
> Something based on what you wrote here would be more understandable IMHO:
> https://lore.kernel.org/lkml/[email protected]/
I will try to rephrase it! I am sorry that you found it hard to parse.
Honestly, I cannot copy paste the code snippet I pointed out to you here
in comment.(And I think that is the reason you found it easier to
understand) But I will try my best to put it in words.
>
>
> Thanks!
>
> --
> Qais Yousef
>
>>
>> if (unlikely(used >= max))
>> return 1;
>> --
>> 2.1.4
>>
--
Warm Regards
Thara
On 11/06/19 12:31, Thara Gopinath wrote:
> On 11/06/2019 11:56 AM, Qais Yousef wrote:
> > On 11/05/19 13:49, Thara Gopinath wrote:
> >> cpu_capacity relflects the maximum available capacity of a cpu. Thermal
> >> pressure on a cpu means this maximum available capacity is reduced. This
> >> patch reduces the average thermal pressure for a cpu from its maximum
> >> available capacity so that cpu_capacity reflects the actual
> >> available capacity.
> >>
> >> Other signals that are deducted from cpu_capacity to reflect the actual
> >> capacity available are rt and dl util_avg. util_avg tracks a binary signal
> >> and uses the weights 1024 and 0. Whereas thermal pressure is tracked
> >> using load_avg. load_avg uses the actual "delta" capacity as the weight.
> >
> > I think you intended to put this as comment...
> >
> >>
> >> Signed-off-by: Thara Gopinath <[email protected]>
> >> ---
> >> kernel/sched/fair.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 9fb0494..5f6c371 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)
> >>
> >> used = READ_ONCE(rq->avg_rt.util_avg);
> >> used += READ_ONCE(rq->avg_dl.util_avg);
> >> + used += READ_ONCE(rq->avg_thermal.load_avg);
> >
> > ... here?
>
> I did not! But I can.
> >
> > I find the explanation hard to parse too. Do you think you can rephrase it?
> > Something based on what you wrote here would be more understandable IMHO:
> > https://lore.kernel.org/lkml/[email protected]/
> I will try to rephrase it! I am sorry that you found it hard to parse.
> Honestly, I cannot copy paste the code snippet I pointed out to you here
> in comment.(And I think that is the reason you found it easier to
> understand) But I will try my best to put it in words.
No worries. The problem could be me :-)
But a comment in the code is very important as util_avg + load_avg is confusing
without a comment. I wouldn't expect both signal to be compatible but the
thermal one is special. A comment explaining why it's special is all we need.
Thanks
--
Qais Yousef
On 11/06/2019 07:50 AM, Dietmar Eggemann wrote:
> On 05/11/2019 22:53, Ionela Voinescu wrote:
>> 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.
>
> Coming back to the v4 discussion
> https://lore.kernel.org/r/[email protected]
>
> There is no API between pelt.c and other parts of the scheduler/kernel
> so why should we keep an unnecessary parameter and wrapper functions?
>
> There is also no abstraction, update_thermal_load_avg() in pelt.c even
> carries '_thermal_' in its name.
>
> So why not define this shift value '[sched_|pelt_]thermal_decay_shift'
> there as well? It belongs to update_thermal_load_avg().
>
> All call sites of update_thermal_load_avg() use 'rq_clock_task(rq) >>
> sched_thermal_decay_shift' so I don't see the need to pass it in.
>
> IMHO, preparing for eventual code changes (e.g. parsing different now
> values) is not a good practice in the kernel. Keeping the code small and
> tidy is.
I think we are going in circles on this one. I acknowledge you have an
issue. That being said, I also understand the need to keep the pelt
framework code tight. Also Ionela pointed out that there could be a need
for a faster decay in which case it could mean a left shift leading to
further complications if defined in pelt.c (I am not saying that I will
implement a faster decay in this patch set but it is more of a future
extension if needed!)
I can make trigger_thermal_pressure_average inline if that will
alleviate some of the concerns.
I would also urge you to reconsider the merits of arguing this point
back and forth.
>
--
Warm Regards
Thara
On 06/11/2019 18:53, Thara Gopinath wrote:
> On 11/06/2019 07:50 AM, Dietmar Eggemann wrote:
>> On 05/11/2019 22:53, Ionela Voinescu wrote:
>>> 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.
>>
>> Coming back to the v4 discussion
>> https://lore.kernel.org/r/[email protected]
>>
>> There is no API between pelt.c and other parts of the scheduler/kernel
>> so why should we keep an unnecessary parameter and wrapper functions?
>>
>> There is also no abstraction, update_thermal_load_avg() in pelt.c even
>> carries '_thermal_' in its name.
>>
>> So why not define this shift value '[sched_|pelt_]thermal_decay_shift'
>> there as well? It belongs to update_thermal_load_avg().
>>
>> All call sites of update_thermal_load_avg() use 'rq_clock_task(rq) >>
>> sched_thermal_decay_shift' so I don't see the need to pass it in.
>>
>> IMHO, preparing for eventual code changes (e.g. parsing different now
>> values) is not a good practice in the kernel. Keeping the code small and
>> tidy is.
>
> I think we are going in circles on this one. I acknowledge you have an
> issue. That being said, I also understand the need to keep the pelt
> framework code tight. Also Ionela pointed out that there could be a need
> for a faster decay in which case it could mean a left shift leading to
> further complications if defined in pelt.c (I am not saying that I will
> implement a faster decay in this patch set but it is more of a future
> extension if needed!)
The issue still exists so why not discussing it here?
Placing thermal related time shift operations in a
update_*thermal*_load_avg() PELT function look perfectly fine to me.
> I can make trigger_thermal_pressure_average inline if that will
> alleviate some of the concerns.
That's not the issue here. The issue is the extra shim layer which is
unnecessary in the current implementation.
update_blocked_averages()
{
...
update_rt_rq_load_avg()
update_dl_rq_load_avg()
update_irq_load_avg()
trigger_thermal_pressure_average() <--- ?
...
}
Wouldn't a direct call to update_thermal_load_avg() here make things so
much more clear? And I'm not talking about today and about people
involved in this review.
> I would also urge you to reconsider the merits of arguing this point
> back and forth.
I just did.
[...]
On Thu, 7 Nov 2019 at 10:32, Dietmar Eggemann <[email protected]> wrote:
>
> On 06/11/2019 18:53, Thara Gopinath wrote:
> > On 11/06/2019 07:50 AM, Dietmar Eggemann wrote:
> >> On 05/11/2019 22:53, Ionela Voinescu wrote:
> >>> 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.
> >>
> >> Coming back to the v4 discussion
> >> https://lore.kernel.org/r/[email protected]
> >>
> >> There is no API between pelt.c and other parts of the scheduler/kernel
> >> so why should we keep an unnecessary parameter and wrapper functions?
> >>
> >> There is also no abstraction, update_thermal_load_avg() in pelt.c even
> >> carries '_thermal_' in its name.
> >>
> >> So why not define this shift value '[sched_|pelt_]thermal_decay_shift'
> >> there as well? It belongs to update_thermal_load_avg().
> >>
> >> All call sites of update_thermal_load_avg() use 'rq_clock_task(rq) >>
> >> sched_thermal_decay_shift' so I don't see the need to pass it in.
> >>
> >> IMHO, preparing for eventual code changes (e.g. parsing different now
> >> values) is not a good practice in the kernel. Keeping the code small and
> >> tidy is.
> >
> > I think we are going in circles on this one. I acknowledge you have an
> > issue. That being said, I also understand the need to keep the pelt
> > framework code tight. Also Ionela pointed out that there could be a need
> > for a faster decay in which case it could mean a left shift leading to
> > further complications if defined in pelt.c (I am not saying that I will
> > implement a faster decay in this patch set but it is more of a future
> > extension if needed!)
>
> The issue still exists so why not discussing it here?
>
> Placing thermal related time shift operations in a
> update_*thermal*_load_avg() PELT function look perfectly fine to me.
As already said, having thermal related clock shift operation in
update_thermal_load_avg and pelt.c is a nack for me
>
> > I can make trigger_thermal_pressure_average inline if that will
> > alleviate some of the concerns.
In fact, trigger_thermal_pressure_average is only there because of
shifting the clock which is introduced only in patch 6.
So remove trigger_thermal_pressure_average from this patch and call directly
+ update_thermal_load_avg(rq_clock_task(rq), rq,
+ per_cpu(thermal_pressure, cpu_of(rq)));
in patch3
>
> That's not the issue here. The issue is the extra shim layer which is
> unnecessary in the current implementation.
>
> update_blocked_averages()
> {
> ...
> update_rt_rq_load_avg()
> update_dl_rq_load_avg()
> update_irq_load_avg()
> trigger_thermal_pressure_average() <--- ?
> ...
> }
>
> Wouldn't a direct call to update_thermal_load_avg() here make things so
> much more clear? And I'm not talking about today and about people
> involved in this review.
>
> > I would also urge you to reconsider the merits of arguing this point
> > back and forth.
>
> I just did.
>
> [...]
On 07/11/2019 11:48, Vincent Guittot wrote:
> On Thu, 7 Nov 2019 at 10:32, Dietmar Eggemann <[email protected]> wrote:
>>
>> On 06/11/2019 18:53, Thara Gopinath wrote:
>>> On 11/06/2019 07:50 AM, Dietmar Eggemann wrote:
>>>> On 05/11/2019 22:53, Ionela Voinescu wrote:
>>>>> 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:
[...]
> In fact, trigger_thermal_pressure_average is only there because of
> shifting the clock which is introduced only in patch 6.
> So remove trigger_thermal_pressure_average from this patch and call directly
>
> + update_thermal_load_avg(rq_clock_task(rq), rq,
> + per_cpu(thermal_pressure, cpu_of(rq)));
>
> in patch3
I like the rq_clock_thermal() idea in
https://lore.kernel.org/r/[email protected] to get rid of
trigger_thermal_pressure_average().
>> That's not the issue here. The issue is the extra shim layer which is
>> unnecessary in the current implementation.
>>
>> update_blocked_averages()
>> {
>> ...
>> update_rt_rq_load_avg()
>> update_dl_rq_load_avg()
>> update_irq_load_avg()
>> trigger_thermal_pressure_average() <--- ?
>> ...
>> }
[...]
Hi Thara,
I am going to try your patch set on some different board.
To do that I need more information regarding your setup.
Please find my comments below. I need probably one hack
which do not fully understand.
On 11/5/19 6:49 PM, Thara Gopinath wrote:
> Thermal governors can respond to an overheat event of a cpu by
> capping the cpu's maximum possible frequency. This in turn
> means that the maximum available compute capacity of the
> cpu is restricted. But today in the kernel, task scheduler is
> not notified of capping of maximum frequency of a cpu.
> In other words, scheduler is unaware of maximum capacity
> restrictions placed on a cpu due to thermal activity.
> This patch series attempts to address this issue.
> The benefits identified are better task placement among available
> cpus in event of overheating which in turn leads to better
> performance numbers.
>
> The reduction in the maximum possible capacity of a cpu due to a
> thermal event can be considered as thermal pressure. Instantaneous
> thermal pressure is hard to record and can sometime be erroneous
> as there can be mismatch between the actual capping of capacity
> and scheduler recording it. Thus solution is to have a weighted
> average per cpu value for thermal pressure over time.
> The weight reflects the amount of time the cpu has spent at a
> capped maximum frequency. Since thermal pressure is recorded as
> an average, it must be decayed periodically. Exisiting algorithm
> in the kernel scheduler pelt framework is re-used to calculate
> the weighted average. This patch series also defines a sysctl
> inerface to allow for a configurable decay period.
>
> Regarding testing, basic build, boot and sanity testing have been
> performed on db845c platform with debian file system.
> Further, dhrystone and hackbench tests have been
> run with the thermal pressure algorithm. During testing, due to
> constraints of step wise governor in dealing with big little systems,
I don't understand this modification. Could you explain what was the
issue and if this modification did not break the original
thermal solution upfront? You are then comparing this modified
version and treat it as an 'origin', am I right?
> trip point 0 temperature was made assymetric between cpus in little
> cluster and big cluster; the idea being that
> big core will heat up and cpu cooling device will throttle the
> frequency of the big cores faster, there by limiting the maximum available
> capacity and the scheduler will spread out tasks to little cores as well.
>
> Test Results
>
> Hackbench: 1 group , 30000 loops, 10 runs
> Result SD
> (Secs) (% of mean)
> No Thermal Pressure 14.03 2.69%
> Thermal Pressure PELT Algo. Decay : 32 ms 13.29 0.56%
> Thermal Pressure PELT Algo. Decay : 64 ms 12.57 1.56%
> Thermal Pressure PELT Algo. Decay : 128 ms 12.71 1.04%
> Thermal Pressure PELT Algo. Decay : 256 ms 12.29 1.42%
> Thermal Pressure PELT Algo. Decay : 512 ms 12.42 1.15%
>
> Dhrystone Run Time : 20 threads, 3000 MLOOPS
> Result SD
> (Secs) (% of mean)
> No Thermal Pressure 9.452 4.49%
> Thermal Pressure PELT Algo. Decay : 32 ms 8.793 5.30%
> Thermal Pressure PELT Algo. Decay : 64 ms 8.981 5.29%
> Thermal Pressure PELT Algo. Decay : 128 ms 8.647 6.62%
> Thermal Pressure PELT Algo. Decay : 256 ms 8.774 6.45%
> Thermal Pressure PELT Algo. Decay : 512 ms 8.603 5.41%
>
What I would like to see also for this performance results is
avg temperature of the chip. Is it higher than in the 'origin'?
Regards,
Lukasz Luba
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Wed, Nov 6, 2019 at 12:20 AM Thara Gopinath
<[email protected]> wrote:
>
> cpu_capacity relflects the maximum available capacity of a cpu. Thermal
s/relflects/reflects
> pressure on a cpu means this maximum available capacity is reduced. This
> patch reduces the average thermal pressure for a cpu from its maximum
> available capacity so that cpu_capacity reflects the actual
> available capacity.
>
> Other signals that are deducted from cpu_capacity to reflect the actual
> capacity available are rt and dl util_avg. util_avg tracks a binary signal
> and uses the weights 1024 and 0. Whereas thermal pressure is tracked
> using load_avg. load_avg uses the actual "delta" capacity as the weight.
Consider rephrasing as,
Currently, effective cpu capacity is calculated by deducting RT and DL
average utilization (util_avg) from cpu_capacity. For thermal
pressure, we use load_avg instead of util_avg which is a binary signal
(0 or 1024) because <put why here>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> kernel/sched/fair.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 9fb0494..5f6c371 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7738,6 +7738,7 @@ static unsigned long scale_rt_capacity(struct sched_domain *sd, int cpu)
>
> used = READ_ONCE(rq->avg_rt.util_avg);
> used += READ_ONCE(rq->avg_dl.util_avg);
> + used += READ_ONCE(rq->avg_thermal.load_avg);
>
> if (unlikely(used >= max))
> return 1;
> --
> 2.1.4
>
On Wed, Nov 6, 2019 at 12:20 AM Thara Gopinath
<[email protected]> wrote:
>
> Thermal governors can respond to an overheat event of a cpu by
> capping the cpu's maximum possible frequency. This in turn
> means that the maximum available compute capacity of the
> cpu is restricted. But today in the kernel, task scheduler is
> not notified of capping of maximum frequency of a cpu.
> In other words, scheduler is unaware of maximum capacity
> restrictions placed on a cpu due to thermal activity.
> This patch series attempts to address this issue.
> The benefits identified are better task placement among available
> cpus in event of overheating which in turn leads to better
> performance numbers.
>
> The reduction in the maximum possible capacity of a cpu due to a
> thermal event can be considered as thermal pressure. Instantaneous
> thermal pressure is hard to record and can sometime be erroneous
> as there can be mismatch between the actual capping of capacity
> and scheduler recording it. Thus solution is to have a weighted
> average per cpu value for thermal pressure over time.
> The weight reflects the amount of time the cpu has spent at a
> capped maximum frequency. Since thermal pressure is recorded as
> an average, it must be decayed periodically. Exisiting algorithm
> in the kernel scheduler pelt framework is re-used to calculate
> the weighted average. This patch series also defines a sysctl
> inerface to allow for a configurable decay period.
>
> Regarding testing, basic build, boot and sanity testing have been
> performed on db845c platform with debian file system.
> Further, dhrystone and hackbench tests have been
> run with the thermal pressure algorithm. During testing, due to
> constraints of step wise governor in dealing with big little systems,
What contraints?
> trip point 0 temperature was made assymetric between cpus in little
> cluster and big cluster; the idea being that
> big core will heat up and cpu cooling device will throttle the
> frequency of the big cores faster, there by limiting the maximum available
> capacity and the scheduler will spread out tasks to little cores as well.
Can you share the hack to get this behaviour as well so I can try to
reproduce on 845c?
> Test Results
>
> Hackbench: 1 group , 30000 loops, 10 runs
> Result SD
> (Secs) (% of mean)
> No Thermal Pressure 14.03 2.69%
> Thermal Pressure PELT Algo. Decay : 32 ms 13.29 0.56%
> Thermal Pressure PELT Algo. Decay : 64 ms 12.57 1.56%
> Thermal Pressure PELT Algo. Decay : 128 ms 12.71 1.04%
> Thermal Pressure PELT Algo. Decay : 256 ms 12.29 1.42%
> Thermal Pressure PELT Algo. Decay : 512 ms 12.42 1.15%
>
On Wed, Nov 6, 2019 at 12:20 AM Thara Gopinath
<[email protected]> wrote:
>
> Add interface APIs to initialize, update/average, track, accumulate
> and decay thermal pressure per cpu basis. A per cpu variable
*on a* per cpu basis.
> thermal_pressure is introduced to keep track of instantaneous per
> cpu thermal pressure. Thermal pressure is the delta between maximum
> capacity and capped capacity due to a thermal event.
> API trigger_thermal_pressure_average is called for periodic accumulate
s/accumulate/accumulation
> and decay of the thermal pressure.This API passes on the instantaneous
> thermal pressure of a cpu to update_thermal_load_avg to do the necessary
> accumulate, decay and average.
s/accumulate/accumulation
Add blank line here.
> API update_thermal_pressure is for the system to update the thermal
> pressure by providing a capped maximum capacity.
This is redundant given the below sentence.
> Considering, trigger_thermal_pressure_average reads thermal_pressure and
Lose the comma.
> update_thermal_pressure writes into thermal_pressure, one can argue for
> some sort of locking mechanism to avoid a stale value.
> But considering trigger_thermal_pressure_average can be called from a
> system critical path like scheduler tick function, a locking mechanism
> is not ideal. This means that it is possible the thermal_pressure value
> used to calculate average thermal pressure for a cpu can be
> stale for upto 1 tick period.
Please consider reflowing all your patch descriptions to make the
paragraphs better aligned and easier to read. Leave a blank line
between paragraphs.
In vim, you can do :gqap at each paragraph.
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
>
> v3->v4:
> - Dropped per cpu max_capacity_info struct and instead added a per
> delta_capacity variable to store the delta between maximum
> capacity and capped capacity. The delta is now calculated when
> thermal pressure is updated and not every tick.
> - Dropped populate_max_capacity_info api as only per cpu delta
> capacity is stored.
> - Renamed update_periodic_maxcap to
> trigger_thermal_pressure_average and update_maxcap_capacity to
> update_thermal_pressure.
> v4->v5:
> - As per Peter's review comments folded thermal.c into fair.c.
> - As per Ionela's review comments revamped update_thermal_pressure
> to take maximum available capacity as input instead of maximum
> capped frequency ration.
>
> ---
> include/linux/sched.h | 9 +++++++++
> kernel/sched/fair.c | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 263cf08..3c31084 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1993,6 +1993,15 @@ static inline void rseq_syscall(struct pt_regs *regs)
>
> #endif
>
> +#ifdef CONFIG_SMP
> +void update_thermal_pressure(int cpu, unsigned long capped_capacity);
> +#else
> +static inline void
> +update_thermal_pressure(int cpu, unsigned long capped_capacity)
> +{
> +}
> +#endif
> +
> const struct sched_avg *sched_trace_cfs_rq_avg(struct cfs_rq *cfs_rq);
> char *sched_trace_cfs_rq_path(struct cfs_rq *cfs_rq, char *str, int len);
> int sched_trace_cfs_rq_cpu(struct cfs_rq *cfs_rq);
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 682a754..2e907cc 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -86,6 +86,12 @@ static unsigned int normalized_sysctl_sched_wakeup_granularity = 1000000UL;
>
> const_debug unsigned int sysctl_sched_migration_cost = 500000UL;
>
> +/*
> + * Per-cpu instantaneous delta between maximum capacity
> + * and maximum available capacity due to thermal events.
> + */
> +static DEFINE_PER_CPU(unsigned long, thermal_pressure);
> +
> #ifdef CONFIG_SMP
> /*
> * For asym packing, by default the lower numbered CPU has higher priority.
> @@ -10401,6 +10407,37 @@ static unsigned int get_rr_interval_fair(struct rq *rq, struct task_struct *task
> return rr_interval;
> }
>
> +#ifdef CONFIG_SMP
> +/**
> + * update_thermal_pressure: Update thermal pressure
> + * @cpu: the cpu for which thermal pressure is to be updated for
> + * @capped_capacity: maximum capacity of the cpu after the capping
> + * due to thermal event.
> + *
> + * Delta between the arch_scale_cpu_capacity and capped max capacity is
> + * stored in per cpu thermal_pressure variable.
> + */
> +void update_thermal_pressure(int cpu, unsigned long capped_capacity)
> +{
> + unsigned long delta;
> +
> + delta = arch_scale_cpu_capacity(cpu) - capped_capacity;
> + per_cpu(thermal_pressure, cpu) = delta;
Any reason you to save to delta first and then to the per-cpu
variable? Just make it
per_cpu(thermal_pressure, cpu) = arch_scale_cpu_capacity(cpu) -
capped_capacity;
> +}
> +#endif
> +
> +/**
> + * trigger_thermal_pressure_average: Trigger the thermal pressure accumulate
> + * and average algorithm
> + */
> +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
> +}
> +
> /*
> * All the scheduling class methods:
> */
> --
> 2.1.4
>
On 11/12/19 11:21 AM, Lukasz Luba wrote:
> Hi Thara,
>
> I am going to try your patch set on some different board.
> To do that I need more information regarding your setup.
> Please find my comments below. I need probably one hack
> which do not fully understand.
>
> On 11/5/19 6:49 PM, Thara Gopinath wrote:
>> Thermal governors can respond to an overheat event of a cpu by
>> capping the cpu's maximum possible frequency. This in turn
>> means that the maximum available compute capacity of the
>> cpu is restricted. But today in the kernel, task scheduler is
>> not notified of capping of maximum frequency of a cpu.
>> In other words, scheduler is unaware of maximum capacity
>> restrictions placed on a cpu due to thermal activity.
>> This patch series attempts to address this issue.
>> The benefits identified are better task placement among available
>> cpus in event of overheating which in turn leads to better
>> performance numbers.
>>
>> The reduction in the maximum possible capacity of a cpu due to a
>> thermal event can be considered as thermal pressure. Instantaneous
>> thermal pressure is hard to record and can sometime be erroneous
>> as there can be mismatch between the actual capping of capacity
>> and scheduler recording it. Thus solution is to have a weighted
>> average per cpu value for thermal pressure over time.
>> The weight reflects the amount of time the cpu has spent at a
>> capped maximum frequency. Since thermal pressure is recorded as
>> an average, it must be decayed periodically. Exisiting algorithm
>> in the kernel scheduler pelt framework is re-used to calculate
>> the weighted average. This patch series also defines a sysctl
>> inerface to allow for a configurable decay period.
>>
>> Regarding testing, basic build, boot and sanity testing have been
>> performed on db845c platform with debian file system.
>> Further, dhrystone and hackbench tests have been
>> run with the thermal pressure algorithm. During testing, due to
>> constraints of step wise governor in dealing with big little systems,
> I don't understand this modification. Could you explain what was the
> issue and if this modification did not break the original
> thermal solution upfront? You are then comparing this modified
> version and treat it as an 'origin', am I right?
With Ionela's help I understood the reason for doing this hack.
For those who follow: She created a 'capacity inversion' between
big and little cores to tests if the patches really work.
How: she starts throttling big cores at lower temperature, so earlier
in time, thus the power is shifted towards little cores (which are more
energy efficient and can run with higher frequency). The big cores
run at minimum frequency and little (hopefully) at max frequency.
This 'capacity inversion' is the use case which might occur in the
real world. It is hard to trigger it in normal benchmarks, though.
I don't know how often this 'capacity inversion' occurs and for how
long it stays in real workloads. Based on the tests run with default
thermal solution and results almost the same, I would say that it is
not often (maybe 3% of the test period, otherwise I would get better
results because this patch set solves this issue).
I have run a few different kernels and benchmarks without this
'capacity inversions' and I don't see the regression (and benefits
from this solution), which is also a big plus in case of mainlining it.
In case where the 'capacity inversion' is artificially introduced
into the system for 100% time, the stress tests show huge difference.
Please refer to Ionela's test results [1] (~30% better).
Regards,
Lukasz Luba
[1]
https://docs.google.com/spreadsheets/d/1ibxDSSSLTodLzihNAw6jM36eVZABuPMMnjvV-Xh4NEo/edit#gid=0