[PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[Lianwei Wang]

The rcu_idle_endter may call wakeup_softirqd to set the need resched
flag on idle process. But if we don't check it after that, then the
cpu will enter idle state with RESCHED flag set and can not be woken
up by wakeup/resched call anymore.

Check need_resched after rcu_idle_enter to make sure the cpu is able
to be out of idle immediatley to run other tasks.

Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 kernel/sched/idle.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 4a2ef5a02fd3..6e96a1f41041 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -137,21 +137,21 @@ static void cpuidle_idle_call(void)
        int next_state, entered_state;

        /*
+        * Tell the RCU framework we are entering an idle section,
+        * so no more rcu read side critical sections and one more
+        * step to the grace period
+        */
+       rcu_idle_enter();
+
+       /*
         * Check if the idle task must be rescheduled. If it is the
         * case, exit the function after re-enabling the local irq.
         */
        if (need_resched()) {
                local_irq_enable();
-               return;
+               goto exit_idle;
        }

-       /*
-        * Tell the RCU framework we are entering an idle section,
-        * so no more rcu read side critical sections and one more
-        * step to the grace period
-        */
-       rcu_idle_enter();
-
        if (cpuidle_not_available(drv, dev)) {
                default_idle_call();
                goto exit_idle;

Re: [PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[Peter Zijlstra]

On Sat, Nov 21, 2015 at 01:02:02AM -0800, Lianwei Wang wrote:
> The rcu_idle_endter may call wakeup_softirqd to set the need resched
> flag on idle process. But if we don't check it after that, then the
> cpu will enter idle state with RESCHED flag set and can not be woken
> up by wakeup/resched call anymore.

Fair enough; but which cpuidle driver did you observe that with? All the
ones I checked test for need_resched again after this.

Re: [PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[Lianwei Wang]

Issue was found on ARM/Qcom platform, but I think it is applicable to
other platforms as well.  I add a BUG_ON after rcu_enter_idle to catch
it. The Qcom's cpuidle driver is not in mainline kernel yet.

On Mon, Nov 23, 2015 at 5:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sat, Nov 21, 2015 at 01:02:02AM -0800, Lianwei Wang wrote:
>> The rcu_idle_endter may call wakeup_softirqd to set the need resched
>> flag on idle process. But if we don't check it after that, then the
>> cpu will enter idle state with RESCHED flag set and can not be woken
>> up by wakeup/resched call anymore.
>
> Fair enough; but which cpuidle driver did you observe that with? All the
> ones I checked test for need_resched again after this.

Re: [PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[Peter Zijlstra]

On Mon, Nov 23, 2015 at 09:42:06AM -0800, Lianwei Wang wrote:
> Issue was found on ARM/Qcom platform, but I think it is applicable to
> other platforms as well.  I add a BUG_ON after rcu_enter_idle to catch
> it. The Qcom's cpuidle driver is not in mainline kernel yet.


A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?


And I suspect that your qualcom thing doesn't have TIF_POLLING_NRFLAG
either, which would introduce another need_resched() test after the
rcu_enter_idle() call.

Re: [PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[Lianwei Wang]

On Mon, Nov 23, 2015 at 9:57 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Nov 23, 2015 at 09:42:06AM -0800, Lianwei Wang wrote:
>> Issue was found on ARM/Qcom platform, but I think it is applicable to
>> other platforms as well.  I add a BUG_ON after rcu_enter_idle to catch
>> it. The Qcom's cpuidle driver is not in mainline kernel yet.
>
>
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
>
> And I suspect that your qualcom thing doesn't have TIF_POLLING_NRFLAG
> either, which would introduce another need_resched() test after the
> rcu_enter_idle() call.

Sorry for the top posting and thank you for pointing it out.

I check the idle code again and realize that both the
default_idle_call and call_cpuidle will test TIF_NEED_RESCHED flag
again after rcu_enter_idle call by calling
current_clr_polling_and_test. So no issue on mainline kernel now.

Re: [PATCH] sched: idle: move need_resched check after function rcu_idle_enter

[Peter Zijlstra]

On Sat, Nov 21, 2015 at 01:02:02AM -0800, Lianwei Wang wrote:
> The rcu_idle_endter may call wakeup_softirqd to set the need resched
> flag on idle process. But if we don't check it after that, then the
> cpu will enter idle state with RESCHED flag set and can not be woken
> up by wakeup/resched call anymore.
> 
> Check need_resched after rcu_idle_enter to make sure the cpu is able
> to be out of idle immediatley to run other tasks.
> 
> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
> ---
>  kernel/sched/idle.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 4a2ef5a02fd3..6e96a1f41041 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -137,21 +137,21 @@ static void cpuidle_idle_call(void)
>         int next_state, entered_state;
> 
>         /*
> +        * Tell the RCU framework we are entering an idle section,
> +        * so no more rcu read side critical sections and one more
> +        * step to the grace period
> +        */
> +       rcu_idle_enter();
> +
> +       /*
>          * Check if the idle task must be rescheduled. If it is the
>          * case, exit the function after re-enabling the local irq.
>          */
>         if (need_resched()) {
>                 local_irq_enable();
> -               return;
> +               goto exit_idle;
>         }
> 
> -       /*
> -        * Tell the RCU framework we are entering an idle section,
> -        * so no more rcu read side critical sections and one more
> -        * step to the grace period
> -        */
> -       rcu_idle_enter();
> -
>         if (cpuidle_not_available(drv, dev)) {
>                 default_idle_call();
>                 goto exit_idle;

FWIW your patch is whitespace mangled; I could not apply if I wanted to.