Re: [PATCH v6] sched: Consolidate cpufreq updates

[Dietmar Eggemann]

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

[...]