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

[Vincent Guittot]

On Thu, 7 Nov 2019 at 10:32, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> 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@xxxxxxx
> >>
> >> 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.
>
> [...]