From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752554Ab3JAVbF (ORCPT ); Tue, 1 Oct 2013 17:31:05 -0400 Received: from smtp.codeaurora.org ([198.145.11.231]:35136 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751418Ab3JAVbB (ORCPT ); Tue, 1 Oct 2013 17:31:01 -0400 Message-ID: <524B3F14.5040001@codeaurora.org> Date: Tue, 01 Oct 2013 14:31:00 -0700 From: Stephen Boyd User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 MIME-Version: 1.0 To: Daniel Lezcano CC: tglx@linutronix.de, linux-kernel@vger.kernel.org, patches@linaro.org, linaro-kernel@lists.linaro.org Subject: Re: [PATCH] tick: make sleep length calculation more accurate References: <1380279140-27728-1-git-send-email-daniel.lezcano@linaro.org> In-Reply-To: <1380279140-27728-1-git-send-email-daniel.lezcano@linaro.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/27/13 03:52, Daniel Lezcano wrote: > The sleep_length is computed in the tick_nohz_stop_sched_tick function but it > is used later in the code with in between the local irq enabled. > > cpu_idle_loop > tick_nohz_idle_enter [ exits with local irq enabled ] > __tick_nohz_idle_enter > tick_nohz_stop_sched_tick > ... > > arch_cpu_idle > menu_select [ uses here 'sleep_length' ] > ... > > Between the computation of the sleep length and its usage, some interrupts > can occur, making the sleep length shorter than actually it is. > > This patch fixes that by moving the sleep_length computation in the > tick_nohz_get_sleep_length function and store the next_event for the device > instead of the sleep_length. > > Signed-off-by: Daniel Lezcano > --- > include/linux/tick.h | 2 +- > kernel/time/tick-sched.c | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/linux/tick.h b/include/linux/tick.h > index 5128d33..4932004 100644 > --- a/include/linux/tick.h > +++ b/include/linux/tick.h > @@ -67,7 +67,7 @@ struct tick_sched { > ktime_t idle_exittime; > ktime_t idle_sleeptime; > ktime_t iowait_sleeptime; > - ktime_t sleep_length; > + ktime_t next_event; > unsigned long last_jiffies; > unsigned long next_jiffies; > ktime_t idle_expires; Documentation update? > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index 3612fc7..2007a7f 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -673,7 +673,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > out: > ts->next_jiffies = next_jiffies; > ts->last_jiffies = last_jiffies; > - ts->sleep_length = ktime_sub(dev->next_event, now); > + ts->next_event = dev->next_event; > > return ret; > } > @@ -837,8 +837,9 @@ void tick_nohz_irq_exit(void) > ktime_t tick_nohz_get_sleep_length(void) > { > struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); > + ktime_t now = ktime_get(); > > - return ts->sleep_length; > + return ktime_sub(ts->next_event, now); > } > > static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) What happens if the idling CPU's next_event is updated via that interrupt? Say if the interrupt handler schedules a timer to fire before the next timer on the CPU? It looks like we won't notice that. Perhaps it's better to do this instead? ktime_t tick_nohz_get_sleep_length(void) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); + ktime_t now = ktime_get(); + struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev; - return ts->sleep_length; + return ktime_sub(dev->next_event, now); } -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation