linux-kernel - Re: [PATCH v5 15/23] PM: EM: Optimize em_cpu

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 16 Jan 2024 13:10:33 +0000
From: Qais Yousef <qyousef@...alina.io>
To: Lukasz Luba <lukasz.luba@....com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	rafael@...nel.org, dietmar.eggemann@....com, rui.zhang@...el.com,
	amit.kucheria@...durent.com, amit.kachhap@...il.com,
	daniel.lezcano@...aro.org, viresh.kumar@...aro.org,
	len.brown@...el.com, pavel@....cz, mhiramat@...nel.org,
	wvw@...gle.com
Subject: Re: [PATCH v5 15/23] PM: EM: Optimize em_cpu_energy() and remove
 division

On 01/15/24 12:36, Lukasz Luba wrote:
> 
> 
> On 1/15/24 12:21, Qais Yousef wrote:
> > On 01/10/24 13:53, Lukasz Luba wrote:
> > > 
> > > 
> > > On 1/4/24 19:23, Qais Yousef wrote:
> > > > On 01/02/24 11:47, Lukasz Luba wrote:
> > > > > > Did you see a problem or just being extra cautious here?
> > > > > 
> > > > > There is no problem, 'cost' is a private coefficient for EAS only.
> > > > 
> > > > Let me  ask differently, what goes wrong if you don't increase the resolution
> > > > here? Why is it necessary?
> > > > 
> > > 
> > > 
> > > When you have 800mW at CPU capacity 1024, then the value is small (below
> > > 1 thousand).
> > > Example:
> > > power = 800000 uW
> > > cost = 800000 / 1024 = 781
> > > 
> > > While I know from past that sometimes OPPs might have close voltage
> > > values and a rounding could occur and make some OPPs inefficient
> > > while they aren't.
> > > 
> > > This is what would happen when we have the 1x resolution:
> > > /sys/kernel/debug/energy_model/cpu4/ps:1008000/cost:551
> > > /sys/kernel/debug/energy_model/cpu4/ps:1200000/cost:644
> > > /sys/kernel/debug/energy_model/cpu4/ps:1416000/cost:744
> > > /sys/kernel/debug/energy_model/cpu4/ps:1512000/cost:851
> > > /sys/kernel/debug/energy_model/cpu4/ps:408000/cost:493
> > > /sys/kernel/debug/energy_model/cpu4/ps:600000/cost:493
> > > /sys/kernel/debug/energy_model/cpu4/ps:816000/cost:493
> > > The bottom 3 OPPs have the same 'cost' thus 2 OPPs are in-efficient,
> > > which is not true (see below).
> > > 
> > > This is what would happen when we have the 10x resolution:
> > > /sys/kernel/debug/energy_model/cpu4/ps:1008000/cost:5513
> > > /sys/kernel/debug/energy_model/cpu4/ps:1200000/cost:6443
> > > /sys/kernel/debug/energy_model/cpu4/ps:1416000/cost:7447
> > > /sys/kernel/debug/energy_model/cpu4/ps:1512000/cost:8514
> > > /sys/kernel/debug/energy_model/cpu4/ps:408000/cost:4934
> > > /sys/kernel/debug/energy_model/cpu4/ps:600000/cost:4933
> > > /sys/kernel/debug/energy_model/cpu4/ps:816000/cost:4934
> > > Here the OPP with 600MHz is more efficient than 408MHz,
> > > which is true. So only 408MHz will be marked as in-efficient OPP.
> > > 
> > > 
> > > This is what would happen when we have the 100x resolution:
> > > /sys/kernel/debug/energy_model/cpu4/ps:1008000/cost:55137
> > > /sys/kernel/debug/energy_model/cpu4/ps:1200000/cost:64433
> > > /sys/kernel/debug/energy_model/cpu4/ps:1416000/cost:74473
> > > /sys/kernel/debug/energy_model/cpu4/ps:1512000/cost:85140
> > > /sys/kernel/debug/energy_model/cpu4/ps:408000/cost:49346
> > > /sys/kernel/debug/energy_model/cpu4/ps:600000/cost:49331
> > > /sys/kernel/debug/energy_model/cpu4/ps:816000/cost:49346
> > > The higher (100x) resolution does not bring that much in
> > > practice.
> > 
> > So it seems a uW is not sufficient enough. We moved from mW because of
> > resolution already. Shall we make it nW then and multiply by 1000 always? The
> > choice of 10 looks arbitrary IMHO
> > 
> 
> No, there is no need of nW in the 'power' field for this.
> You've missed the point.

I think you're missing what I am saying. The multiplication by 10 looks like
magic value to increase resolution based on a single observation you noticed.

The feedback I am giving is that this needs to be better explained, in
a comment. And instead of multiplying by 10 multiply by 1000. Saying this is
enough based on a single observation is not adequate IMO.

Also the difference is tiny. Could you actually measure a difference in overall
power with and without this extra decimal point resolution? It might be better
to run at 816MHz and go back to idle faster. So the trade-off is not clear cut
to me.

So generally I am not keen on magic values based on single observations.
I think removing this or use 1000 is better.

AFAICT you decided that 0.1uW is worth caring about. But 0.19uW difference
isn't.

I can't see how much difference this makes in practice tbh. But using more
uniform conversion so that the cost is in nW (keep the power field in uW) makes
more sense at least.

It still raises the question whether this minuscule cost difference is actually
better taken into account. I think the perf/watt for 816MHz is much better so
skipping 600MHz as inefficient looks better to me.