From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752034AbcDRIXO (ORCPT ); Mon, 18 Apr 2016 04:23:14 -0400 Received: from LGEAMRELO13.lge.com ([156.147.23.53]:43622 "EHLO lgeamrelo13.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751149AbcDRIXL (ORCPT ); Mon, 18 Apr 2016 04:23:11 -0400 X-Original-SENDERIP: 156.147.1.126 X-Original-MAILFROM: byungchul.park@lge.com X-Original-SENDERIP: 10.177.222.33 X-Original-MAILFROM: byungchul.park@lge.com Date: Mon, 18 Apr 2016 17:22:41 +0900 From: Byungchul Park To: Frederic Weisbecker Cc: Peter Zijlstra , LKML , Chris Metcalf , Thomas Gleixner , Luiz Capitulino , Christoph Lameter , "Paul E . McKenney" , Mike Galbraith , Rik van Riel , Ingo Molnar Subject: Re: [PATCH 2/3] sched: Correctly handle nohz ticks cpu load accounting Message-ID: <20160418082241.GG2279@X58A-UD3R> References: <1460555812-25375-1-git-send-email-fweisbec@gmail.com> <1460555812-25375-3-git-send-email-fweisbec@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1460555812-25375-3-git-send-email-fweisbec@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 13, 2016 at 03:56:51PM +0200, Frederic Weisbecker wrote: > @@ -4524,12 +4523,12 @@ decay_load_missed(unsigned long load, unsigned long missed_updates, int idx) > * load[i]_n = (1 - 1/2^i)^n * load[i]_0 > * > * see decay_load_misses(). For NOHZ_FULL we get to subtract and add the extra > - * term. See the @active paramter. > + * term. > */ > -static void __cpu_load_update(struct rq *this_rq, unsigned long this_load, > - unsigned long pending_updates, int active) > +static void cpu_load_update(struct rq *this_rq, unsigned long this_load, > + unsigned long pending_updates) > { > - unsigned long tickless_load = active ? this_rq->cpu_load[0] : 0; > + unsigned long tickless_load = this_rq->cpu_load[0]; Hello, Good for my humble code to be fixed so we can write it like this here. > @@ -4618,26 +4617,56 @@ static void cpu_load_update_idle(struct rq *this_rq) > if (weighted_cpuload(cpu_of(this_rq))) > return; > > - __cpu_load_update_nohz(this_rq, READ_ONCE(jiffies), 0, 0); > + cpu_load_update_nohz(this_rq, READ_ONCE(jiffies), 0); > } > > /* > - * Called from tick_nohz_idle_exit() -- try and fix up the ticks we missed. > + * Record CPU load on nohz entry so we know the tickless load to account > + * on nohz exit. cpu_load[0] happens then to be updated more frequently > + * than other cpu_load[idx] but it should be fine as cpu_load readers > + * shouldn't rely into synchronized cpu_load[*] updates. > */ > -void cpu_load_update_nohz(int active) > +void cpu_load_update_nohz_start(void) > { > struct rq *this_rq = this_rq(); > + > + /* > + * This is all lockless but should be fine. If weighted_cpuload changes > + * concurrently we'll exit nohz. And cpu_load write can race with > + * cpu_load_update_idle() but both updater would be writing the same. > + */ > + this_rq->cpu_load[0] = weighted_cpuload(cpu_of(this_rq)); Like it. > +/* > + * Account the tickless load in the end of a nohz frame. > + */ > +void cpu_load_update_nohz_stop(void) > +{ > unsigned long curr_jiffies = READ_ONCE(jiffies); > - unsigned long load = active ? weighted_cpuload(cpu_of(this_rq)) : 0; > + struct rq *this_rq = this_rq(); > + unsigned long load; > > if (curr_jiffies == this_rq->last_load_update_tick) > return; > > + load = weighted_cpuload(cpu_of(this_rq)); Like it. > @@ -4645,11 +4674,11 @@ void cpu_load_update_nohz(int active) > void cpu_load_update_active(struct rq *this_rq) > { > unsigned long load = weighted_cpuload(cpu_of(this_rq)); > - /* > - * See the mess around cpu_load_update_idle() / cpu_load_update_nohz(). > - */ > - this_rq->last_load_update_tick = jiffies; > - __cpu_load_update(this_rq, load, 1, 1); > + > + if (tick_nohz_tick_stopped()) > + cpu_load_update_nohz(this_rq, READ_ONCE(jiffies), load); > + else > + cpu_load_update_periodic(this_rq, load); Oh! We have missed it until now. Terrible.. :-( Thank you, Byungchul