[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[James Hogan]

Hi Peter,

On 11 April 2014 14:42, Peter Zijlstra <peterz@infradead.org> wrote:
> Because mwait_idle_with_hints() gets called from !idle context it must
> call current_clr_polling(). This however means that resched_task() is
> very likely to send an IPI even when we were polling:
>
>   CPU0                                  CPU1
>
>   if (current_set_polling_and_test())
>     goto out;
>
>   __monitor(&ti->flags);
>   if (!need_resched())
>     __mwait(eax, ecx);
>                                         set_tsk_need_resched(p);
>                                         smp_mb();
> out:
>   current_clr_polling();
>                                         if (!tsk_is_polling(p))
>                                           smp_send_reschedule(cpu);
>
>
> So while it is correct (extra IPIs aren't a problem, whereas a missed
> IPI would be) it is a performance problem (for some).
>
> Avoid this issue by using fetch_or() to atomically set NEED_RESCHED
> and test if POLLING_NRFLAG is set.
>
> Since a CPU stuck in mwait is unlikely to modify the flags word,
> contention on the cmpxchg is unlikely and thus we should mostly
> succeed in a single go.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
>  kernel/sched/core.c |   41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -505,6 +505,39 @@ static inline void init_hrtick(void)
>  #endif /* CONFIG_SCHED_HRTICK */
>
>  /*
> + * cmpxchg based fetch_or, macro so it works for different integer types
> + */
> +#define fetch_or(ptr, val)                                             \
> +({     typeof(*(ptr)) __old, __val = *(ptr);                           \
> +       for (;;) {                                                      \
> +               __old = cmpxchg((ptr), __val, __val | (val));           \
> +               if (__old == __val)                                     \
> +                       break;                                          \
> +               __val = __old;                                          \
> +       }                                                               \
> +       __old;                                                          \
> +})
> +
> +#ifdef TIF_POLLING_NRFLAG
> +/*
> + * Atomically set TIF_NEED_RESCHED and test for TIF_POLLING_NRFLAG,
> + * this avoids any races wrt polling state changes and thereby avoids
> + * spurious IPIs.
> + */
> +static bool set_nr_and_not_polling(struct task_struct *p)
> +{
> +       struct thread_info *ti = task_thread_info(p);
> +       return !(fetch_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG);

This breaks the build on metag, and I suspect arm64 too:

kernel/sched/core.c In function ?set_nr_and_not_polling?:
kernel/sched/core.c +531 : error: ?_TIF_POLLING_NRFLAG? undeclared

since metag/arm64 define TIF_POLLING_NRFLAG but not
_TIF_POLLING_NRFLAG. Could you please fix that prior to your patch to
avoid breaking bisection?

BTW what is it that determines whether an arch needs TIF_POLLING_NRFLAG?

Thanks
James


> +}
> +#else
> +static bool set_nr_and_not_polling(struct task_struct *p)
> +{
> +       set_tsk_need_resched(p);
> +       return true;
> +}
> +#endif
> +
> +/*
>   * resched_task - mark a task 'to be rescheduled now'.
>   *
>   * On UP this means the setting of the need_resched flag, on SMP it
> @@ -520,17 +553,15 @@ void resched_task(struct task_struct *p)
>         if (test_tsk_need_resched(p))
>                 return;
>
> -       set_tsk_need_resched(p);
> -
>         cpu = task_cpu(p);
> +
>         if (cpu == smp_processor_id()) {
> +               set_tsk_need_resched(p);
>                 set_preempt_need_resched();
>                 return;
>         }
>
> -       /* NEED_RESCHED must be visible before we test polling */
> -       smp_mb();
> -       if (!tsk_is_polling(p))
> +       if (set_nr_and_not_polling(p))
>                 smp_send_reschedule(cpu);
>  }
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Peter Zijlstra]

On Fri, May 09, 2014 at 02:37:27PM +0100, James Hogan wrote:
> Hi Peter,
> 
> On 11 April 2014 14:42, Peter Zijlstra <peterz@infradead.org> wrote:
> > +       return !(fetch_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG);
> 
> This breaks the build on metag, and I suspect arm64 too:

Yep, I just got a patch for arm64.

> kernel/sched/core.c In function ?set_nr_and_not_polling?:
> kernel/sched/core.c +531 : error: ?_TIF_POLLING_NRFLAG? undeclared
> 
> since metag/arm64 define TIF_POLLING_NRFLAG but not
> _TIF_POLLING_NRFLAG. Could you please fix that prior to your patch to
> avoid breaking bisection?

