From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752039AbbJLRpp (ORCPT ); Mon, 12 Oct 2015 13:45:45 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:34726 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751630AbbJLRpn (ORCPT ); Mon, 12 Oct 2015 13:45:43 -0400 Date: Mon, 12 Oct 2015 19:45:35 +0200 From: Frederic Weisbecker To: Peter Zijlstra Cc: Byungchul Park , mingo@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de Subject: Re: [PATCH v3 2/2] sched: consider missed ticks when updating global cpu load Message-ID: <20151012174532.GB1113@lerouge> References: <1443771974-27077-1-git-send-email-byungchul.park@lge.com> <1443771974-27077-3-git-send-email-byungchul.park@lge.com> <20151002155906.GD3816@twins.programming.kicks-ass.net> <20151004065819.GS6455@byungchulpark-X58A-UD3R> <20151005081555.GF2903@worktop.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151005081555.GF2903@worktop.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 05, 2015 at 10:15:55AM +0200, Peter Zijlstra wrote: > On Sun, Oct 04, 2015 at 03:58:19PM +0900, Byungchul Park wrote: > > anyway, it's wrong for update_process_times() to assume 1 tick because > > tick_irq_exit() -> tick_nohz_irq_exit() -> tick_nohz_full_update_tick() > > -> tick_nohz_restart_sched_tick() can happen at full NOHZ as i already > > said. in this full NOHZ case for tick to restart from non-idle, > > NO_HZ_FULL is very much a work in progress, there's plenty wrong with > it. But yes, if it does this then its broken here too, I'm not sure if > Frederic is aware of this or not (I'm sure he's got a fairly big list of > broken for NO_HZ_FULL). Indeed and cpu load active is part of what needs to be fixed. I hope this patchset will help. > > > 1. update_process_times() -> account_process_tick() must be able to handle > > more than one tick, or tick_nohz_restart_sched_tick() should handle the > > case additionally. (i think the latter is better.) i will try to modify > > the code to handle it if you agree with me. > > Yes, and we need to audit all the other stuff called from > update_process_times(). > > run_local_timers() seems be ok. > rcu_check_clalbacks() also doesn't seem to care about ticks. > > I _think_ we fixed most of the scheduler_tick() > stuff (under the assumption that TSC is stable), but I'm not sure. Concerning the variable pending ticks, we are fine with update_process_times() except a few stuff in scheduler_tick(): * cpu load active * sched_avg_update() handles well missed ticks as it's based on rq clock and specific period for updates. But I'm worried about remote reads of rt_avg, if any. * calc_global_load_tick(), not sure about this one * trigger_load_balance() * the infamous task_tick() :-) But load avg appears to me as a pretty standalone issue. So are each of these small issues. > > and run_posix_cpu_timers() might also be ok. > > > 2. to handle full NOHZ, tick_nohz_restart_sched_tick() should call > > update_cpu_load_active() instead of update_cpu_load_nohz() with my 1/2 > > patch and 2/2 patch, or we should modify update_cpu_load_nohz() to know > > full NOHZ, which currently don't know full NOHZ. (you may agree with the > > latter.) in any case, 1/2 patch is necessary which current code is > > absolutely missing. > > > > peter, what do you think about my opinion? and about my 1/2 patch? > > I did not look too closely, but it might have the right shape for > dealing with !idle ticks. I'd have to look more closely at it. > > > i will modify 2/2 patch depending on your feedback. > > I think it will take more than a single patch to rework all of > update_process_times(). And we should also ask Thomas for his opinion, > but I think we want: > > - make update_process_times() take a nr_ticks argument > - fixup everything below it > > - fix tick_nohz_handler to not ignore the hrtimer_forward() > return value and pass it into > tick_sched_handle()/update_process_times(). > > (assuming this is the right oneshot tick part, tick-common > seems to be about periodic timers which aren't used much ?!) this_nohz_handler() is the low res nohz handler. tick_sched_handle() is the high res one (I should rename these). I think we should rather find out the pending updates from update_process_times() itself and pass it to scheduler_tick() which is the one interested in it. Thanks.