Re: [PATCH v6] sched: Consolidate cpufreq updates

[Qais Yousef <qyousef@layalina.io>]

On 07/05/24 13:50, Dietmar Eggemann wrote:

> > Yes I am referring to SCHED_IDLE policy too. What is your expectation? AFAIK
> > the goal of this policy to run when there's nothing else needs running.
> 
> IMHO, SCHED_IDLE tasks fight with all the other FAIR task over the
> resource rq. I would include SCHED_IDLE into this switch statement next
> to SCHED_NORMAL and SCHED_BATCH.
> What do you do if only SCHED_IDLE FAIR tasks are runnable? They probably
> also want to have their CPU frequency needs adjusted.

Okay I added it. I think there's room for improvements, but will pursue this in
future patches after more thinking.

> > I am not seeing the issue, could you expand on what is it?
> 
> 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.

I updated the code to only set for root cfs_rq anyway in spite of not seeing
any tangible benefit.


Thanks!

--
Qais Yousef