Hi,
This is the continuation of [1] where I'm hunting down
preempt_schedule_irq() callers because of [2].
I told myself the best way to get this moving forward wouldn't be to write
doc about it, but to go write some fixes and get some discussions going,
which is what this patch-set is about.
I've looked at users of preempt_schedule_irq(), and made sure they didn't
have one of those useless loops. The list of offenders is:
$ 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
riscv
s390
sh
sparc
x86
xtensa
Regarding that loop, archs seem to fall in 3 categories:
A) Those that don't have the loop
B) Those that have a small need_resched() loop around the
preempt_schedule_irq() callsite
C) Those that branch to some more generic code further up the entry code
and eventually branch back to preempt_schedule_irq()
arc, m68k, nios2 fall in A)
sparc, ia64, s390 fall in C)
all the others fall in B)
I've written patches for B) and C) EXCEPT for ia64 and s390 because I
haven't been able to tell if it's actually fine to kill that "long jump"
(and maybe I'm wrong on sparc). Hopefully folks who understand what goes on
in there might be able to shed some light.
Also, since I sent patches for arm & arm64 in [1] I'm not including them
here.
Boot-tested on:
- x86
Build-tested on:
- h8300
- c6x
- powerpc
- mips
- nds32
- microblaze
- sparc
- xtensa
Thanks,
Valentin
[1]: https://lore.kernel.org/lkml/[email protected]/
[2]: https://lore.kernel.org/lkml/[email protected]/
Valentin Schneider (14):
sched/core: Fix preempt_schedule() interrupt return comment
c6x: entry: Remove unneeded need_resched() loop
csky: entry: Remove unneeded need_resched() loop
h8300: entry: Remove unneeded need_resched() loop
microblaze: entry: Remove unneeded need_resched() loop
MIPS: entry: Remove unneeded need_resched() loop
nds32: ex-exit: Remove unneeded need_resched() loop
powerpc: entry: Remove unneeded need_resched() loop
RISC-V: entry: Remove unneeded need_resched() loop
sh: entry: Remove unneeded need_resched() loop
sh64: entry: Remove unneeded need_resched() loop
sparc64: rtrap: Remove unneeded need_resched() loop
x86/entry: Remove unneeded need_resched() loop
xtensa: entry: Remove unneeded need_resched() loop
arch/c6x/kernel/entry.S | 3 +--
arch/csky/kernel/entry.S | 4 ----
arch/h8300/kernel/entry.S | 3 +--
arch/microblaze/kernel/entry.S | 5 -----
arch/mips/kernel/entry.S | 3 +--
arch/nds32/kernel/ex-exit.S | 4 ++--
arch/powerpc/kernel/entry_32.S | 6 +-----
arch/powerpc/kernel/entry_64.S | 8 +-------
arch/riscv/kernel/entry.S | 3 +--
arch/sh/kernel/cpu/sh5/entry.S | 5 +----
arch/sh/kernel/entry-common.S | 4 +---
arch/sparc/kernel/rtrap_64.S | 1 -
arch/x86/entry/entry_32.S | 3 +--
arch/x86/entry/entry_64.S | 3 +--
arch/xtensa/kernel/entry.S | 2 +-
kernel/sched/core.c | 7 +++----
16 files changed, 16 insertions(+), 48 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.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Mark Salter <[email protected]>
Cc: Aurelien Jacquiot <[email protected]>
Cc: [email protected]
---
arch/c6x/kernel/entry.S | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/c6x/kernel/entry.S b/arch/c6x/kernel/entry.S
index 2721c90b0121..17926e942edb 100644
--- a/arch/c6x/kernel/entry.S
+++ b/arch/c6x/kernel/entry.S
@@ -567,7 +567,6 @@ resume_kernel:
NOP 4
[A1] BNOP .S2 restore_all,5
-preempt_schedule:
GET_THREAD_INFO A2
LDW .D1T1 *+A2(THREAD_INFO_FLAGS),A1
#ifdef CONFIG_C6X_BIG_KERNEL
@@ -584,7 +583,7 @@ preempt_schedule:
#else
B .S2 preempt_schedule_irq
#endif
- ADDKPC .S2 preempt_schedule,B3,4
+ ADDKPC .S2 restore_all,B3,4
#endif /* CONFIG_PREEMPT */
ENTRY(enable_exception)
--
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: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: [email protected]
---
arch/powerpc/kernel/entry_32.S | 6 +-----
arch/powerpc/kernel/entry_64.S | 8 +-------
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 0768dfd8a64e..ff3fe3824a4a 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -896,11 +896,7 @@ resume_kernel:
*/
bl trace_hardirqs_off
#endif
-1: bl preempt_schedule_irq
- CURRENT_THREAD_INFO(r9, r1)
- lwz r3,TI_FLAGS(r9)
- andi. r0,r3,_TIF_NEED_RESCHED
- bne- 1b
+ bl preempt_schedule_irq
#ifdef CONFIG_TRACE_IRQFLAGS
/* And now, to properly rebalance the above, we tell lockdep they
* are being turned back on, which will happen when we return
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 435927f549c4..9c86c6826856 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -857,13 +857,7 @@ resume_kernel:
* sure we are soft-disabled first and reconcile irq state.
*/
RECONCILE_IRQ_STATE(r3,r4)
-1: bl preempt_schedule_irq
-
- /* Re-test flags and eventually loop */
- CURRENT_THREAD_INFO(r9, r1)
- ld r4,TI_FLAGS(r9)
- andi. r0,r4,_TIF_NEED_RESCHED
- bne 1b
+ bl preempt_schedule_irq
/*
* arch_local_irq_restore() from preempt_schedule_irq above may
--
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: Yoshinori Sato <[email protected]>
Cc: Rich Felker <[email protected]>
Cc: [email protected]
---
arch/sh/kernel/entry-common.S | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
index d31f66e82ce5..65a105de52a0 100644
--- a/arch/sh/kernel/entry-common.S
+++ b/arch/sh/kernel/entry-common.S
@@ -93,7 +93,7 @@ ENTRY(resume_kernel)
mov.l @(TI_PRE_COUNT,r8), r0 ! current_thread_info->preempt_count
tst r0, r0
bf noresched
-need_resched:
+
mov.l @(TI_FLAGS,r8), r0 ! current_thread_info->flags
tst #_TIF_NEED_RESCHED, r0 ! need_resched set?
bt noresched
@@ -107,8 +107,6 @@ need_resched:
mov.l 1f, r0
jsr @r0 ! call preempt_schedule_irq
nop
- bra need_resched
- nop
noresched:
bra __restore_all
--
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: Andy Lutomirski <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
---
arch/x86/entry/entry_32.S | 3 +--
arch/x86/entry/entry_64.S | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index d309f30cf7af..b1856fe9decf 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -766,13 +766,12 @@ END(ret_from_exception)
#ifdef CONFIG_PREEMPT
ENTRY(resume_kernel)
DISABLE_INTERRUPTS(CLBR_ANY)
-.Lneed_resched:
cmpl $0, PER_CPU_VAR(__preempt_count)
jnz restore_all_kernel
testl $X86_EFLAGS_IF, PT_EFLAGS(%esp) # interrupts off (exception path) ?
jz restore_all_kernel
call preempt_schedule_irq
- jmp .Lneed_resched
+ jmp restore_all_kernel
END(resume_kernel)
#endif
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1f0efdb7b629..e7e270603fe7 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -645,10 +645,9 @@ retint_kernel:
/* Check if we need preemption */
btl $9, EFLAGS(%rsp) /* were interrupts off? */
jnc 1f
-0: cmpl $0, PER_CPU_VAR(__preempt_count)
+ cmpl $0, PER_CPU_VAR(__preempt_count)
jnz 1f
call preempt_schedule_irq
- jmp 0b
1:
#endif
/*
--
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: Yoshinori Sato <[email protected]>
Cc: Rich Felker <[email protected]>
Cc: [email protected]
---
arch/sh/kernel/cpu/sh5/entry.S | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/sh/kernel/cpu/sh5/entry.S b/arch/sh/kernel/cpu/sh5/entry.S
index de68ffdfffbf..40e6d9a7a6a2 100644
--- a/arch/sh/kernel/cpu/sh5/entry.S
+++ b/arch/sh/kernel/cpu/sh5/entry.S
@@ -897,7 +897,6 @@ resume_kernel:
ld.l r6, TI_PRE_COUNT, r7
beq/u r7, ZERO, tr0
-need_resched:
ld.l r6, TI_FLAGS, r7
movi (1 << TIF_NEED_RESCHED), r8
and r8, r7, r8
@@ -911,9 +910,7 @@ need_resched:
ori r7, 1, r7
ptabs r7, tr1
blink tr1, LINK
-
- pta need_resched, tr1
- blink tr1, ZERO
+ blink tr0, ZERO
#endif
.global ret_from_syscall
--
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: Chris Zankel <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: [email protected]
---
arch/xtensa/kernel/entry.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/xtensa/kernel/entry.S b/arch/xtensa/kernel/entry.S
index e50f5124dc6f..43ecaac14ae6 100644
--- a/arch/xtensa/kernel/entry.S
+++ b/arch/xtensa/kernel/entry.S
@@ -529,7 +529,7 @@ common_exception_return:
l32i a4, a2, TI_PRE_COUNT
bnez a4, 4f
call4 preempt_schedule_irq
- j 1b
+ j 4f
#endif
#if XTENSA_FAKE_NMI
--
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.
We seem to be looping back somewhere much higher than the usual
preempt/need_resched checks, but AFAICT we would just branch to
'rtrap_no_irq_enable' and then back to 'to_kernel', which is
a need_resched() loop with a few extra steps.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
---
arch/sparc/kernel/rtrap_64.S | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/sparc/kernel/rtrap_64.S b/arch/sparc/kernel/rtrap_64.S
index 29aa34f11720..57fb75686957 100644
--- a/arch/sparc/kernel/rtrap_64.S
+++ b/arch/sparc/kernel/rtrap_64.S
@@ -322,7 +322,6 @@ to_kernel:
nop
call preempt_schedule_irq
nop
- ba,pt %xcc, rtrap
#endif
kern_fpucheck: ldub [%g6 + TI_FPDEPTH], %l5
brz,pt %l5, rt_continue
--
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.
Do note that commit a18815abcdfd ("Use preempt_schedule_irq.")
initially removed the existing loop, but commit cdaed73afb61 ("Fix
preemption bug.") reintroduced it. It is however not clear what the
issue was and why such a loop was reintroduced, so I'm probably
missing something.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: James Hogan <[email protected]>
Cc: [email protected]
---
arch/mips/kernel/entry.S | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
index d7de8adcfcc8..2240faeda62a 100644
--- a/arch/mips/kernel/entry.S
+++ b/arch/mips/kernel/entry.S
@@ -58,7 +58,6 @@ resume_kernel:
local_irq_disable
lw t0, TI_PRE_COUNT($28)
bnez t0, restore_all
-need_resched:
LONG_L t0, TI_FLAGS($28)
andi t1, t0, _TIF_NEED_RESCHED
beqz t1, restore_all
@@ -66,7 +65,7 @@ need_resched:
andi t0, 1
beqz t0, restore_all
jal preempt_schedule_irq
- b need_resched
+ j restore_all
#endif
FEXPORT(ret_from_kernel_thread)
--
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: Yoshinori Sato <[email protected]>
Cc: [email protected]
---
arch/h8300/kernel/entry.S | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/h8300/kernel/entry.S b/arch/h8300/kernel/entry.S
index 4ade5f8299ba..6bde028e7d4a 100644
--- a/arch/h8300/kernel/entry.S
+++ b/arch/h8300/kernel/entry.S
@@ -323,7 +323,6 @@ restore_all:
resume_kernel:
mov.l @(TI_PRE_COUNT:16,er4),er0
bne restore_all:8
-need_resched:
mov.l @(TI_FLAGS:16,er4),er0
btst #TIF_NEED_RESCHED,r0l
beq restore_all:8
@@ -332,7 +331,7 @@ need_resched:
mov.l sp,er0
jsr @set_esp0
jsr @preempt_schedule_irq
- bra need_resched:8
+ bra restore_all:8
#endif
ret_from_fork:
--
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: Greentime Hu <[email protected]>
Cc: Vincent Chen <[email protected]>
---
arch/nds32/kernel/ex-exit.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/nds32/kernel/ex-exit.S b/arch/nds32/kernel/ex-exit.S
index 97ba15cd4180..1df02a793364 100644
--- a/arch/nds32/kernel/ex-exit.S
+++ b/arch/nds32/kernel/ex-exit.S
@@ -163,7 +163,7 @@ resume_kernel:
gie_disable
lwi $t0, [tsk+#TSK_TI_PREEMPT]
bnez $t0, no_work_pending
-need_resched:
+
lwi $t0, [tsk+#TSK_TI_FLAGS]
andi $p1, $t0, #_TIF_NEED_RESCHED
beqz $p1, no_work_pending
@@ -173,7 +173,7 @@ need_resched:
beqz $t0, no_work_pending
jal preempt_schedule_irq
- b need_resched
+ b no_work_pending
#endif
/*
--
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: Palmer Dabbelt <[email protected]>
Cc: Albert Ou <[email protected]>
Cc: [email protected]
---
arch/riscv/kernel/entry.S | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index fd9b57c8b4ce..18e3337e9c30 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -258,12 +258,11 @@ restore_all:
resume_kernel:
REG_L s0, TASK_TI_PREEMPT_COUNT(tp)
bnez s0, restore_all
-need_resched:
REG_L s0, TASK_TI_FLAGS(tp)
andi s0, s0, _TIF_NEED_RESCHED
beqz s0, restore_all
call preempt_schedule_irq
- j need_resched
+ j restore_all
#endif
work_pending:
--
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: Michal Simek <[email protected]>
---
arch/microblaze/kernel/entry.S | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/microblaze/kernel/entry.S b/arch/microblaze/kernel/entry.S
index 4e1b567becd6..de7083bd1d24 100644
--- a/arch/microblaze/kernel/entry.S
+++ b/arch/microblaze/kernel/entry.S
@@ -738,14 +738,9 @@ no_intr_resched:
andi r5, r5, _TIF_NEED_RESCHED;
beqi r5, restore /* if zero jump over */
-preempt:
/* interrupts are off that's why I am calling preempt_chedule_irq */
bralid r15, preempt_schedule_irq
nop
- lwi r11, CURRENT_TASK, TS_THREAD_INFO; /* get thread info */
- lwi r5, r11, TI_FLAGS; /* get flags in thread info */
- andi r5, r5, _TIF_NEED_RESCHED;
- bnei r5, preempt /* if non zero jump to resched */
restore:
#endif
VM_OFF /* MS: turn off MMU */
--
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: Guo Ren <[email protected]>
---
arch/csky/kernel/entry.S | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/csky/kernel/entry.S b/arch/csky/kernel/entry.S
index 5137ed9062bd..2d9e4776ba7a 100644
--- a/arch/csky/kernel/entry.S
+++ b/arch/csky/kernel/entry.S
@@ -307,11 +307,7 @@ ENTRY(csky_irq)
ldw r8, (r9, TINFO_FLAGS)
btsti r8, TIF_NEED_RESCHED
bf 2f
-1:
jbsr preempt_schedule_irq /* irq en/disable is done inside */
- ldw r7, (r9, TINFO_FLAGS) /* get new tasks TI_FLAGS */
- btsti r7, TIF_NEED_RESCHED
- bt 1b /* go again */
#endif
2:
jmpi ret_from_exception
--
2.20.1
preempt_schedule_irq() is the one that should be called on return from
interrupt, clean up the comment to avoid any ambiguity.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
kernel/sched/core.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ead464a0f2e5..78c5aaa28278 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3652,9 +3652,8 @@ static void __sched notrace preempt_schedule_common(void)
#ifdef CONFIG_PREEMPT
/*
- * 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.
+ * This is the entry point to schedule() from in-kernel preemption
+ * off of preempt_enable.
*/
asmlinkage __visible void __sched notrace preempt_schedule(void)
{
@@ -3725,7 +3724,7 @@ EXPORT_SYMBOL_GPL(preempt_schedule_notrace);
#endif /* CONFIG_PREEMPT */
/*
- * this is the entry point to schedule() from kernel preemption
+ * This is the entry point to schedule() from kernel preemption
* off of irq context.
* Note, that this is called and return with irqs disabled. This will
* protect us against recursive calling from irq.
--
2.20.1
From: Valentin Schneider <[email protected]>
Date: Mon, 11 Mar 2019 22:47:50 +0000
> 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.
>
> We seem to be looping back somewhere much higher than the usual
> preempt/need_resched checks, but AFAICT we would just branch to
> 'rtrap_no_irq_enable' and then back to 'to_kernel', which is
> a need_resched() loop with a few extra steps.
>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: [email protected]
> ---
> arch/sparc/kernel/rtrap_64.S | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/sparc/kernel/rtrap_64.S b/arch/sparc/kernel/rtrap_64.S
> index 29aa34f11720..57fb75686957 100644
> --- a/arch/sparc/kernel/rtrap_64.S
> +++ b/arch/sparc/kernel/rtrap_64.S
> @@ -322,7 +322,6 @@ to_kernel:
> nop
> call preempt_schedule_irq
> nop
> - ba,pt %xcc, rtrap
> #endif
> kern_fpucheck: ldub [%g6 + TI_FPDEPTH], %l5
> brz,pt %l5, rt_continue
> --
> 2.20.1
>
We must re-evaluate the %tstate value stored in ptregs, you cannot
make this change.
On Mon, Mar 11, 2019 at 3:48 PM Valentin Schneider
<[email protected]> 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: Chris Zankel <[email protected]>
> Cc: Max Filippov <[email protected]>
> Cc: [email protected]
> ---
> arch/xtensa/kernel/entry.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Max Filippov <[email protected]>
--
Thanks.
-- Max
Hi,
On 11/03/2019 23:13, David Miller wrote:
[...]
>
> We must re-evaluate the %tstate value stored in ptregs, you cannot
> make this change.
>
That's the one I was the less sure about, thanks for clearing it up and
sorry for the noise.
On 12/03/2019 01:10, Max Filippov wrote:
[...]
>
> Acked-by: Max Filippov <[email protected]>
>
Thanks!
On 3/11/19 3:47 PM, Valentin Schneider wrote:
> Hi,
>
> This is the continuation of [1] where I'm hunting down
> preempt_schedule_irq() callers because of [2].
>
> I told myself the best way to get this moving forward wouldn't be to write
> doc about it, but to go write some fixes and get some discussions going,
> which is what this patch-set is about.
>
> I've looked at users of preempt_schedule_irq(), and made sure they didn't
> have one of those useless loops. The list of offenders is:
>
> $ grep -r -I "preempt_schedule_irq" arch/ | cut -d/ -f2 | sort | uniq
>
...
>
> Regarding that loop, archs seem to fall in 3 categories:
> A) Those that don't have the loop
Please clarify that this is the right thing to do (since core code already has the
loop) hence no fixing is required for this "category"
> B) Those that have a small need_resched() loop around the
> preempt_schedule_irq() callsite
> C) Those that branch to some more generic code further up the entry code
> and eventually branch back to preempt_schedule_irq()
>
> arc, m68k, nios2 fall in A)
> sparc, ia64, s390 fall in C)
> all the others fall in B)
>
> I've written patches for B) and C) EXCEPT for ia64 and s390 because I
> haven't been able to tell if it's actually fine to kill that "long jump"
> (and maybe I'm wrong on sparc). Hopefully folks who understand what goes on
> in there might be able to shed some light.
On 12/03/2019 18:03, Vineet Gupta wrote:
[...]
>> Regarding that loop, archs seem to fall in 3 categories:
>> A) Those that don't have the loop
>
> Please clarify that this is the right thing to do (since core code already has the
> loop) hence no fixing is required for this "category"
>
Right, those don't need any change. I had a brief look at them to double
check they had the proper need_resched() gate before calling
preempt_schedule_irq() (with no loop) and they all seem fine. Also...
>> B) Those that have a small need_resched() loop around the
>> preempt_schedule_irq() callsite
>> C) Those that branch to some more generic code further up the entry code
>> and eventually branch back to preempt_schedule_irq()
>>
>> arc, m68k, nios2 fall in A)
>
I forgot to include parisc in here.
[...]
On Mon, 2019-03-11 at 22:47 +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.
>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Mark Salter <[email protected]>
> Cc: Aurelien Jacquiot <[email protected]>
> Cc: [email protected]
> ---
> arch/c6x/kernel/entry.S | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/c6x/kernel/entry.S b/arch/c6x/kernel/entry.S
> index 2721c90b0121..17926e942edb 100644
> --- a/arch/c6x/kernel/entry.S
> +++ b/arch/c6x/kernel/entry.S
> @@ -567,7 +567,6 @@ resume_kernel:
> NOP 4
> [A1] BNOP .S2 restore_all,5
>
> -preempt_schedule:
> GET_THREAD_INFO A2
> LDW .D1T1 *+A2(THREAD_INFO_FLAGS),A1
> #ifdef CONFIG_C6X_BIG_KERNEL
> @@ -584,7 +583,7 @@ preempt_schedule:
> #else
> B .S2 preempt_schedule_irq
> #endif
> - ADDKPC .S2 preempt_schedule,B3,4
> + ADDKPC .S2 restore_all,B3,4
> #endif /* CONFIG_PREEMPT */
>
> ENTRY(enable_exception)
Acked-by: Mark Salter <[email protected]>
Hi Valentin,
On Mon, Mar 11, 2019 at 10:47:44PM +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.
>
> Do note that commit a18815abcdfd ("Use preempt_schedule_irq.")
> initially removed the existing loop, but commit cdaed73afb61 ("Fix
> preemption bug.") reintroduced it. It is however not clear what the
> issue was and why such a loop was reintroduced, so I'm probably
> missing something.
It looks to me like commit a18815abcdfd ("Use preempt_schedule_irq.")
forgot the branch to restore_all, so would have fallen through to
ret_from_fork() & done weird things.
Adding the branch to restore_all as you're doing here would have been a
better fix than commit cdaed73afb61 ("Fix preemption bug.").
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: James Hogan <[email protected]>
> Cc: [email protected]
> ---
> arch/mips/kernel/entry.S | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
> index d7de8adcfcc8..2240faeda62a 100644
> --- a/arch/mips/kernel/entry.S
> +++ b/arch/mips/kernel/entry.S
> @@ -58,7 +58,6 @@ resume_kernel:
> local_irq_disable
> lw t0, TI_PRE_COUNT($28)
> bnez t0, restore_all
> -need_resched:
> LONG_L t0, TI_FLAGS($28)
> andi t1, t0, _TIF_NEED_RESCHED
> beqz t1, restore_all
> @@ -66,7 +65,7 @@ need_resched:
> andi t0, 1
> beqz t0, restore_all
> jal preempt_schedule_irq
> - b need_resched
> + j restore_all
One nit - why change from branch to jump? It's not a big deal, but I'd
prefer we stick with the branch ("b") instruction for a few reasons:
- restore_all is nearby so there's no issue with it being out of range
of a branch in any variation of the MIPS ISA.
- It's more consistent with the future of the MIPS architecture, eg.
nanoMIPS in which branch instructions all use PC-relative immediate
offsets & jump instructions are always of the "register" variety where
the destination is specified by a register rather than an immediate
encoded in the instruction (the assembler will fix it up & emit a
branch anyway, but I generally prefer to invoke less magic in these
areas...).
- A PC-relative branch won't generate an extra reloc in a relocatable
kernel, whereas a jump will.
Even better would be if we take advantage of this being a tail call & do
this:
PTR_LA ra, restore_all
j preempt_schedule_irq
(Where I left the call to preempt_schedule_irq using a jump because it
may be further away.)
Though I don't mind if you wanna just s/j/b/ & leave the tail call
optimisation for someone else to do as a later change.
Thanks,
Paul
> #endif
>
> FEXPORT(ret_from_kernel_thread)
> --
> 2.20.1
>
Hi Paul,
On 14/03/2019 18:13, Paul Burton wrote:
[...]
>
> It looks to me like commit a18815abcdfd ("Use preempt_schedule_irq.")
> forgot the branch to restore_all, so would have fallen through to
> ret_from_fork() & done weird things.
>
> Adding the branch to restore_all as you're doing here would have been a
> better fix than commit cdaed73afb61 ("Fix preemption bug.").
>
I didn't notice the missing branch to restore_all in that first commit -
that makes (more) sense now.
[...]
>> @@ -66,7 +65,7 @@ need_resched:
>> andi t0, 1
>> beqz t0, restore_all
>> jal preempt_schedule_irq
>> - b need_resched
>> + j restore_all
>
> One nit - why change from branch to jump?
No actual reason there, I most likely deleted the branch, looked around,
saw the "j restore_all" in @resume_userspace and went for that (shoddy
I know...)
> It's not a big deal, but I'd
> prefer we stick with the branch ("b") instruction for a few reasons:
>
> - restore_all is nearby so there's no issue with it being out of range
> of a branch in any variation of the MIPS ISA.
>
> - It's more consistent with the future of the MIPS architecture, eg.
> nanoMIPS in which branch instructions all use PC-relative immediate
> offsets & jump instructions are always of the "register" variety where
> the destination is specified by a register rather than an immediate
> encoded in the instruction (the assembler will fix it up & emit a
> branch anyway, but I generally prefer to invoke less magic in these
> areas...).
>
> - A PC-relative branch won't generate an extra reloc in a relocatable
> kernel, whereas a jump will.
>
Makes total sense, thanks for the detailed reasoning!
> Even better would be if we take advantage of this being a tail call & do
> this:
>
> PTR_LA ra, restore_all
> j preempt_schedule_irq
>
> (Where I left the call to preempt_schedule_irq using a jump because it
> may be further away.)
>
Right, that's even better, I'll send a v2 with that.
> Though I don't mind if you wanna just s/j/b/ & leave the tail call
> optimisation for someone else to do as a later change.
>
> Thanks,
> Paul
>
>> #endif
>>
>> FEXPORT(ret_from_kernel_thread)
>> --
>> 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.
Note that commit a18815abcdfd ("Use preempt_schedule_irq.") initially
removed the existing loop, but missed the final branch to restore_all.
Commit cdaed73afb61 ("Fix preemption bug.") missed that and reintroduced
the loop.
Signed-off-by: Valentin Schneider <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: James Hogan <[email protected]>
Cc: [email protected]
---
arch/mips/kernel/entry.S | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S
index d7de8adcfcc8..5469d43b6966 100644
--- a/arch/mips/kernel/entry.S
+++ b/arch/mips/kernel/entry.S
@@ -58,15 +58,14 @@ resume_kernel:
local_irq_disable
lw t0, TI_PRE_COUNT($28)
bnez t0, restore_all
-need_resched:
LONG_L t0, TI_FLAGS($28)
andi t1, t0, _TIF_NEED_RESCHED
beqz t1, restore_all
LONG_L t0, PT_STATUS(sp) # Interrupts off?
andi t0, 1
beqz t0, restore_all
- jal preempt_schedule_irq
- b need_resched
+ PTR_LA ra, restore_all
+ j preempt_schedule_irq
#endif
FEXPORT(ret_from_kernel_thread)
--
2.20.1
Hello,
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.
>
> Note that commit a18815abcdfd ("Use preempt_schedule_irq.") initially
> removed the existing loop, but missed the final branch to restore_all.
> Commit cdaed73afb61 ("Fix preemption bug.") missed that and reintroduced
> the loop.
>
> Signed-off-by: Valentin Schneider <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: James Hogan <[email protected]>
> Cc: [email protected]
Applied to mips-next.
Thanks,
Paul
[ This message was auto-generated; if you believe anything is incorrect
then please email [email protected] to report it. ]
Commit-ID: b5b447b6b4e899e0978b95cb42d272978f8c7b4d
Gitweb: https://git.kernel.org/tip/b5b447b6b4e899e0978b95cb42d272978f8c7b4d
Author: Valentin Schneider <[email protected]>
AuthorDate: Mon, 11 Mar 2019 22:47:51 +0000
Committer: Thomas Gleixner <[email protected]>
CommitDate: Fri, 5 Apr 2019 15:08:03 +0200
x86/entry: Remove unneeded need_resched() loop
Since the enabling and disabling of IRQs within preempt_schedule_irq() is
contained in a need_resched() loop, there is no need for the outer
architecture specific loop.
Signed-off-by: Valentin Schneider <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/entry/entry_32.S | 3 +--
arch/x86/entry/entry_64.S | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index d309f30cf7af..b1856fe9decf 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -766,13 +766,12 @@ END(ret_from_exception)
#ifdef CONFIG_PREEMPT
ENTRY(resume_kernel)
DISABLE_INTERRUPTS(CLBR_ANY)
-.Lneed_resched:
cmpl $0, PER_CPU_VAR(__preempt_count)
jnz restore_all_kernel
testl $X86_EFLAGS_IF, PT_EFLAGS(%esp) # interrupts off (exception path) ?
jz restore_all_kernel
call preempt_schedule_irq
- jmp .Lneed_resched
+ jmp restore_all_kernel
END(resume_kernel)
#endif
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1f0efdb7b629..e7e270603fe7 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -645,10 +645,9 @@ retint_kernel:
/* Check if we need preemption */
btl $9, EFLAGS(%rsp) /* were interrupts off? */
jnc 1f
-0: cmpl $0, PER_CPU_VAR(__preempt_count)
+ cmpl $0, PER_CPU_VAR(__preempt_count)
jnz 1f
call preempt_schedule_irq
- jmp 0b
1:
#endif
/*
Hi Valentin,
Valentin Schneider <[email protected]> 於 2019年3月12日 週二 上午6:48寫道：
>
> 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: Greentime Hu <[email protected]>
> Cc: Vincent Chen <[email protected]>
> ---
> arch/nds32/kernel/ex-exit.S | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/nds32/kernel/ex-exit.S b/arch/nds32/kernel/ex-exit.S
> index 97ba15cd4180..1df02a793364 100644
> --- a/arch/nds32/kernel/ex-exit.S
> +++ b/arch/nds32/kernel/ex-exit.S
> @@ -163,7 +163,7 @@ resume_kernel:
> gie_disable
> lwi $t0, [tsk+#TSK_TI_PREEMPT]
> bnez $t0, no_work_pending
> -need_resched:
> +
> lwi $t0, [tsk+#TSK_TI_FLAGS]
> andi $p1, $t0, #_TIF_NEED_RESCHED
> beqz $p1, no_work_pending
> @@ -173,7 +173,7 @@ need_resched:
> beqz $t0, no_work_pending
>
> jal preempt_schedule_irq
> - b need_resched
> + b no_work_pending
> #endif
>
> /*
Thank you. :)
Acked-by: Greentime Hu <[email protected]>
On Mon, 2019-03-11 at 22:47:46 UTC, 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: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: [email protected]
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/90437bffa5f9b1440ba03e023f4875d1
cheers
On Thu, 14 Mar 2019, Paul Burton wrote:
> > @@ -66,7 +65,7 @@ need_resched:
> > andi t0, 1
> > beqz t0, restore_all
> > jal preempt_schedule_irq
> > - b need_resched
> > + j restore_all
>
> One nit - why change from branch to jump? It's not a big deal, but I'd
> prefer we stick with the branch ("b") instruction for a few reasons:
>
> - restore_all is nearby so there's no issue with it being out of range
> of a branch in any variation of the MIPS ISA.
FYI, if it does go out of range for whatever reason, then for non-PIC
code GAS will relax it to a jump anyway (with a relocation attached); for
the regular MIPS ISA that is, where it has been doing that since forever
(I meant to implement this for the microMIPS ISA too, but IIRC there was a
complication there, probably coming from the existing more complex branch
relaxation code and/or slightly different use of relocations, and then it
fell through the cracks).
Maciej