Re: [PATCH v6] sched: Consolidate cpufreq updates

[Qais Yousef]

On 07/27/24 10:15, Dietmar Eggemann wrote:
> On 24/07/2024 23:10, Qais Yousef wrote:
> > On 07/05/24 13:50, Dietmar Eggemann wrote:
> 
> [...]
> 
> >> I tried to explained it in the 4 lines below. With a local 'decayed'
> >> update_cfs_rq_load_avg() and propagate_entity_load_avg() set it every
> >> time update_load_avg() gets called. And this then determines whether
> >> update_tg_load_avg() is called on this cfs_rq later in update_load_avg().
> >>
> >> The new code:
> >>
> >>   cfs_rq->decayed |= update_cfs_rq_load_avg() (*)
> >>   cfs_rq->decayed |= propagate_entity_load_avg()
> >>
> >> will not reset 'cfs_rq->decayed' for non-root cfs_rq's.
> >>
> >> (*) You changed this in v3 from:
> >>
> >>   cfs_rq->decayed  = update_cfs_rq_load_avg()
> >>
> >>
> >>>> update_load_avg() itself. They will stay decayed after cfs_rq->decayed
> >>>> has been set to 1 once and will never be reset to 0. So with UPDATE_TG
> >>>> update_tg_load_avg() will then always be called on those non-root
> >>>> cfs_rq's all the time.
> >>>
> >>> We could add a check to update only the root cfs_rq. But what do we gain? Or
> >>> IOW, what is the harm of unconditionally updating cfs_rq->decayed given that we
> >>> only care about the root cfs_rq? I see more if conditions and branches which
> >>> I am trying to avoid.
> >>
> >> Yes, keep 'decayed' local and add a:
> >>
> >>     if (cfs_rq == &rq_of(cfs_rq)->cfs)
> >>         cfs_rq->decayed = decayed
> > 
> > I still don't see a problem here. If we don't do it this way, how the outcome
> > of frequency selection will change? You're replacing set-but-not-cleared with
> > never-set, and un unconditional write with a branch.
> 
> This seems to be a misunderstanding. The problem is not with the
> frequency selection.
> 
> The issue I see is when you set 'cfs_rq->decayed' for a non-root cfs_rq
> it stays set forever since there is no code to reset it. This leads to
> the fact that update_tg_load_avg(..., UPDATE_TG) is then always called
> for this cfs_rq within update_load_avg() whereas before 'decayed' was
> evaluated with each invocation of update_load_avg().

Ah I see you're talking about the if condition. I got you now, sorry it took
a while :). Yes this was wrong. I did follow your advice anyway and changed it
as you suggested in v7.

Thanks!

--
Qais Yousef