[v5,2/6] sched/fair: Add infrastructure to store and update instantaneous thermal pressure

Message ID 1572979786-20361-3-git-send-email-thara.gopinath@linaro.org
State New
Headers show
Series
  • Introduce Thermal Pressure
Related show

Commit Message

Thara Gopinath Nov. 5, 2019, 6:49 p.m.
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 <thara.gopinath@linaro.org>

---

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(+)

-- 
2.1.4

Comments

Ionela Voinescu Nov. 5, 2019, 8:21 p.m. | #1
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

>
Thara Gopinath Nov. 5, 2019, 9:02 p.m. | #2
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
Ionela Voinescu Nov. 5, 2019, 9:15 p.m. | #3
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
Thara Gopinath Nov. 5, 2019, 9:29 p.m. | #4
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
Ionela Voinescu Nov. 5, 2019, 9:53 p.m. | #5
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
Vincent Guittot Nov. 6, 2019, 8:27 a.m. | #6
Hi Thara,


On Tue, 5 Nov 2019 at 19:49, Thara Gopinath <thara.gopinath@linaro.org> 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 <thara.gopinath@linaro.org>

> ---

>

> 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

>
Dietmar Eggemann Nov. 6, 2019, 12:50 p.m. | #7
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/379d23e5-79a5-9d90-0fb6-125d9be85e99@arm.com

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.
Thara Gopinath Nov. 6, 2019, 5 p.m. | #8
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 <thara.gopinath@linaro.org> 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 <thara.gopinath@linaro.org>

>> ---

>>

>> 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
Thara Gopinath Nov. 6, 2019, 5:53 p.m. | #9
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/379d23e5-79a5-9d90-0fb6-125d9be85e99@arm.com

> 

> 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
Dietmar Eggemann Nov. 7, 2019, 9:32 a.m. | #10
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/379d23e5-79a5-9d90-0fb6-125d9be85e99@arm.com

>>

>> 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.

[...]
Vincent Guittot Nov. 7, 2019, 10:48 a.m. | #11
On Thu, 7 Nov 2019 at 10:32, Dietmar Eggemann <dietmar.eggemann@arm.com> 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/379d23e5-79a5-9d90-0fb6-125d9be85e99@arm.com

> >>

> >> 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.

>

> [...]
Dietmar Eggemann Nov. 7, 2019, 11:36 a.m. | #12
On 07/11/2019 11:48, Vincent Guittot wrote:
> On Thu, 7 Nov 2019 at 10:32, Dietmar Eggemann <dietmar.eggemann@arm.com> 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/20191107104901.GA472@linaro.org 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() <--- ?

>>     ...

>> }


[...]
Amit Kucheria Nov. 19, 2019, 10:51 a.m. | #13
On Wed, Nov 6, 2019 at 12:20 AM Thara Gopinath
<thara.gopinath@linaro.org> 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 <thara.gopinath@linaro.org>

> ---

>

> 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

>

Patch

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:
  */