[RFC PATCH 0/3] arm/arm64: entry: Remove need_resched() loop

[RFC PATCH 0/3] arm/arm64: entry: Remove need_resched() loop

[Valentin Schneider]

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/cc989920-a13b-d53b-db83-1584a7f53edc@arm.com/

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

[RFC PATCH 1/3] arm64: 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.

Reported-by: Julien Thierry <julien.thierry@arm.com>
Reported-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
---
 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

Re: [RFC PATCH 1/3] arm64: entry: Remove unneeded need_resched() loop

[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 <julien.thierry@arm.com>
> Reported-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

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

Re: [RFC PATCH 1/3] arm64: entry: Remove unneeded need_resched() loop

[Will Deacon]

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 <julien.thierry@arm.com>
> Reported-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  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 <will.deacon@arm.com>

Catalin can pick this up for 5.1.

Will

Re: [RFC PATCH 1/3] arm64: entry: Remove unneeded need_resched() loop

[Catalin Marinas]

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 <julien.thierry@arm.com>
> Reported-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org

Queued for 5.1. Thanks

-- 
Catalin

[RFC PATCH 2/3] ARM: 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.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
---
 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

Re: [RFC PATCH 2/3] ARM: entry: Remove unneeded need_resched() loop

[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 <valentin.schneider@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

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

[RFC PATCH 3/3] sched/Documentation: Point out use of preempt_schedule_irq()

[Valentin Schneider]

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 <valentin.schneider@arm.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: linux-doc@vger.kernel.org
---
 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

Re: [RFC PATCH 3/3] sched/Documentation: Point out use of preempt_schedule_irq()

[Julien Thierry]

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 <valentin.schneider@arm.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: linux-doc@vger.kernel.org
> ---
>  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

Re: [RFC PATCH 3/3] sched/Documentation: Point out use of preempt_schedule_irq()

[Valentin Schneider]

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