arm64: Only call into preempt_schedule() if need_resched()
v2 cover arm64: Only call into preempt_schedule() if need_resched()
Hi all,
This is version two of the patches I originally posted here:
http://lkml.kernel.org/r/[email protected]
The only change since v1 is that __preempt_count_dec_and_test() now reloads the need_resched flag if it initially saw that it was set. This resolves the issue spotted by Peter, where an IRQ coming in during the decrement can cause a reschedule to be missed.
Feedback welcome.
Will
--->8
Will Deacon (2):
preempt: Move PREEMPT_NEED_RESCHED definition into arch code
arm64: preempt: Provide our own implementation of asm/preempt.h
arch/arm64/include/asm/Kbuild | 1 - arch/arm64/include/asm/preempt.h | 88 ++++++++++++++++++++++++++++++++++++ arch/arm64/include/asm/thread_info.h | 13 +++++- arch/s390/include/asm/preempt.h | 2 + arch/x86/include/asm/preempt.h | 3 ++ include/linux/preempt.h | 3 -- 6 files changed, 105 insertions(+), 5 deletions(-) create mode 100644 arch/arm64/include/asm/preempt.h
Signature · 1 line
2.1.4
PATCH v2 1/2 preempt: Move PREEMPT_NEED_RESCHED definition into arch code
PREEMPT_NEED_RESCHED is never used directly, so move it into the arch code where it can potentially be implemented using either a different bit in the preempt count or as an entirely separate entity.
--- arch/s390/include/asm/preempt.h | 2 ++ arch/x86/include/asm/preempt.h | 3 +++ include/linux/preempt.h | 3 --- 3 files changed, 5 insertions(+), 3 deletions(-)
arch/s390/include/asm/preempt.h +2 -0
@@ -8,6 +8,8 @@#ifdef CONFIG_HAVE_MARCH_Z196_FEATURES +/* We use the MSB mostly because its available */ +#define PREEMPT_NEED_RESCHED 0x80000000#define PREEMPT_ENABLED (0 + PREEMPT_NEED_RESCHED)static inline int preempt_count (void )
arch/x86/include/asm/preempt.h +3 -0
@@ -8,6 +8,9 @@DECLARE_PER_CPU (int, __preempt_count); +/* We use the MSB mostly because its available */ +#define PREEMPT_NEED_RESCHED 0x80000000 +/* * We use the PREEMPT_NEED_RESCHED bit as an inverted NEED_RESCHED such * that a decrement hitting 0 means we can and should reschedule.
include/linux/preempt.h +0 -3
@@ -53,9 +53,6 @@#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET) -/* We use the MSB mostly because its available */ -#define PREEMPT_NEED_RESCHED 0x80000000 -#define PREEMPT_DISABLED (PREEMPT_DISABLE_OFFSET + PREEMPT_ENABLED)/*
Signature · 1 line
2.1.4
PATCH v2 2/2 arm64: preempt: Provide our own implementation of asm/preempt.h
In reply to Will DeaconThe asm-generic/preempt.h implementation doesn't make use of the PREEMPT_NEED_RESCHED flag, since this can interact badly with load/store architectures which rely on the preempt_count word being unchanged across an interrupt.
However, since we're a 64-bit architecture and the preempt count is only 32 bits wide, we can simply pack it next to the resched flag and load the whole thing in one go, so that a dec-and-test operation doesn't need to load twice.
--- arch/arm64/include/asm/Kbuild | 1 - arch/arm64/include/asm/preempt.h | 88 ++++++++++++++++++++++++++++++++++++ arch/arm64/include/asm/thread_info.h | 13 +++++- 3 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/include/asm/preempt.h
arch/arm64/include/asm/Kbuild +0 -1
@@ -14,7 +14,6 @@ generic-y += local64.h generic-y += mcs_spinlock.h generic-y += mm-arch-hooks.h generic-y += msi.h -generic-y += preempt.h generic-y += qrwlock.h generic-y += qspinlock.h generic-y += rwsem.h
arch/arm64/include/asm/preempt.h new file +88 -0
88 lines not on this page · raw message
arch/arm64/include/asm/thread_info.h +12 -1
@@ -42,7 +42,18 @@ struct thread_info {#ifdef CONFIG_ARM64_SW_TTBR0_PAN u64 ttbr0;/* saved TTBR0_EL1 */ #endif -int preempt_count;/* 0 => preemptable, <0 => bug */ +union { +u64 preempt_count;/* 0 => preemptible, <0 => bug */ +struct { +#ifdef CONFIG_CPU_BIG_ENDIAN +u32 need_resched; +u32 count; +#else +u32 count; +u32 need_resched; +#endif + } preempt; + }; };#define thread_saved_pc (tsk) \
Signature · 1 line
2.1.4
Re: [PATCH v2 2/2] arm64: preempt: Provide our own implementation of asm/preempt.h
Quoted 6 lines · Will Deacon
The
asm-generic/preempt.himplementation doesn't make use of thePREEMPT_NEED_RESCHEDflag, since this can interact badly with load/store architectures which rely on thepreempt_countword being unchanged across an interrupt.
Quoted 6 lines
However, since we're a 64-bit architecture and the preempt count is only 32 bits wide, we can simply pack it next to the resched flag and load the whole thing in one go, so that a dec-and-test operation doesn't need to load twice.
Signed-off-by Will Deacon <[email protected]>
One nit below.
Quoted 78 lines
--- arch/arm64/include/asm/Kbuild | 1 - arch/arm64/include/asm/preempt.h | 88 ++++++++++++++++++++++++++++++++++++ arch/arm64/include/asm/thread_info.h | 13 +++++- 3 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/include/asm/preempt.harch/arm64/include/asm/Kbuild +0
-1@@ -14,7 +14,6 @@ generic-y += local64.h generic-y += mcs_spinlock.h generic-y += mm-arch-hooks.h generic-y += msi.h -generic-y += preempt.h generic-y += qrwlock.h generic-y += qspinlock.h generic-y += rwsem.harch/arm64/include/asm/preempt.h new file +53
-0@@ -0,0 +1,88 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ASM_PREEMPT_H +#define __ASM_PREEMPT_H + +#include <linux/thread_info.h> + +#define PREEMPT_NEED_RESCHED BIT(32) +#define PREEMPT_ENABLED (PREEMPT_NEED_RESCHED) + +static inline int preempt_count (void ) +{ +return READ_ONCE (current_thread_info ()->preempt.count); +} + +static inline void preempt_count_set (u64 pc) +{ +/* Preserve existing value of PREEMPT_NEED_RESCHED */ +WRITE_ONCE (current_thread_info ()->preempt.count, pc); +} + +#define init_task_preempt_count (p) do { \ + task_thread_info(p)->preempt_count = FORK_PREEMPT_COUNT; \ +} while (0) + +#define init_idle_preempt_count (p, cpu) do { \ + task_thread_info(p)->preempt_count = PREEMPT_ENABLED; \ +} while (0) + +static inline void set_preempt_need_resched (void ) +{ +current_thread_info ()->preempt.need_resched =0 ; +} + +static inline void clear_preempt_need_resched (void ) +{ +current_thread_info ()->preempt.need_resched =1 ; +} + +static inline bool test_preempt_need_resched (void ) +{ +return !current_thread_info ()->preempt.need_resched; +} + +static inline void __preempt_count_add (int val) +{ +u32 pc =READ_ONCE (current_thread_info ()->preempt.count); + pc += val; +WRITE_ONCE (current_thread_info ()->preempt.count, pc); +} + +static inline void __preempt_count_sub (int val) +{ +u32 pc =READ_ONCE (current_thread_info ()->preempt.count);
arch/arm64/include/asm/preempt.h line 63
Quoted 10 lines
arch/arm64/include/asm/preempt.h new file +10
-0+ pc -= val; +WRITE_ONCE (current_thread_info ()->preempt.count, pc); +} + +static inline bool __preempt_count_dec_and_test (void ) +{ +struct thread_info *ti =current_thread_info (); +u64 pc =READ_ONCE (ti->preempt_count); + +WRITE_ONCE (ti->preempt.count, --pc);
The difference between preempt.count and preempt_count doesn't really stand out visually, so perhaps add a comment here that the truncation is intentional?
Quoted 52 lines
arch/arm64/include/asm/preempt.h new file +25
-0+ +/* +* If we wrote back all zeroes, then we're preemptible and in +* need of a reschedule. Otherwise, we need to reload the +* preempt_count in case the need_resched flag was cleared by an +* interrupt occurring between the non-atomic READ_ONCE/WRITE_ONCE +* pair. +*/ + return !pc || !READ_ONCE(ti->preempt_count); +} + +static inline bool should_resched (int preempt_offset) +{ +u64 pc =READ_ONCE (current_thread_info ()->preempt_count); +return pc == preempt_offset; +} + +#ifdef CONFIG_PREEMPT +void preempt_schedule (void ); +#define __preempt_schedule () preempt_schedule() +void preempt_schedule_notrace (void ); +#define __preempt_schedule_notrace () preempt_schedule_notrace() +#endif /* CONFIG_PREEMPT */ + +#endif /* __ASM_PREEMPT_H */ arch/arm64/include/asm/thread_info.h +12
-2@@ -42,7 +42,18 @@ struct thread_info {#ifdef CONFIG_ARM64_SW_TTBR0_PAN u64 ttbr0;/* saved TTBR0_EL1 */ #endif -int preempt_count;/* 0 => preemptable, <0 => bug */ +union { +u64 preempt_count;/* 0 => preemptible, <0 => bug */ +struct { +#ifdef CONFIG_CPU_BIG_ENDIAN +u32 need_resched; +u32 count; +#else +u32 count; +u32 need_resched; +#endif + } preempt; + }; };#define thread_saved_pc (tsk) \ --2.1.4
Quoted 10 lines · Will Deacon
Hi all,
This is version two of the patches I originally posted here:
http://lkml.kernel.org/r/[email protected]
The only change since v1 is that
__preempt_count_dec_and_test()now reloads theneed_reschedflag if it initially saw that it was set. This resolves the issue spotted by Peter, where an IRQ coming in during the decrement can cause a reschedule to be missed.
Yes, I think this one will work, so:
However, this leaves me wondering if the sequence is actually much better than what you had?
I suppose there's a win due to cache locality -- you only have to load a single line -- but I'm thinking that on pure instruction count, you're not actually winning much.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Peter,
Quoted 5 lines · Peter Zijlstra
Quoted 4 lines · Will Deacon
This is version two of the patches I originally posted here:
Quoted 8 lines
Quoted 4 lines
The only change since v1 is that
__preempt_count_dec_and_test()now reloads theneed_reschedflag if it initially saw that it was set. This resolves the issue spotted by Peter, where an IRQ coming in during the decrement can cause a reschedule to be missed.Yes, I think this one will work, so:
Thanks!
Quoted 6 lines
However, this leaves me wondering if the sequence is actually much better than what you had?
I suppose there's a win due to cache locality -- you only have to load a single line -- but I'm thinking that on pure instruction count, you're not actually winning much.
The fast path is still slightly shorter in terms of executed instructions, but you're right that the win is likely to be because everything hits in the cache or the store buffer when we're not preempting, so we should run through the code reasonably quickly and avoid the unconditional call to preempt_schedule().
Will
--->8
// Before
20: a9bf7bfd stp x29, x30, [sp, #-16]! 24: 910003fd mov x29, sp 28: d5384101 mrs x1, sp_el0 2c: b9401020 ldr w0, [x1, #16] 30: 51000400 sub w0, w0, #0x1 34: b9001020 str w0, [x1, #16] 38: 350000a0 cbnz w0, 4c <preempt_enable+0x2c> 3c: f9400020 ldr x0, [x1] 40: 721f001f tst w0, #0x2 44: 54000040 b.eq 4c <preempt_enable+0x2c> // b.none 48: 94000000 bl 0 <preempt_schedule> 4c: a8c17bfd ldp x29, x30, [sp], #16 50: d65f03c0 ret
// After
20: a9bf7bfd stp x29, x30, [sp, #-16]! 24: 910003fd mov x29, sp 28: d5384101 mrs x1, sp_el0 2c: f9400820 ldr x0, [x1, #16] 30: d1000400 sub x0, x0, #0x1 34: b9001020 str w0, [x1, #16] 38: b5000080 cbnz x0, 48 <preempt_enable+0x28> 3c: 94000000 bl 0 <preempt_schedule> 40: a8c17bfd ldp x29, x30, [sp], #16 44: d65f03c0 ret 48: f9400820 ldr x0, [x1, #16] 4c: b5ffffa0 cbnz x0, 40 <preempt_enable+0x20> 50: 94000000 bl 0 <preempt_schedule> 54: 17fffffb b 40 <will_preempt_enable+0x20>