linux-kernel - Re: [PATCH] cpufreq: Change default transition delay to 2ms

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]
Message-ID: <20240205215410.3opxw4ty2tpbsgbc@airbuntu>
Date: Mon, 5 Feb 2024 21:54:10 +0000
From: Qais Yousef <qyousef@...alina.io>
To: Christian Loehle <christian.loehle@....com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>
Subject: Re: [PATCH] cpufreq: Change default transition delay to 2ms

On 02/05/24 17:35, Christian Loehle wrote:
> On 05/02/2024 12:01, Qais Yousef wrote:
> > Hi Christian
> > 
> > On 02/05/24 09:17, Christian Loehle wrote:
> >> On 05/02/2024 02:25, Qais Yousef wrote:
> >>> 10ms is too high for today's hardware, even low end ones. This default
> >>> end up being used a lot on Arm machines at least. Pine64, mac mini and
> >>> pixel 6 all end up with 10ms rate_limit_us when using schedutil, and
> >>> it's too high for all of them.
> >>>
> >>> Change the default to 2ms which should be 'pessimistic' enough for worst
> >>> case scenario, but not too high for platforms with fast DVFS hardware.
> >>>
> >>> Signed-off-by: Qais Yousef <qyousef@...alina.io>
> >>> ---
> >>>  drivers/cpufreq/cpufreq.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >>> index 44db4f59c4cc..8207f7294cb6 100644
> >>> --- a/drivers/cpufreq/cpufreq.c
> >>> +++ b/drivers/cpufreq/cpufreq.c
> >>> @@ -582,11 +582,11 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
> >>>  		 * for platforms where transition_latency is in milliseconds, it
> >>>  		 * ends up giving unrealistic values.
> >>>  		 *
> >>> -		 * Cap the default transition delay to 10 ms, which seems to be
> >>> +		 * Cap the default transition delay to 2 ms, which seems to be
> >>>  		 * a reasonable amount of time after which we should reevaluate
> >>>  		 * the frequency.
> >>>  		 */
> >>> -		return min(latency * LATENCY_MULTIPLIER, (unsigned int)10000);
> >>> +		return min(latency * LATENCY_MULTIPLIER, (unsigned int)(2*MSEC_PER_SEC));
> >>>  	}
> >>>  
> >>>  	return LATENCY_MULTIPLIER;
> >>
> >> Hi Qais,
> >> as previously mentioned I'm working on improving iowait boost and while I'm not against
> >> this patch per se it does make iowait boosting more aggressive. ((Doubling limited by rate_limit_us)
> >> Since the boost is often applied when not useful (for Android e.g. periodic f2fs writebacks),
> >> this might have some side effects. Please give me a couple of days for verifying any impact,
> >> or did you do that already?
> > 
> > I don't understand the concern, could you elaborate more please?
> > 
> > Products already ship with 500us and 1ms which is lower than this 2ms.
> > 
> > On my AMD desktop it is already 1ms. And I think I've seen Intel systems
> > defaulting to 500us or something low too. Ideally cpufreq drivers should set
> > policy->transition_delay_us; so this path is taken if the driver didn't
> > populate that. Which seems to be more common than I'd like tbh.
> 
> I'm not disagreeing with you on that part. I'm just worried about the side
> effects w.r.t iowait boosting.

I don't see a reason for that. This value should represent hardware's ability
to change frequencies. It is not designed for iowait boost. And it being high
means folks with good hardware are getting crap performance as changing
frequency once every 10ms with today's bursty workloads means we leave a lot of
perf on the floor for no good reason. And as I tried to explain, already
platforms ship with low value as this is how the hardware behaves. We are not
making iowait boost more aggressive; but bringing the fallback behavior more
inline to what properly configured platforms behave already today.

> 
> > 
> > I never run with 10ms. It's too slow. But I had several tests in the past
> > against 2ms posted for those margin and removal of uclamp-max aggregation
> > series. Anyway. I ran PCMark storage on Pixel 6 (running mainlinish kernel) and
> > I see
> > 
> > 10ms: 27600
> > 2ms: 29750
> 
> Yes, decreasing the rate limit makes it more aggressive, nothing unexpected here.
> But let's be frank, the scenarios in which iowait boosting actually shows its
> biggest benefit you are either doing:
> - Random Read (small iosize), single-threaded, synchronous IO
> - anything O_DIRECT
> and I'd argue more than likely you are doing something wrong if you're actually in
> such a case in the real world. So I'm much more worried about boosting in scenarios
> where it doesn't help (e.g. on an Android quite frequently: f2fs page cache writeback).
> 
> Decreasing the default transition latency makes (sugov) iowait boosting much more aggressive,
> so I'm curious if this patch increases power consumption on systems that were at 10ms previously
> when in non-IO workloads.
> 
> Hope that clears that up. Again, not an argument against your change, just being cautious of
> the potential side effects and if they need some mitigations.
> 
> Kind Regards,
> Christian