[PATCH 00/14] entry: preempt_schedule_irq() callers scrub

[PATCH 00/14] entry: preempt_schedule_irq() callers scrub

[Valentin Schneider]

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/20190131182339.9835-1-valentin.schneider@arm.com/
[2]: https://lore.kernel.org/lkml/cc989920-a13b-d53b-db83-1584a7f53edc@arm.com/

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

[PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop

[Valentin Schneider]

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 <valentin.schneider@arm.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@mips.com>
Cc: James Hogan <jhogan@kernel.org>
Cc: linux-mips@vger.kernel.org
---
 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

Re: [PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop

[Paul Burton]

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 <valentin.schneider@arm.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paul.burton@mips.com>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: linux-mips@vger.kernel.org
> ---
>  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
> 

Re: [PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop

[Valentin Schneider]

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
>>

Re: [PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop

[Maciej W. Rozycki]

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

[PATCH v2] MIPS: entry: Remove unneeded need_resched() loop

[Valentin Schneider]

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 <valentin.schneider@arm.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@mips.com>
Cc: James Hogan <jhogan@kernel.org>
Cc: linux-mips@vger.kernel.org
---
 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

Re: [PATCH v2] MIPS: entry: Remove unneeded need_resched() loop

[Paul Burton]

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 <valentin.schneider@arm.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paul.burton@mips.com>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: linux-mips@vger.kernel.org

Applied to mips-next.

Thanks,
    Paul

[ This message was auto-generated; if you believe anything is incorrect
  then please email paul.burton@mips.com to report it. ]

Re: [PATCH 00/14] entry: preempt_schedule_irq() callers scrub

[Vineet Gupta]

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.

Re: [PATCH 00/14] entry: preempt_schedule_irq() callers scrub

[Valentin Schneider]

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.

[...]