linux-kernel - Re: [PATCH v6] sched: Consolidate cpufreq updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <962a0fe6-c6bf-415a-9c57-ecb259bd0066@arm.com>
Date: Sat, 27 Jul 2024 10:15:26 +0200
From: Dietmar Eggemann <dietmar.eggemann@....com>
To: Qais Yousef <qyousef@...alina.io>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>, Ingo Molnar <mingo@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>,
 Vincent Guittot <vincent.guittot@...aro.org>,
 Juri Lelli <juri.lelli@...hat.com>, Steven Rostedt <rostedt@...dmis.org>,
 Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
 Daniel Bristot de Oliveira <bristot@...hat.com>,
 Valentin Schneider <vschneid@...hat.com>,
 Christian Loehle <christian.loehle@....com>,
 Hongyan Xia <hongyan.xia2@....com>, John Stultz <jstultz@...gle.com>,
 linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] sched: Consolidate cpufreq updates

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

[...]