Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933735AbdC3NrJ (ORCPT ); Thu, 30 Mar 2017 09:47:09 -0400 Received: from merlin.infradead.org ([205.233.59.134]:59078 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933126AbdC3NrI (ORCPT ); Thu, 30 Mar 2017 09:47:08 -0400 Date: Thu, 30 Mar 2017 15:46:58 +0200 From: Peter Zijlstra To: Paul Turner Cc: Yuyang Du , Ingo Molnar , LKML , Benjamin Segall , Morten Rasmussen , Vincent Guittot , Dietmar Eggemann , Matt Fleming , umgwanakikbuti@gmail.com Subject: Re: [RESEND PATCH 2/2] sched/fair: Optimize __update_sched_avg() Message-ID: <20170330134658.tobwqu3i5kvv64ug@hirez.programming.kicks-ass.net> References: <1486935863-25251-1-git-send-email-yuyang.du@intel.com> <1486935863-25251-3-git-send-email-yuyang.du@intel.com> <20170328144625.GX3093@worktop> <20170329000442.GC2459@ydu19desktop> <20170329104126.lg6ismevfbqywpcj@hirez.programming.kicks-ass.net> <20170330121658.6mo7datma4ssw7st@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170330121658.6mo7datma4ssw7st@hirez.programming.kicks-ass.net> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2590 Lines: 82 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? --- 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 periods; - u32 contrib; scale_freq = arch_scale_freq_capacity(NULL, cpu); scale_cpu = arch_scale_cpu_capacity(NULL, cpu); @@ -2880,13 +2877,14 @@ accumulate_sum(u64 delta, int cpu, struct sched_avg *sa, decay_load(cfs_rq->runnable_load_sum, periods); } sa->util_sum = decay_load((u64)(sa->util_sum), periods); - } - /* - * Step 2 - */ - delta %= 1024; - contrib = __accumulate_sum(periods, sa->period_contrib, delta); + /* + * Step 2 + */ + delta %= 1024; + contrib = __accumulate_pelt_segments(periods, + 1024 - sa->period_contrib, delta); + } sa->period_contrib = delta; contrib = cap_scale(contrib, scale_freq);