[Query]: tick-sched: can idle_active be false in tick_nohz_idle_exit()?

[Query]: tick-sched: can idle_active be false in tick_nohz_idle_exit()?

[Viresh Kumar]

Hi Guys,

File: kernel/time/tick-sched.c
function: tick_nohz_idle_exit()

We are checking here if idle_active is true or not and then
do some stuff. But is it possible that idle_active be false
here?

The sequence as far as I understood is:
idle-loop:

tick_nohz_idle_enter(), i.e. idle_active = true;
local_irq_disable()

IDLE
.
.

wake up due to IPI ??

local_irq_enable()
    tick_nohz_irq_enter(), i.e. idle_active = false;
    tick_nohz_irq_exit(), i.e. idle_active = true;

tick_nohz_idle_exit()

How can idle_active be false here?

--
viresh

Re: [Query]: tick-sched: can idle_active be false in tick_nohz_idle_exit()?

[Frederic Weisbecker]

On Wed, Apr 09, 2014 at 05:28:57PM +0530, Viresh Kumar wrote:
> Hi Guys,
> 
> File: kernel/time/tick-sched.c
> function: tick_nohz_idle_exit()
> 
> We are checking here if idle_active is true or not and then
> do some stuff. But is it possible that idle_active be false
> here?
> 
> The sequence as far as I understood is:
> idle-loop:
> 
> tick_nohz_idle_enter(), i.e. idle_active = true;
> local_irq_disable()
> 
> IDLE
> .
> .
> 
> wake up due to IPI ??
> 
> local_irq_enable()
>     tick_nohz_irq_enter(), i.e. idle_active = false;
>     tick_nohz_irq_exit(), i.e. idle_active = true;
> 
> tick_nohz_idle_exit()
> 
> How can idle_active be false here?

When a dynticks idle CPU is woken up (typically with an IPI), tick_nohz_stop_idle()
is called on interrupt entry but, because this is a waking up IPI, tick_nohz_start_idle()
won't be called. The reason is that need_resched() prevents tick_nohz_irq_exit() to be
called in irq_exit().

After all if we know that the CPU is going to exit the idle task, we don't need to account
any more idle time. We also don't need to retry to enter in dynticks idle mode since we
are going to restart the tick with tick_nohz_idle_exit().

So in case of wake up IPIs, we may end up with !ts->idle_active in tick_nohz_idle_exit() :)

I must confess this is not obvious. It confused me as well when I met that part. A small
comment in tick_nohz_idle_exit() would be welcome ;)

Thanks.

> 
> --
> viresh

Re: [Query]: tick-sched: can idle_active be false in tick_nohz_idle_exit()?

[Viresh Kumar]

On 10 April 2014 20:26, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> When a dynticks idle CPU is woken up (typically with an IPI), tick_nohz_stop_idle()
> is called on interrupt entry but, because this is a waking up IPI, tick_nohz_start_idle()
> won't be called. The reason is that need_resched() prevents tick_nohz_irq_exit() to be
> called in irq_exit().
>
> After all if we know that the CPU is going to exit the idle task, we don't need to account
> any more idle time. We also don't need to retry to enter in dynticks idle mode since we
> are going to restart the tick with tick_nohz_idle_exit().
>
> So in case of wake up IPIs, we may end up with !ts->idle_active in tick_nohz_idle_exit() :)
>
> I must confess this is not obvious.

I agree.. I didn't had a clue that this can happen. Good that I asked
this question :)

> It confused me as well when I met that part. A small
> comment in tick_nohz_idle_exit() would be welcome ;)

Looks hard. I tried for a small comment and this is the smallest I could get:

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index c2d868d..26cf5bb 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -925,6 +925,22 @@ void tick_nohz_idle_exit(void)

        ts->inidle = 0;

