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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240728192913.ocesyzgsmcu632ds@airbuntu>
Date: Sun, 28 Jul 2024 20:29:13 +0100
From: Qais Yousef <qyousef@...alina.io>
To: Dietmar Eggemann <dietmar.eggemann@....com>
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 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