[PATCH V2] tick: Make sleep length calculation more accurate

[PATCH V2] tick: Make sleep length calculation more accurate

[Daniel Lezcano]

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
may occur, making the sleep length shorter than actually it is because of the
interrupt processing, or different if the timer itself expired.

This patch fixes that by moving the sleep_length computation in the
tick_nohz_get_sleep_length function and using the tick device's next_event.

As the sleep_length field is no longer needed, it is removed from the
tick_sched structure.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 include/linux/tick.h     |    2 --
 kernel/time/tick-sched.c |    5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 5128d33..53dbbd7 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -48,7 +48,6 @@ enum tick_nohz_mode {
  * @idle_exittime:	Time when the idle state was left
  * @idle_sleeptime:	Sum of the time slept in idle with sched tick stopped
  * @iowait_sleeptime:	Sum of the time slept in idle with sched tick stopped, with IO outstanding
- * @sleep_length:	Duration of the current idle sleep
  * @do_timer_lst:	CPU was the last one doing do_timer before going idle
  */
 struct tick_sched {
@@ -67,7 +66,6 @@ struct tick_sched {
 	ktime_t				idle_exittime;
 	ktime_t				idle_sleeptime;
 	ktime_t				iowait_sleeptime;
-	ktime_t				sleep_length;
 	unsigned long			last_jiffies;
 	unsigned long			next_jiffies;
 	ktime_t				idle_expires;
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 3612fc7..60b1dcd 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -673,7 +673,6 @@ 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);
 
 	return ret;
 }
@@ -837,8 +836,10 @@ void tick_nohz_irq_exit(void)
 ktime_t tick_nohz_get_sleep_length(void)
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
+	struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev;
+	ktime_t now = ktime_get();
 
-	return ts->sleep_length;
+	return ktime_sub(dev->next_event, now);
 }
 
 static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
-- 
1.7.9.5

Re: [PATCH V2] tick: Make sleep length calculation more accurate

[Daniel Lezcano]

On 10/02/2013 12:26 PM, 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
> may occur, making the sleep length shorter than actually it is because of the
> interrupt processing, or different if the timer itself expired.
>
> This patch fixes that by moving the sleep_length computation in the
> tick_nohz_get_sleep_length function and using the tick device's next_event.
>
> As the sleep_length field is no longer needed, it is removed from the
> tick_sched structure.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Stephen,

I added your signed-off as the change in tick_nohz_get_sleep_length is 
the one you proposed previously.

   -- Daniel

> ---
>   include/linux/tick.h     |    2 --
>   kernel/time/tick-sched.c |    5 +++--
>   2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/tick.h b/include/linux/tick.h
> index 5128d33..53dbbd7 100644
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -48,7 +48,6 @@ enum tick_nohz_mode {
>    * @idle_exittime:	Time when the idle state was left
>    * @idle_sleeptime:	Sum of the time slept in idle with sched tick stopped
>    * @iowait_sleeptime:	Sum of the time slept in idle with sched tick stopped, with IO outstanding
> - * @sleep_length:	Duration of the current idle sleep
>    * @do_timer_lst:	CPU was the last one doing do_timer before going idle
>    */
>   struct tick_sched {
> @@ -67,7 +66,6 @@ struct tick_sched {
>   	ktime_t				idle_exittime;
>   	ktime_t				idle_sleeptime;
>   	ktime_t				iowait_sleeptime;
> -	ktime_t				sleep_length;
>   	unsigned long			last_jiffies;
>   	unsigned long			next_jiffies;
>   	ktime_t				idle_expires;
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 3612fc7..60b1dcd 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -673,7 +673,6 @@ 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);
>
>   	return ret;
>   }
> @@ -837,8 +836,10 @@ void tick_nohz_irq_exit(void)
>   ktime_t tick_nohz_get_sleep_length(void)
>   {
>   	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
> +	struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev;
> +	ktime_t now = ktime_get();
>
> -	return ts->sleep_length;
> +	return ktime_sub(dev->next_event, now);
>   }
>
>   static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
>


-- 
  <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

Re: [PATCH V2] tick: Make sleep length calculation more accurate

[Frederic Weisbecker]

2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>:
> 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
> may occur, making the sleep length shorter than actually it is because of the
> interrupt processing

So, do you mean that the ts->sleep_length would return a value that is too long
given that the CPU already spent some time to service the irqs since we computed
the sleep length in tick_nohz_idle_enter()?

But then tick_nohz_irq_exit() should take care of that as it calls
again tick_nohz_stop_sched_tick().
So I'm a bit confused.

> or different if the timer itself expired.

Same here, if the timer expired, it triggers an interrupt which can do
two things:

1) reprogram a new timer and this recompute sleep_length
2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even
be called. Or the timer interrupts hlt, but then menu_select() was
called before.

So I probably missed something here.

Thanks.

Re: [PATCH V2] tick: Make sleep length calculation more accurate

[Daniel Lezcano]

On 10/02/2013 05:57 PM, Frederic Weisbecker wrote:
> 2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> 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
>> may occur, making the sleep length shorter than actually it is because of the
>> interrupt processing
>
> So, do you mean that the ts->sleep_length would return a value that is too long
> given that the CPU already spent some time to service the irqs since we computed
> the sleep length in tick_nohz_idle_enter()?
>
> But then tick_nohz_irq_exit() should take care of that as it calls
> again tick_nohz_stop_sched_tick().
> So I'm a bit confused.
>
>> or different if the timer itself expired.
>
> Same here, if the timer expired, it triggers an interrupt which can do
> two things:
>
> 1) reprogram a new timer and this recompute sleep_length
> 2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even
> be called. Or the timer interrupts hlt, but then menu_select() was
> called before.
>
> So I probably missed something here.

