linux-kernel - Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq to its 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 1 May 2017 17:56:04 -0400
From:   Tejun Heo <tj@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Vincent Guittot <vincent.guittot@...aro.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Mike Galbraith <efault@....de>, Paul Turner <pjt@...gle.com>,
        Chris Mason <clm@...com>, kernel-team@...com
Subject: Re: [PATCH 1/2] sched/fair: Fix how load gets propagated from cfs_rq
 to its sched_entity

Hello, Peter.

On Mon, May 01, 2017 at 04:17:33PM +0200, Peter Zijlstra wrote:
> So this here does:
> 
>   ( tg->load_avg = \Sum cfs_rq->load_avg )
> 
>   load      = cfs_rq->load.weight
> 
>   tg_weight = tg->load_avg - cfs_rq->contrib + load
> 
> 
>            tg->shares * load
>   shares = -----------------
>                tg_weight
> 
> 
>                         cfs_rq->load_avg
>   avg_shares = shares * ----------------
>                               load
> 
> 	       tg->shares * cfs_rq->load_avg
>              = -----------------------------
>                       tg_weight
> 
> 
>   ( se->load.weight = shares )
> 
>   se->load_avg = min(shares, avg_shares);
> 
> 
> So where shares (and se->load.weight) are an upper bound (due to using
> cfs_rq->load.weight, see calc_cfs_shares), avg_shares is supposedly a
> more accurate representation based on our PELT averages.
> 
> This looks OK; and I agree with Vincent that we should use
> cfs_rq->avg.load_avg, not cfs_rq->runnable_load_avg, since tg->load_avg
> is a sum of the former, not the latter.

With this, we end up using a different metric for picking the busiest
queue depending on whether there are nested cfs_rq's or not.  The
root's runnable_load_avg ends up including blocked load avgs queued
behind nested cfs_rq's because we lose the resolution across threads
across nesting.

> Also, arguably calculating the above avg_shares directly (using the
> second equation) might be more precise; but I doubt it makes much of a
> difference, however since we do min(), we should at least clamp against
> MIN_SHARES again.
> 
> Furthermore, it appears to me we want a different tg_weight value for
> the avg_shares, something like:
> 
>   tg_weight = tg->load_avg - cfs_rq->contrib + cfs_rq->avg.load_avg
> 
> To better match with the numerator's units, otherwise it will have a
> tendency to push avg_shares down further than it needs to be.
> 
> 
> (All assuming it actually works of course.. compile tested only)

So, if changing gcfs_rq se->load_avg.avg to match the gcfs_rq's
runnable_load_avg is icky, and I can see why that would be, we can
simply introduce a separate channel of propagation so that
runnable_load_avg gets propagated independently from se->load_avg
propagation, so that for all every cfs_rq, its runnable_load_avg is
the sum of all active load_avgs queued on itself and its descendents,
which is the number we want for load balancing anyway.  I'll try to
spin a patch which does that.

I still wonder what gcfs_rq se->load_avg.avg is good for tho?  It's
nice to keep the value in line but is it actually used anywhere?  The
parent cfs_rq's values are independently calculated and, AFAICS, the
only time the value is used is to propagate into the parent's
runnable_load_sum, which has to use a different value, as explained
above.

Thanks.

-- 
tejun