linux-kernel - Re: [RESEND PATCH 2/2] sched/fair: Optimize __update_sched

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: <20170330185002.GC16440@ydu19desktop>
Date:   Fri, 31 Mar 2017 02:50:02 +0800
From:   Yuyang Du <yuyang.du@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Paul Turner <pjt@...gle.com>, Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Benjamin Segall <bsegall@...gle.com>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        umgwanakikbuti@...il.com
Subject: Re: [RESEND PATCH 2/2] sched/fair: Optimize __update_sched_avg()

On Thu, Mar 30, 2017 at 03:46:58PM +0200, Peter Zijlstra wrote:
> On Thu, Mar 30, 2017 at 02:16:58PM +0200, Peter Zijlstra wrote:
> > On Thu, Mar 30, 2017 at 04:21:08AM -0700, Paul Turner wrote:
> 
> > > - The naming here is really ambiguous:
> > >     "__accumulate_sum" -> "__accumulate_pelt_segments"?
> > 
> > OK, I did struggle with that a bit too but failed to improve, I'll change it.
> > 
> > > - Passing in "remainder" seems irrelevant to the sum accumulation.  It would be
> > >   more clear to handle it from the caller.
> > 
> > Well, this way we have all 3 delta parts in one function. I'll try it
> > and see what it looks like though.
> 
> > > This is super confusing.  It only works because remainder already had
> > > period_contrib aggregated _into_ it.  We're literally computing:
> > >   remainder + period_contrib - period_contrib
> > 
> > Correct; although I didn't find it too confusing. Could be because I'd
> > been staring at this code for a few hours though.
> > 
> > > We should just not call this in the !periods case and handle the remainder
> > > below.
> > 
> > I'll change it see what it looks like.
> 
> How's this?

That is good. Only:
 
> ---
>  kernel/sched/fair.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 76f67b3e34d6..10d34498b5fe 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2795,12 +2795,9 @@ static u64 decay_load(u64 val, u64 n)
>  	return val;
>  }
>  
> -static u32 __accumulate_sum(u64 periods, u32 period_contrib, u32 remainder)
> +static u32 __accumulate_pelt_segments(u64 periods, u32 d1, u32 d3)
>  {
> -	u32 c1, c2, c3 = remainder; /* y^0 == 1 */
> -
> -	if (!periods)
> -		return remainder - period_contrib;
> +	u32 c1, c2, c3 = d3; /* y^0 == 1 */
>  
>  	if (unlikely(periods >= LOAD_AVG_MAX_N))
>  		return LOAD_AVG_MAX;
> @@ -2861,8 +2858,8 @@ accumulate_sum(u64 delta, int cpu, struct sched_avg *sa,
>  	       unsigned long weight, int running, struct cfs_rq *cfs_rq)
>  {
>  	unsigned long scale_freq, scale_cpu;
> +	u32 contrib = delta;

u64 -> u32 may raise some question, so better cast it to show confidence
at the first.