Messages in this thread | | | From | Vincent Guittot <> | Date | Fri, 24 Jan 2020 16:45:13 +0100 | Subject | Re: [Patch v8 4/7] sched/fair: Enable periodic update of average thermal pressure |
| |
On Fri, 24 Jan 2020 at 16:37, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 17/01/2020 16:39, Vincent Guittot wrote: > > On Fri, 17 Jan 2020 at 15:55, Peter Zijlstra <peterz@infradead.org> wrote: > >> > >> On Fri, Jan 17, 2020 at 02:22:51PM +0100, Vincent Guittot wrote: > >>> On Thu, 16 Jan 2020 at 16:15, Peter Zijlstra <peterz@infradead.org> wrote: > >> > >>>> > >>>> That there indentation trainwreck is a reason to rename the function. > >>>> > >>>> decayed = update_rt_rq_load_avg(now, rq, curr_class == &rt_sched_class) | > >>>> update_dl_rq_load_avg(now, rq, curr_class == &dl_sched_class) | > >>>> update_thermal_load_avg(rq_clock_task(rq), rq, thermal_pressure) | > >>>> update_irq_load_avg(rq, 0); > >>>> > >>>> Is much better. > >>>> > >>>> But now that you made me look at that, I noticed it's using a different > >>>> clock -- it is _NOT_ using now/rq_clock_pelt(), which means it'll not be > >>>> in sync with the other averages. > >>>> > >>>> Is there a good reason for that? > >>> > >>> We don't need to apply frequency and cpu capacity invariance on the > >>> thermal capping signal which is what rq_clock_pelt does > >> > >> Hmm, I suppose that is true, and that really could've done with a > >> comment. Now clock_pelt is sort-of in sync with clock_task, but won't it > >> still give weird artifacts by having it on a slightly different basis? > > > > No we should not. Weird artifacts happens when we > > add/subtract/propagate signals between each other and then apply pelt > > algorithm on the results. In the case of thermal signal, we only add > > it to others to update cpu_capacity but pelt algo is then not applied > > on it. The error because of some signals being at segment boundaries > > whereas others are not, is limited to 2% and doesn't accumulate over > > time. > > > >> > >> Anyway, looking at this, would it make sense to remove the @now argument > >> from update_*_load_avg()? All those functions already take @rq, and > >> rq_clock_*() are fairly trivial inlines. > > > > TBH I was thinking of doing the opposite for update_irq_load_avg which > > hides the clock that is used for irq_avg. This helps to easily > > identify which signals use the exact same clock and can be mixed to > > create a new pelt signal and which can't > > The 'now' argument is one thing but why not: > > -int update_thermal_load_avg(u64 now, struct rq *rq, u64 capacity) > +int update_thermal_load_avg(u64 now, struct rq *rq) > { > + u64 capacity = arch_cpu_thermal_pressure(cpu_of(rq)); > + > if (___update_load_sum(now, &rq->avg_thermal, > > This would make the call-sites __update_blocked_others() and > task_tick(_fair)() cleaner.
I prefer to keep the capacity as argument. This is more aligned with others that provides the value of the signal to apply
> > I guess the argument is not to pollute pelt.c. But it already contains
you've got it. I don't want to pollute the pelt.c file with things not related to pelt but thermal as an example.
> arch_scale_[freq|cpu]_capacity() for irq. > >
| |