Messages in this thread | | | Date | Wed, 02 Oct 2013 11:55:43 +0200 | From | Daniel Lezcano <> | Subject | Re: [PATCH] tick: make sleep length calculation more accurate |
| |
On 10/01/2013 11:31 PM, Stephen Boyd wrote: > 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 <daniel.lezcano@linaro.org> >> --- >> 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.
Yes, or after.
It sounds like this issue also occurs with the current code, no ?
> 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); > }
Yes, I agree.
Thanks for the review.
-- Daniel
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |