linux-kernel - Re: [PATCH v3 2/2] cpufreq: schedutil: Cleanup and document iowait boost

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:   Mon, 21 May 2018 11:11:08 +0100
From:   Patrick Bellasi <patrick.bellasi@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Joel Fernandes <joelaf@...gle.com>
Subject: Re: [PATCH v3 2/2] cpufreq: schedutil: Cleanup and document iowait
 boost

On 21-May 15:22, Viresh Kumar wrote:
> On 21-05-18, 09:51, Patrick Bellasi wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > +static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> > +			       unsigned int flags)
> > +{
> > +	bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
> > +
> > +	/* Reset boost if the CPU appears to have been idle enough */
> > +	if (sg_cpu->iowait_boost &&
> > +	    sugov_iowait_reset(sg_cpu, time, set_iowait_boost))
> > +		return;
> > +
> > +	/* Boost only tasks waking up after IO */
> > +	if (!set_iowait_boost)
> > +		return;
> > +
> > +	/* Ensure boost doubles only one time at each request */
> > +	if (sg_cpu->iowait_boost_pending)
> > +		return;
> > +	sg_cpu->iowait_boost_pending = true;
> > +
> > +	/* Double the boost at each request */
> > +	if (sg_cpu->iowait_boost) {
> > +		sg_cpu->iowait_boost <<= 1;
> > +		if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
> > +			sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> > +		return;
> 
> Maybe add "else" part of the if block and drop the "return" statement
> here ?

If not "mandatory", I would prefer as it is: I'm running with a small
stack size in my mind. :)

This "bail out of a function as soon as possible" template allows me
to see immediately that, for example in this function, once we decided
to double the boost value there is anything more to do here.

At the same time, the statement below it reads as the function
default action.

Does it make any sense?

[...]

> > +	/*
> > +	 * Apply the current boost value: a CPU is boosted only if its current
> > +	 * utilization is smaller then the current IO boost level.
> > +	 */
> >  	boost_util = sg_cpu->iowait_boost;
> >  	boost_max = sg_cpu->iowait_boost_max;
> > -
> 
> Maybe keep this blank line as is ?

I've removed it because the above comment now refers to all these
lines together.

> >  	if (*util * boost_max < *max * boost_util) {
> >  		*util = boost_util;
> >  		*max = boost_max;
> 
> Otherwise looks good.
> 
> Acked-by: Viresh Kumar <viresh.kumar@...aro.org>

Cheers

-- 
#include <best/regards.h>

Patrick Bellasi