A while back (before [1]), i386 had this in the tail of its irq
handling code:
need_resched:
movl TI_flags(%ebp), %ecx # need_resched set ?
testb $_TIF_NEED_RESCHED, %cl
jz restore_all
testl $IF_MASK,EFLAGS(%esp) # interrupts off (exception path) ?
jz restore_all
sti
call preempt_schedule
cli
movl $0,TI_preempt_count(%ebp)
jmp need_resched
preempt_schedule() already had an inner need_resched() loop introduced by
commit 4d0b85ea4610 ("[PATCH] kernel preemption bits (2/2)")
and the outer loop was needed since, as the above commit explains, it
was possible to return from preempt_schedule(), get an interrupt and
have need_resched() true.
This was eventually changed when preempt_schedule_irq() was introduced
by [1]:
commit b268264c6299 ("[PATCH] sched: fix preemption race (Core/i386)")
which moved the enabling & disabling of interrupts inside the then
new preempt_schedule_irq(). From then on, the arch-code loop was no
longer necessary, providing preempt_schedule_irq() was used. This was
talked over on LKML in [2].
It's worth noting that it seems most archs calling
preempt_schedule_irq() have that outer loop, and for most of them it
really looks like they could get rid of it as well.
FWIW the suspects are
$ grep -r -I "preempt_schedule_irq" arch/ | cut -d/ -f2 | sort | uniq
arc
arm
arm64
c6x
csky
h8300
ia64
m68k
microblaze
mips
nds32
nios2
parisc
powerpc
s390
sh
sparc
x86
xtensa
- Patches 1-2 remove the loop for arm & arm64
- Patch 3 adds a bit of documentation to point out the loop isn't needed
to try and spread the word.
[2]: https://lore.kernel.org/lkml/[email protected]/
Valentin Schneider (3):
arm64: entry: Remove unneeded need_resched() loop
ARM: entry: Remove unneeded need_resched() loop
sched/Documentation: Point out use of preempt_schedule_irq()
Documentation/scheduler/sched-arch.txt | 10 ++++++++++
arch/arm/kernel/entry-armv.S | 12 +-----------
arch/arm64/kernel/entry.S | 11 +----------
3 files changed, 12 insertions(+), 21 deletions(-)
--
2.20.1
Since the enabling and disabling of IRQs within preempt_schedule_irq()
is contained in a need_resched() loop, we don't need the outer arch
code loop.
Reported-by: Julien Thierry <[email protected]>
Reported-by: Will Deacon <[email protected]>
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Julien Grall <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
---
arch/arm64/kernel/entry.S | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 0ec0c46b2c0c..4d0c81f29a60 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -611,7 +611,7 @@ el1_irq:
#ifdef CONFIG_PREEMPT
ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count
cbnz x24, 1f // preempt count != 0
- bl el1_preempt
+ bl preempt_schedule_irq // irq en/disable is done inside
1:
#endif
#ifdef CONFIG_TRACE_IRQFLAGS
@@ -620,15 +620,6 @@ el1_irq:
kernel_exit 1
ENDPROC(el1_irq)
-#ifdef CONFIG_PREEMPT
-el1_preempt:
- mov x24, lr
-1: bl preempt_schedule_irq // irq en/disable is done inside
- ldr x0, [tsk, #TSK_TI_FLAGS] // get new tasks TI_FLAGS
- tbnz x0, #TIF_NEED_RESCHED, 1b // needs rescheduling?
- ret x24
-#endif
-
/*
* EL0 mode handlers.
*/
--
2.20.1
Since the enabling and disabling of IRQs within preempt_schedule_irq()
is contained in a need_resched() loop, we don't need the outer arch
code loop.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Russell King <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: [email protected]
---
arch/arm/kernel/entry-armv.S | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index e85a3af9ddeb..a7ac22e09bb2 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -220,7 +220,7 @@ __irq_svc:
teq r8, #0 @ if preempt count != 0
movne r0, #0 @ force flags to 0
tst r0, #_TIF_NEED_RESCHED
- blne svc_preempt
+ blne preempt_schedule_irq @ irq en/disable is done inside
#endif
svc_exit r5, irq = 1 @ return from exception
@@ -229,16 +229,6 @@ ENDPROC(__irq_svc)
.ltorg
-#ifdef CONFIG_PREEMPT
-svc_preempt:
- mov r8, lr
-1: bl preempt_schedule_irq @ irq en/disable is done inside
- ldr r0, [tsk, #TI_FLAGS] @ get new tasks TI_FLAGS
- tst r0, #_TIF_NEED_RESCHED
- reteq r8 @ go again
- b 1b
-#endif
-
__und_fault:
@ Correct the PC such that it is pointing at the instruction
@ which caused the fault. If the faulting instruction was ARM
--
2.20.1
Since there are a few archs out there that call preempt_schedule_irq()
within a need_resched() loop, point out that it's not needed.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: [email protected]
---
Documentation/scheduler/sched-arch.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/scheduler/sched-arch.txt b/Documentation/scheduler/sched-arch.txt
index a2f27bbf2cba..ae41a94da700 100644
--- a/Documentation/scheduler/sched-arch.txt
+++ b/Documentation/scheduler/sched-arch.txt
@@ -59,6 +59,16 @@ Your cpu_idle routines need to obey the following rules:
arch/x86/kernel/process.c has examples of both polling and
sleeping idle functions.
+Kernel preemption
+=================
+When returning from interrupt context, you should call either of
+preempt_schedule() or preempt_schedule_irq() if preemption is enabled
+and need_resched() is true.
+
+Note that, unlike preempt_schedule(), preempt_schedule_irq() handles the
+enabling and disabling of IRQs. Since it also loops on need_resched(),
+you don't need to have that loop in your arch code. See
+arch/arm64/kernel/entry.S for an example.
Possible arch/ problems
=======================
--
2.20.1
Hi Valentin,
On 31/01/2019 18:23, Valentin Schneider wrote:
> Since there are a few archs out there that call preempt_schedule_irq()
> within a need_resched() loop, point out that it's not needed.
>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: [email protected]
> ---
> Documentation/scheduler/sched-arch.txt | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/scheduler/sched-arch.txt b/Documentation/scheduler/sched-arch.txt
> index a2f27bbf2cba..ae41a94da700 100644
> --- a/Documentation/scheduler/sched-arch.txt
> +++ b/Documentation/scheduler/sched-arch.txt
> @@ -59,6 +59,16 @@ Your cpu_idle routines need to obey the following rules:
> arch/x86/kernel/process.c has examples of both polling and
> sleeping idle functions.
>
> +Kernel preemption
> +=================
> +When returning from interrupt context, you should call either of
> +preempt_schedule() or preempt_schedule_irq() if preemption is enabled
> +and need_resched() is true.
> +
I don't think preempt_schedule() is really an option for a return from
interrupt. First thing preempt_schedule() does is:
if (likely(!preemptible()))
return;
And preemptible() is:
preempt_count() == 0 && !irqs_disabled()
Generally on return from interrupt context interrupts are disabled, so
we would never be preemptible() and preempt_schedule() would just do
nothing.
Unless I'm missing something.
Cheers,
--
Julien Thierry
Hi Valentin,
On 31/01/2019 18:23, Valentin Schneider wrote:
> Since the enabling and disabling of IRQs within preempt_schedule_irq()
> is contained in a need_resched() loop, we don't need the outer arch
> code loop.
>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: [email protected]
Reviewed-by: Julien Thierry <[email protected]>
Cheers,
> ---
> arch/arm/kernel/entry-armv.S | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index e85a3af9ddeb..a7ac22e09bb2 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -220,7 +220,7 @@ __irq_svc:
> teq r8, #0 @ if preempt count != 0
> movne r0, #0 @ force flags to 0
> tst r0, #_TIF_NEED_RESCHED
> - blne svc_preempt
> + blne preempt_schedule_irq @ irq en/disable is done inside
> #endif
>
> svc_exit r5, irq = 1 @ return from exception
> @@ -229,16 +229,6 @@ ENDPROC(__irq_svc)
>
> .ltorg
>
> -#ifdef CONFIG_PREEMPT
> -svc_preempt:
> - mov r8, lr
> -1: bl preempt_schedule_irq @ irq en/disable is done inside
> - ldr r0, [tsk, #TI_FLAGS] @ get new tasks TI_FLAGS
> - tst r0, #_TIF_NEED_RESCHED
> - reteq r8 @ go again
> - b 1b
> -#endif
> -
> __und_fault:
> @ Correct the PC such that it is pointing at the instruction
> @ which caused the fault. If the faulting instruction was ARM
>
--
Julien Thierry
Hi Valentin,
On 31/01/2019 18:23, Valentin Schneider wrote:
> Since the enabling and disabling of IRQs within preempt_schedule_irq()
> is contained in a need_resched() loop, we don't need the outer arch
> code loop.
>
> Reported-by: Julien Thierry <[email protected]>
> Reported-by: Will Deacon <[email protected]>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Julien Grall <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
Reviewed-by: Julien Thierry <[email protected]>
> ---
> arch/arm64/kernel/entry.S | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 0ec0c46b2c0c..4d0c81f29a60 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -611,7 +611,7 @@ el1_irq:
> #ifdef CONFIG_PREEMPT
> ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count
> cbnz x24, 1f // preempt count != 0
> - bl el1_preempt
> + bl preempt_schedule_irq // irq en/disable is done inside
> 1:
> #endif
> #ifdef CONFIG_TRACE_IRQFLAGS
> @@ -620,15 +620,6 @@ el1_irq:
> kernel_exit 1
> ENDPROC(el1_irq)
>
> -#ifdef CONFIG_PREEMPT
> -el1_preempt:
> - mov x24, lr
> -1: bl preempt_schedule_irq // irq en/disable is done inside
> - ldr x0, [tsk, #TSK_TI_FLAGS] // get new tasks TI_FLAGS
> - tbnz x0, #TIF_NEED_RESCHED, 1b // needs rescheduling?
> - ret x24
> -#endif
> -
> /*
> * EL0 mode handlers.
> */
>
--
Julien Thierry
On Thu, Jan 31, 2019 at 06:23:37PM +0000, Valentin Schneider wrote:
> Since the enabling and disabling of IRQs within preempt_schedule_irq()
> is contained in a need_resched() loop, we don't need the outer arch
> code loop.
>
> Reported-by: Julien Thierry <[email protected]>
> Reported-by: Will Deacon <[email protected]>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Julien Grall <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> ---
> arch/arm64/kernel/entry.S | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 0ec0c46b2c0c..4d0c81f29a60 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -611,7 +611,7 @@ el1_irq:
> #ifdef CONFIG_PREEMPT
> ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count
> cbnz x24, 1f // preempt count != 0
> - bl el1_preempt
> + bl preempt_schedule_irq // irq en/disable is done inside
> 1:
> #endif
> #ifdef CONFIG_TRACE_IRQFLAGS
> @@ -620,15 +620,6 @@ el1_irq:
> kernel_exit 1
> ENDPROC(el1_irq)
>
> -#ifdef CONFIG_PREEMPT
> -el1_preempt:
> - mov x24, lr
> -1: bl preempt_schedule_irq // irq en/disable is done inside
> - ldr x0, [tsk, #TSK_TI_FLAGS] // get new tasks TI_FLAGS
> - tbnz x0, #TIF_NEED_RESCHED, 1b // needs rescheduling?
> - ret x24
> -#endif
> -
Acked-by: Will Deacon <[email protected]>
Catalin can pick this up for 5.1.
Will
On 01/02/2019 08:45, Julien Thierry wrote:
[...]
>> +Kernel preemption
>> +=================
>> +When returning from interrupt context, you should call either of
>> +preempt_schedule() or preempt_schedule_irq() if preemption is enabled
>> +and need_resched() is true.
>> +
>
> I don't think preempt_schedule() is really an option for a return from
> interrupt. First thing preempt_schedule() does is:
>
> if (likely(!preemptible()))
> return;
>
> And preemptible() is:
>
> preempt_count() == 0 && !irqs_disabled()
>
> Generally on return from interrupt context interrupts are disabled, so
> we would never be preemptible() and preempt_schedule() would just do
> nothing.
>
> Unless I'm missing something.
>
No, I think you're right. The main reason I still mentioned it here is to
be conservative, although I must admit I've started confusing what we
have vs what we used to have since my dive into the history.
If I look at some archs that don't use preempt_schedule_irq() (e.g. alpha,
unicore32), they seem to be calling schedule() directly - but I don't see
any (*current*) user of preempt_schedule() on interrupt return.
preempt_schedule() still has this comment attached to it:
* this is the entry point to schedule() from in-kernel preemption
* off of preempt_enable. Kernel preemptions off return from interrupt
* occur there and call schedule directly.
So I might just remove the mention to preempt_schedule() in the doc and
also change the comment.
Thanks,
Valentin
On Thu, Jan 31, 2019 at 06:23:37PM +0000, Valentin Schneider wrote:
> Since the enabling and disabling of IRQs within preempt_schedule_irq()
> is contained in a need_resched() loop, we don't need the outer arch
> code loop.
>
> Reported-by: Julien Thierry <[email protected]>
> Reported-by: Will Deacon <[email protected]>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Julien Grall <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
Queued for 5.1. Thanks
--
Catalin