From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754047AbcAOQ4o (ORCPT ); Fri, 15 Jan 2016 11:56:44 -0500 Received: from foss.arm.com ([217.140.101.70]:54069 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753963AbcAOQ4l (ORCPT ); Fri, 15 Jan 2016 11:56:41 -0500 Subject: Re: [RFC PATCH 0/4] sched: Improve cpu load accounting with nohz To: Byungchul Park , perterz@infradead.org References: <1452700891-21807-1-git-send-email-fweisbec@gmail.com> <569810C4.7090900@arm.com> <20160114212704.GJ6357@twins.programming.kicks-ass.net> <56981FF2.6030700@arm.com> <20160115070749.GA1914@X58A-UD3R> Cc: Peter Zijlstra , Frederic Weisbecker , LKML , Chris Metcalf , Thomas Gleixner , Luiz Capitulino , Christoph Lameter , "Paul E . McKenney" , Mike Galbraith , Rik van Riel From: Dietmar Eggemann Message-ID: <569924C4.8010903@arm.com> Date: Fri, 15 Jan 2016 16:56:36 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20160115070749.GA1914@X58A-UD3R> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Byungchul, On 15/01/16 07:07, Byungchul Park wrote: > On Thu, Jan 14, 2016 at 10:23:46PM +0000, Dietmar Eggemann wrote: >> On 01/14/2016 09:27 PM, Peter Zijlstra wrote: >>> On Thu, Jan 14, 2016 at 09:19:00PM +0000, Dietmar Eggemann wrote: >>>> @@ -4346,7 +4346,10 @@ static void __update_cpu_load(struct rq *this_rq, unsigned long this_load, [...] > > I re-checked the equation I expanded and fortunately found it had no > problem. I think there are several ways to do it correctly. That is, > > option 1. decay the absolute value with decay_load_missed() and adjust > the sign. > > option 2. make decay_load_missed() can handle negative value. > > option 3. refer to the patch below. I think this option is the best. > > -----8<----- > > From ba3d3355fcce51c901376d268206f58a7d0e4214 Mon Sep 17 00:00:00 2001 > From: Byungchul Park > Date: Fri, 15 Jan 2016 15:58:09 +0900 > Subject: [PATCH] sched/fair: prevent using decay_load_missed() with a negative > value > > decay_load_missed() cannot handle nagative value. So we need to prevent > using the function with a negative value. > > Signed-off-by: Byungchul Park > --- > kernel/sched/fair.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 8dde8b6..3f08d75 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -4443,8 +4443,14 @@ static void __update_cpu_load(struct rq *this_rq, unsigned long this_load, > > /* scale is effectively 1 << i now, and >> i divides by scale */ > > - old_load = this_rq->cpu_load[i] - tickless_load; > + old_load = this_rq->cpu_load[i]; > old_load = decay_load_missed(old_load, pending_updates - 1, i); > + old_load -= decay_load_missed(tickless_load, pending_updates - 1, i); > + /* > + * old_load can never be a negative value because a decayed > + * tickless_load cannot be greater than the original > + * tickless_load. > + */ > old_load += tickless_load; > new_load = this_load; > /* > So in this case you want to decay old_load and add (tickless_load - decay(tickless_load)) on top. IMHO, this makes sense. (w/ your patch and cpu w/ NO_HZ_FULL) update_cpu_load_nohz: cpu=3 jiffies=4294935491 this_rq->last_load_update_tick=4294935489 pending_updates=2 active=1 load=0x114 __update_cpu_load: cpu=3 this_load=0x114 pending_updates=2 tickless_load=0xc2 __update_cpu_load: cpu=3 this_rq->cpu_load[0 .. 4] = [0xc2 0x62 0x32 0x1d 0x14] __update_cpu_load: cpu=3 1. old_load=0x62 __update_cpu_load: cpu=3 2.1 old_load=0x31 __update_cpu_load: cpu=3 2.2 old_load=0xffffffffffffffd0 <-- after decaying tickless_load __update_cpu_load: cpu=3 3. old_load=0x92 <-- after adding tickless_load __update_cpu_load: cpu=3 1. new_load=0x92 __update_cpu_load: cpu=3 2. new_load=0x92 __update_cpu_load: cpu=3 this_rq->cpu_load[1]=0xd3 ... update_cpu_load_active: cpu=3 this_rq->last_load_update_tick=4294935491 pending_updates=1 active=1 load=0x13e __update_cpu_load: cpu=3 this_load=0x13e pending_updates=1 tickless_load=0x114 __update_cpu_load: cpu=3 this_rq->cpu_load[0 .. 4] = [0x114 0xd3 0x86 0x4f 0x2f] ... Another point ... 'active=1' (function header: @active: !0 for NOHZ_FULL is a little bit misleading) is also true for when __update_cpu_load() is called from update_cpu_load_active(). In this case tickless_load wouldn't have to be set at all since pending_updates is 1, decay_load_missed() can handle that by bailing in case missed_updates = 0. Couldn't we set tickless_load only in case: unsigned long tickless_load = (active && pending_updates > 1) ? this_rq->cpu_load[0] : 0; Even though update_cpu_load_nohz() can call with pending_updates=1 and active=1 but then we don't have to decay.