+       /*
+        * Can idle_active be false here?
+        * Ideally this would be the sequence of calls:
+        * - tick_nohz_idle_enter(), i.e. idle_active = true;
+        * - local_irq_disable()
+        * - IDLE
+        * - wake up due to IPI or other interrupt
+        * - local_irq_enable()
+        * - tick_nohz_irq_enter(), i.e. idle_active = false;
+        * - tick_nohz_irq_exit(), i.e. idle_active = true; This is not called
+        *   in case of IPI's as need_resched() will prevent that in
+        *   tick_irq_exit(), as we don't need to account any more for idle time
+        *   or try to enter dyntics mode (We are going to exit idle state).
+        *
+        * - tick_nohz_idle_exit()
+        */
        if (ts->idle_active || ts->tick_stopped)
                now = ktime_get();


I am preparing a cleanup patchset (separate from the timer/hrtimers 36 patch
set) for kernel/time/ and will add this change to that.. I am waiting
for the merge
window to close and Thomas to comment on my timers/hrtimers patches first.
Only then will I send another 40 :)

--
viresh

Re: [Query]: tick-sched: can idle_active be false in tick_nohz_idle_exit()?

[Frederic Weisbecker]

On Fri, Apr 11, 2014 at 03:24:11PM +0530, Viresh Kumar wrote:
> On 10 April 2014 20:26, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > When a dynticks idle CPU is woken up (typically with an IPI), tick_nohz_stop_idle()
> > is called on interrupt entry but, because this is a waking up IPI, tick_nohz_start_idle()
> > won't be called. The reason is that need_resched() prevents tick_nohz_irq_exit() to be
> > called in irq_exit().
> >
> > After all if we know that the CPU is going to exit the idle task, we don't need to account
> > any more idle time. We also don't need to retry to enter in dynticks idle mode since we
> > are going to restart the tick with tick_nohz_idle_exit().
> >
> > So in case of wake up IPIs, we may end up with !ts->idle_active in tick_nohz_idle_exit() :)
> >
> > I must confess this is not obvious.
> 
> I agree.. I didn't had a clue that this can happen. Good that I asked
> this question :)
> 
> > It confused me as well when I met that part. A small
> > comment in tick_nohz_idle_exit() would be welcome ;)
> 
> Looks hard. I tried for a small comment and this is the smallest I could get:
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index c2d868d..26cf5bb 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -925,6 +925,22 @@ void tick_nohz_idle_exit(void)
> 
>         ts->inidle = 0;
> 
> +       /*
> +        * Can idle_active be false here?
> +        * Ideally this would be the sequence of calls:
> +        * - tick_nohz_idle_enter(), i.e. idle_active = true;
> +        * - local_irq_disable()
> +        * - IDLE
> +        * - wake up due to IPI or other interrupt
> +        * - local_irq_enable()
> +        * - tick_nohz_irq_enter(), i.e. idle_active = false;
> +        * - tick_nohz_irq_exit(), i.e. idle_active = true; This is not called
> +        *   in case of IPI's as need_resched() will prevent that in
> +        *   tick_irq_exit(), as we don't need to account any more for idle time
> +        *   or try to enter dyntics mode (We are going to exit idle state).
> +        *
> +        * - tick_nohz_idle_exit()
> +        */
>         if (ts->idle_active || ts->tick_stopped)
>                 now = ktime_get();

I'm sure we can summarize a lot of uninteresting details there. Many of what
you describe can be guessed after a simple review on the code. Let rather
focus on the trickies.

How about something like:

          /*
            * ts->active can be 0 if a wake up IPI pulled us out of idle mode. When
            * that happens we know we're exiting the idle task. Pending idle sleep
            * time is flushed on irq entry then no more is accounted afterward.
            * The need_resched() check on irq_exit() prevents from accounting more.
           */

> 
> 
> I am preparing a cleanup patchset (separate from the timer/hrtimers 36 patch
> set) for kernel/time/ and will add this change to that.. I am waiting
> for the merge
> window to close and Thomas to comment on my timers/hrtimers patches first.
> Only then will I send another 40 :)

Thanks! Note you can post before the merge window closes. Reviews are possible
anytime :)

> 
> --
> viresh