Ingo, is there any git magic to make that happen? Can we have a tree
with 2 patches (one for ARM64 and one for metag) before the sched/core
tree?

> BTW what is it that determines whether an arch needs TIF_POLLING_NRFLAG?

Any SMP arch that has a polling idle function of any kind (including the
default cpu_idle_poll()).

That said, even if that's true, not having TIF_POLLING_NRFLAG isn't
fatal, just sub-optimal in that we'll send an unconditional IPI to wake
the CPU even though its polling TIF_NEED_RESCHED and doesn't need
anything other than that write to wake up.

Most archs have (x86) hlt or (arm) wfi like idle instructions, and if
that is your only possible idle function, you'll require the interrupt
to wake up and there's really no point to having the POLLING bit.

Lastly, having the POLLING bit and not needing it is similarly non-fatal.

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Catalin Marinas]

Hi Peter,

On Fri, May 09, 2014 at 03:15:20PM +0100, Peter Zijlstra wrote:
> On Fri, May 09, 2014 at 02:37:27PM +0100, James Hogan wrote:
> > On 11 April 2014 14:42, Peter Zijlstra <peterz@infradead.org> wrote:
> > > +       return !(fetch_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG);
> > 
> > This breaks the build on metag, and I suspect arm64 too:
> 
> Yep, I just got a patch for arm64.

[...]

> Any SMP arch that has a polling idle function of any kind (including the
> default cpu_idle_poll()).
> 
> That said, even if that's true, not having TIF_POLLING_NRFLAG isn't
> fatal, just sub-optimal in that we'll send an unconditional IPI to wake
> the CPU even though its polling TIF_NEED_RESCHED and doesn't need
> anything other than that write to wake up.
> 
> Most archs have (x86) hlt or (arm) wfi like idle instructions, and if
> that is your only possible idle function, you'll require the interrupt
> to wake up and there's really no point to having the POLLING bit.

I wonder why we still need TIF_POLLING_NRFLAG for arm64. It was on arm
until commit 16a8016372c42c7628eb (sanitize tsk_is_polling()). On arm64
we use wfi for idle or a firmware call but in both cases the assumption
is that we need an interrupt for waking up.

So I think we should remove this macro for arm64.

-- 
Catalin

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Peter Zijlstra]

On Fri, May 09, 2014 at 03:40:34PM +0100, Catalin Marinas wrote:

> I wonder why we still need TIF_POLLING_NRFLAG for arm64. It was on arm
> until commit 16a8016372c42c7628eb (sanitize tsk_is_polling()). On arm64
> we use wfi for idle or a firmware call but in both cases the assumption
> is that we need an interrupt for waking up.
> 
> So I think we should remove this macro for arm64.

Does ARM64 support idle=poll? If so, you could keep it for that,
otherwise it does indeed appear to be pointless.

As to 32bit ARM, are there SMP chips which do not have WFI?

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Catalin Marinas]

On Fri, May 09, 2014 at 03:50:02PM +0100, Peter Zijlstra wrote:
> On Fri, May 09, 2014 at 03:40:34PM +0100, Catalin Marinas wrote:
> 
> > I wonder why we still need TIF_POLLING_NRFLAG for arm64. It was on arm
> > until commit 16a8016372c42c7628eb (sanitize tsk_is_polling()). On arm64
> > we use wfi for idle or a firmware call but in both cases the assumption
> > is that we need an interrupt for waking up.
> > 
> > So I think we should remove this macro for arm64.
> 
> Does ARM64 support idle=poll? If so, you could keep it for that,
> otherwise it does indeed appear to be pointless.

We don't support idle=poll either.

> As to 32bit ARM, are there SMP chips which do not have WFI?

No. WFI is even used for the secondary booting protocol (we need to send
an IPI to get them going).

-- 
Catalin

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Peter Zijlstra]

On Fri, May 09, 2014 at 03:57:45PM +0100, Catalin Marinas wrote:
> On Fri, May 09, 2014 at 03:50:02PM +0100, Peter Zijlstra wrote:
> > On Fri, May 09, 2014 at 03:40:34PM +0100, Catalin Marinas wrote:
> > 
> > > I wonder why we still need TIF_POLLING_NRFLAG for arm64. It was on arm
> > > until commit 16a8016372c42c7628eb (sanitize tsk_is_polling()). On arm64
> > > we use wfi for idle or a firmware call but in both cases the assumption
> > > is that we need an interrupt for waking up.
> > > 
> > > So I think we should remove this macro for arm64.
> > 
> > Does ARM64 support idle=poll? If so, you could keep it for that,
> > otherwise it does indeed appear to be pointless.
> 
> We don't support idle=poll either.
> 
> > As to 32bit ARM, are there SMP chips which do not have WFI?
> 
> No. WFI is even used for the secondary booting protocol (we need to send
> an IPI to get them going).

OK, so I'll queue a patch removing TIF_POLLING_NRFLAG for arm64.

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140509/410edcd6/attachment.sig>

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Peter Zijlstra]

On Fri, May 09, 2014 at 07:02:34PM +0200, Peter Zijlstra wrote:
> OK, so I'll queue a patch removing TIF_POLLING_NRFLAG for arm64.


---
Subject: arm64: Remove TIF_POLLING_NRFLAG
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri May  9 19:04:00 CEST 2014

The only idle method for arm64 is WFI and it therefore
unconditionally requires the reschedule interrupt when idle.

Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/arm64/include/asm/thread_info.h |    2 --
 1 file changed, 2 deletions(-)

--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -95,13 +95,11 @@ static inline struct thread_info *curren
  *  TIF_NEED_RESCHED	- rescheduling necessary
  *  TIF_NOTIFY_RESUME	- callback before returning to user
  *  TIF_USEDFPU		- FPU was used by this task this quantum (SMP)
- *  TIF_POLLING_NRFLAG	- true if poll_idle() is polling TIF_NEED_RESCHED
  */
 #define TIF_SIGPENDING		0
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_SYSCALL_TRACE	8
-#define TIF_POLLING_NRFLAG	16
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_FREEZE		19
 #define TIF_RESTORE_SIGMASK	20
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140509/63698086/attachment.sig>

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Catalin Marinas]

On Fri, May 09, 2014 at 06:06:49PM +0100, Peter Zijlstra wrote:
> Subject: arm64: Remove TIF_POLLING_NRFLAG
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri May  9 19:04:00 CEST 2014
> 
> The only idle method for arm64 is WFI and it therefore
> unconditionally requires the reschedule interrupt when idle.
> 
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>

There's a tag with my name already but just in case you need another:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks.

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[Peter Zijlstra]

On Fri, May 09, 2014 at 06:09:46PM +0100, Catalin Marinas wrote:
> On Fri, May 09, 2014 at 06:06:49PM +0100, Peter Zijlstra wrote:
> > Subject: arm64: Remove TIF_POLLING_NRFLAG
> > From: Peter Zijlstra <peterz@infradead.org>
> > Date: Fri May  9 19:04:00 CEST 2014
> > 
> > The only idle method for arm64 is WFI and it therefore
> > unconditionally requires the reschedule interrupt when idle.
> > 
> > Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> 
> There's a tag with my name already but just in case you need another:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

The more the merrier :-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140509/94428b58/attachment.sig>

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[James Hogan]

On 09/05/14 15:15, Peter Zijlstra wrote:
> On Fri, May 09, 2014 at 02:37:27PM +0100, James Hogan wrote:
>> Hi Peter,
>>
>> On 11 April 2014 14:42, Peter Zijlstra <peterz@infradead.org> wrote:
>>> +       return !(fetch_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG);
>>
>> This breaks the build on metag, and I suspect arm64 too:
> 
> Yep, I just got a patch for arm64.
> 
>> kernel/sched/core.c In function ?set_nr_and_not_polling?:
>> kernel/sched/core.c +531 : error: ?_TIF_POLLING_NRFLAG? undeclared
>>
>> since metag/arm64 define TIF_POLLING_NRFLAG but not
>> _TIF_POLLING_NRFLAG. Could you please fix that prior to your patch to
>> avoid breaking bisection?
> 
> Ingo, is there any git magic to make that happen? Can we have a tree
> with 2 patches (one for ARM64 and one for metag) before the sched/core
> tree?
> 
>> BTW what is it that determines whether an arch needs TIF_POLLING_NRFLAG?
> 
> Any SMP arch that has a polling idle function of any kind (including the
> default cpu_idle_poll()).
> 
> That said, even if that's true, not having TIF_POLLING_NRFLAG isn't
> fatal, just sub-optimal in that we'll send an unconditional IPI to wake
> the CPU even though its polling TIF_NEED_RESCHED and doesn't need
> anything other than that write to wake up.
> 
> Most archs have (x86) hlt or (arm) wfi like idle instructions, and if
> that is your only possible idle function, you'll require the interrupt
> to wake up and there's really no point to having the POLLING bit.
> 
> Lastly, having the POLLING bit and not needing it is similarly non-fatal.
> 

Thanks. I think the flag can go for Metag then. I suggest the following
patch.