No you did not :)

Indeed... At the first glance, this issue sounded so obvious I suspected 
there must be a trick somewhere but I did not think to look at the 
irq_exit, the code is very complex. Thanks for clarifying this.

For my personal information, is there any particular reason to set an 
intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead of 
doing what does this patch ?

Thanks
   -- 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

Re: [PATCH V2] tick: Make sleep length calculation more accurate

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 06:22:29PM +0200, Daniel Lezcano wrote:
> On 10/02/2013 05:57 PM, Frederic Weisbecker wrote:
> >2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>:
> >>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
> >>may occur, making the sleep length shorter than actually it is because of the
> >>interrupt processing
> >
> >So, do you mean that the ts->sleep_length would return a value that is too long
> >given that the CPU already spent some time to service the irqs since we computed
> >the sleep length in tick_nohz_idle_enter()?
> >
> >But then tick_nohz_irq_exit() should take care of that as it calls
> >again tick_nohz_stop_sched_tick().
> >So I'm a bit confused.
> >
> >>or different if the timer itself expired.
> >
> >Same here, if the timer expired, it triggers an interrupt which can do
> >two things:
> >
> >1) reprogram a new timer and this recompute sleep_length
> >2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even
> >be called. Or the timer interrupts hlt, but then menu_select() was
> >called before.
> >
> >So I probably missed something here.
> 
> No you did not :)
> 
> Indeed... At the first glance, this issue sounded so obvious I
> suspected there must be a trick somewhere but I did not think to
> look at the irq_exit, the code is very complex. Thanks for
> clarifying this.
> 
> For my personal information, is there any particular reason to set
> an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead
> of doing what does this patch ?

May be we could do it that way yeah. Is menu_select() called only there?
I don't know how much difference that would make.

> 
> Thanks
>   -- 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
> 

Re: [PATCH V2] tick: Make sleep length calculation more accurate

[Daniel Lezcano]

On 10/02/2013 06:42 PM, Frederic Weisbecker wrote:
> On Wed, Oct 02, 2013 at 06:22:29PM +0200, Daniel Lezcano wrote:
>> On 10/02/2013 05:57 PM, Frederic Weisbecker wrote:
>>> 2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>> 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
>>>> may occur, making the sleep length shorter than actually it is because of the
>>>> interrupt processing
>>>
>>> So, do you mean that the ts->sleep_length would return a value that is too long
>>> given that the CPU already spent some time to service the irqs since we computed
>>> the sleep length in tick_nohz_idle_enter()?
>>>
>>> But then tick_nohz_irq_exit() should take care of that as it calls
>>> again tick_nohz_stop_sched_tick().
>>> So I'm a bit confused.
>>>
>>>> or different if the timer itself expired.
>>>
>>> Same here, if the timer expired, it triggers an interrupt which can do
>>> two things:
>>>
>>> 1) reprogram a new timer and this recompute sleep_length
>>> 2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even
>>> be called. Or the timer interrupts hlt, but then menu_select() was
>>> called before.
>>>
>>> So I probably missed something here.
>>
>> No you did not :)
>>
>> Indeed... At the first glance, this issue sounded so obvious I
>> suspected there must be a trick somewhere but I did not think to
>> look at the irq_exit, the code is very complex. Thanks for
>> clarifying this.
>>
>> For my personal information, is there any particular reason to set
>> an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead
>> of doing what does this patch ?
>
> May be we could do it that way yeah. Is menu_select() called only there?
> I don't know how much difference that would make.

Yes, it is called just one time in all the code. The benefit would be 
just to cleanup a field in the struct tick_sched.


-- 
  <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

Re: [PATCH V2] tick: Make sleep length calculation more accurate

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 08:03:39PM +0200, Daniel Lezcano wrote:
> On 10/02/2013 06:42 PM, Frederic Weisbecker wrote:
> >On Wed, Oct 02, 2013 at 06:22:29PM +0200, Daniel Lezcano wrote:
> >>On 10/02/2013 05:57 PM, Frederic Weisbecker wrote:
> >>>2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>:
> >>>>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
> >>>>may occur, making the sleep length shorter than actually it is because of the
> >>>>interrupt processing
> >>>
> >>>So, do you mean that the ts->sleep_length would return a value that is too long
> >>>given that the CPU already spent some time to service the irqs since we computed
> >>>the sleep length in tick_nohz_idle_enter()?
> >>>
> >>>But then tick_nohz_irq_exit() should take care of that as it calls
> >>>again tick_nohz_stop_sched_tick().
> >>>So I'm a bit confused.
> >>>
> >>>>or different if the timer itself expired.
> >>>
> >>>Same here, if the timer expired, it triggers an interrupt which can do
> >>>two things:
> >>>
> >>>1) reprogram a new timer and this recompute sleep_length
> >>>2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even
> >>>be called. Or the timer interrupts hlt, but then menu_select() was
> >>>called before.
> >>>
> >>>So I probably missed something here.
> >>
> >>No you did not :)
> >>
> >>Indeed... At the first glance, this issue sounded so obvious I
> >>suspected there must be a trick somewhere but I did not think to
> >>look at the irq_exit, the code is very complex. Thanks for
> >>clarifying this.
> >>
> >>For my personal information, is there any particular reason to set
> >>an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead
> >>of doing what does this patch ?
> >
> >May be we could do it that way yeah. Is menu_select() called only there?
> >I don't know how much difference that would make.
> 
> Yes, it is called just one time in all the code. The benefit would
> be just to cleanup a field in the struct tick_sched.

Yeah, why not.

Thanks.

> 
> 
> -- 
>  <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
>