Cheers
James

>From 15dd3f9cc18bdb1c8c4d2c395778421022250aa8 Mon Sep 17 00:00:00 2001
From: James Hogan <james.hogan@imgtec.com>
Date: Fri, 9 May 2014 15:36:21 +0100
Subject: [PATCH] metag: Remove TIF_POLLING_NRFLAG

The Meta idle function jumps into the interrupt handler which
efficiently blocks waiting for the next interrupt when it reads the
interrupt status register (TXSTATI). No other (polling) idle functions
can be used, therefore TIF_POLLING_NRFLAG is unnecessary, so lets remove
it.

Peter Zijlstra said:
> Most archs have (x86) hlt or (arm) wfi like idle instructions, and if
> that is your only possible idle function, you'll require the interrupt
> to wake up and there's really no point to having the POLLING bit.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 arch/metag/include/asm/thread_info.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/metag/include/asm/thread_info.h b/arch/metag/include/asm/thread_info.h
index b19e9c588a16..47711336119e 100644
--- a/arch/metag/include/asm/thread_info.h
+++ b/arch/metag/include/asm/thread_info.h
@@ -117,10 +117,8 @@ static inline int kstack_end(void *addr)
 #define TIF_SECCOMP		5	/* secure computing */
 #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_NOTIFY_RESUME	7	/* callback before returning to user */
-#define TIF_POLLING_NRFLAG      8	/* true if poll_idle() is polling
-					   TIF_NEED_RESCHED */
-#define TIF_MEMDIE		9	/* is terminating due to OOM killer */
-#define TIF_SYSCALL_TRACEPOINT  10	/* syscall tracepoint instrumentation */
+#define TIF_MEMDIE		8	/* is terminating due to OOM killer */
+#define TIF_SYSCALL_TRACEPOINT	9	/* syscall tracepoint instrumentation */
 
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
-- 
1.9.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140509/601c998f/attachment.sig>

[RFC][PATCH 6/8] sched,idle: Avoid spurious wakeup IPIs

[James Hogan]

Hi Peter,

On 09/05/14 15:51, James Hogan wrote:
> On 09/05/14 15:15, Peter Zijlstra wrote:
>> Most archs have (x86) hlt or (arm) wfi like idle instructions, and if
>> that is your only possible idle function, you'll require the interrupt
>> to wake up and there's really no point to having the POLLING bit.
>>
>> Lastly, having the POLLING bit and not needing it is similarly non-fatal.
>>
> 
> Thanks. I think the flag can go for Metag then. I suggest the following
> patch.

This is still bust in linux-next. What's the status of this patch?

Thanks
James

> 
> From 15dd3f9cc18bdb1c8c4d2c395778421022250aa8 Mon Sep 17 00:00:00 2001
> From: James Hogan <james.hogan@imgtec.com>
> Date: Fri, 9 May 2014 15:36:21 +0100
> Subject: [PATCH] metag: Remove TIF_POLLING_NRFLAG
> 
> The Meta idle function jumps into the interrupt handler which
> efficiently blocks waiting for the next interrupt when it reads the
> interrupt status register (TXSTATI). No other (polling) idle functions
> can be used, therefore TIF_POLLING_NRFLAG is unnecessary, so lets remove
> it.
> 
> Peter Zijlstra said:
>> Most archs have (x86) hlt or (arm) wfi like idle instructions, and if
>> that is your only possible idle function, you'll require the interrupt
>> to wake up and there's really no point to having the POLLING bit.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  arch/metag/include/asm/thread_info.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/metag/include/asm/thread_info.h b/arch/metag/include/asm/thread_info.h
> index b19e9c588a16..47711336119e 100644
> --- a/arch/metag/include/asm/thread_info.h
> +++ b/arch/metag/include/asm/thread_info.h
> @@ -117,10 +117,8 @@ static inline int kstack_end(void *addr)
>  #define TIF_SECCOMP		5	/* secure computing */
>  #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
>  #define TIF_NOTIFY_RESUME	7	/* callback before returning to user */
> -#define TIF_POLLING_NRFLAG      8	/* true if poll_idle() is polling
> -					   TIF_NEED_RESCHED */
> -#define TIF_MEMDIE		9	/* is terminating due to OOM killer */
> -#define TIF_SYSCALL_TRACEPOINT  10	/* syscall tracepoint instrumentation */
> +#define TIF_MEMDIE		8	/* is terminating due to OOM killer */
> +#define TIF_SYSCALL_TRACEPOINT	9	/* syscall tracepoint instrumentation */
>  
>  
>  #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
>