Skip to contentSkip to conversation

x86: reimplement ___preempt_schedule*() using THUNK helpers

PATCH Superseded Oleg Nesterov 3 participants 8 messages Sep 4, 2014 – Sep 5, 2014 x86

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

cover x86: reimplement ___preempt_schedule*() using THUNK helpers

Peter, please consider this (hopefully simple) change for inclusion.

Denys, Andy, could you review?

I guess (I hope ;) Denys is going to resend his entry.S/etc rework, but this change should not conflict with his work, just preempt.S goes away.

Oleg.

 arch/x86/kernel/Makefile  |    2 --
 arch/x86/kernel/preempt.S |   25 -------------------------
 arch/x86/lib/Makefile     |    2 +-
 arch/x86/lib/thunk_32.S   |   20 ++++++++++++++++----
 arch/x86/lib/thunk_64.S   |    7 +++++++
 5 files changed, 24 insertions(+), 32 deletions(-)
author
#1 lore raw

PATCH 1/2 x86: reimplement ___preempt_schedule*() using THUNK helpers

___preempt_schedule() does SAVE_ALL/RESTORE_ALL but this is suboptimal, we do not need to save/restore the callee-saved register. And we already have arch/x86/lib/thunk_*.S which implements the similar asm wrappers, so it makes sense to redefine ___preempt_schedule() as "THUNK ..." and remove preempt.S altogether.

Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/x86/kernel/Makefile  |    2 --
 arch/x86/kernel/preempt.S |   25 -------------------------
 arch/x86/lib/thunk_32.S   |   20 ++++++++++++++++----
 arch/x86/lib/thunk_64.S   |    7 +++++++
 4 files changed, 23 insertions(+), 31 deletions(-)
 delete mode 100644 arch/x86/kernel/preempt.S
arch/x86/kernel/Makefile +0 -2
index b5ea75c..58cf60b 100644
@@ -39,8 +39,6 @@ obj-y			+= tsc.o tsc_msr.o io_delay.o rtc.o
 obj-y			+= pci-iommu_table.o
 obj-y			+= resource.o
 
-obj-$(CONFIG_PREEMPT)	+= preempt.o
-
 obj-y				+= process.o
 obj-y				+= i387.o xsave.o
 obj-y				+= ptrace.o
arch/x86/kernel/preempt.S deleted +0 -25
deleted file mode 100644
index ca7f0d5..0000000
@@ -1,25 +0,0 @@
-
-#include <linux/linkage.h>
-#include <asm/dwarf2.h>
-#include <asm/asm.h>
-#include <asm/calling.h>
-
-ENTRY(___preempt_schedule)
-	CFI_STARTPROC
-	SAVE_ALL
-	call preempt_schedule
-	RESTORE_ALL
-	ret
-	CFI_ENDPROC
-
-#ifdef CONFIG_CONTEXT_TRACKING
-
-ENTRY(___preempt_schedule_context)
-	CFI_STARTPROC
-	SAVE_ALL
-	call preempt_schedule_context
-	RESTORE_ALL
-	ret
-	CFI_ENDPROC
-
-#endif
arch/x86/lib/thunk_32.S +16 -4
index 28f85c9..7f1641a 100644
@@ -7,16 +7,19 @@
 	#include <linux/linkage.h>
 	#include <asm/asm.h>
 
-#ifdef CONFIG_TRACE_IRQFLAGS
 	/* put return address in eax (arg1) */
-	.macro thunk_ra name,func
+	.macro thunk_ra name, func, put_ret_addr_in_eax=0
 	.globl \name
 \name:
 	pushl %eax
 	pushl %ecx
 	pushl %edx
+
+	.if \put_ret_addr_in_eax
 	/* Place EIP in the arg1 */
 	movl 3*4(%esp), %eax
+	.endif
+
 	call \func
 	popl %edx
 	popl %ecx

@@ -25,6 +28,15 @@
 	_ASM_NOKPROBE(\name)
 	.endm
 
-	thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller
-	thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller
+#ifdef CONFIG_TRACE_IRQFLAGS
+	thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
+	thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
+#endif
+
+#ifdef CONFIG_PREEMPT
+	THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+	THUNK ___preempt_schedule_context, preempt_schedule_context
 #endif
+#endif
+
arch/x86/lib/thunk_64.S +7 -0
index 92d9fea..b30b5eb 100644
@@ -38,6 +38,13 @@
 	THUNK lockdep_sys_exit_thunk,lockdep_sys_exit
 #endif
 
+#ifdef CONFIG_PREEMPT
+	THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+	THUNK ___preempt_schedule_context, preempt_schedule_context
+#endif
+#endif
+
 	/* SAVE_ARGS below is used only for the .cfi directives it contains. */
 	CFI_STARTPROC
 	SAVE_ARGS
Signature · 1 line
1.5.5.1
author
#6 lore raw

PATCH 2/2 x86, lib/Makefile: remove the unnecessary "+= thunk_64.o"

In reply to Oleg Nesterov

Trivial. We have "lib-y += thunk_$(BITS).o" at the start, no need to add thunk_64.o if !CONFIG_X86_32.

Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/x86/lib/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/lib/Makefile +1 -1
index 4d4f96a..66127b9 100644
@@ -39,7 +39,7 @@ endif
 else
         obj-y += iomap_copy_64.o
         lib-y += csum-partial_64.o csum-copy_64.o csum-wrappers_64.o
-        lib-y += thunk_64.o clear_page_64.o copy_page_64.o
+        lib-y += clear_page_64.o copy_page_64.o
         lib-y += memmove_64.o memset_64.o
         lib-y += copy_user_64.o copy_user_nocache_64.o
 	lib-y += cmpxchg16b_emu.o
Signature · 1 line
1.5.5.1
#2 lore raw
In reply to Oleg Nesterov
Quoted 25 lines · Oleg Nesterov
arch/x86/lib/thunk_32.S +6 -4
-#ifdef CONFIG_TRACE_IRQFLAGS
        /* put return address in eax (arg1) */
-       .macro thunk_ra name,func
+       .macro thunk_ra name, func, put_ret_addr_in_eax=0
        .globl \name
 \name:
        pushl %eax
        pushl %ecx
        pushl %edx
+
+       .if \put_ret_addr_in_eax
        /* Place EIP in the arg1 */
        movl 3*4(%esp), %eax
+       .endif
+
        call \func
        popl %edx
        popl %ecx

@@ -25,6 +28,15 @@
        _ASM_NOKPROBE(\name)
        .endm
 
-       thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller
-       thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller
+#ifdef CONFIG_TRACE_IRQFLAGS

arch/x86/lib/thunk_32.S line 41

Quoted 10 lines
arch/x86/lib/thunk_32.S +9 -0
+       thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
+       thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
+#endif
+
+#ifdef CONFIG_PREEMPT
+       THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+       THUNK ___preempt_schedule_context, preempt_schedule_context
 #endif
+#endif

I think THUNK should be thunk_ra here;
or thunk_ra should be renamed to THUNK (like it is in thunk_64.S)

author
#3 lore raw
Quoted 2 lines · Denys Vlasenko

or thunk_ra should be renamed to THUNK (like it is in thunk_64.S)

Of course! Thanks a lot Denys.

Oleg.

author
#4 lore raw

PATCH v2 1/2 x86: reimplement ___preempt_schedule*() using THUNK helpers

In reply to Oleg Nesterov

___preempt_schedule() does SAVE_ALL/RESTORE_ALL but this is suboptimal, we do not need to save/restore the callee-saved register. And we already have arch/x86/lib/thunk_*.S which implements the similar asm wrappers, so it makes sense to redefine ___preempt_schedule() as "THUNK ..." and remove preempt.S altogether.

Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/x86/kernel/Makefile  |    2 --
 arch/x86/kernel/preempt.S |   25 -------------------------
 arch/x86/lib/thunk_32.S   |   20 ++++++++++++++++----
 arch/x86/lib/thunk_64.S   |    7 +++++++
 4 files changed, 23 insertions(+), 31 deletions(-)
 delete mode 100644 arch/x86/kernel/preempt.S
arch/x86/kernel/Makefile +0 -2
index b5ea75c..58cf60b 100644
@@ -39,8 +39,6 @@ obj-y			+= tsc.o tsc_msr.o io_delay.o rtc.o
 obj-y			+= pci-iommu_table.o
 obj-y			+= resource.o
 
-obj-$(CONFIG_PREEMPT)	+= preempt.o
-
 obj-y				+= process.o
 obj-y				+= i387.o xsave.o
 obj-y				+= ptrace.o
arch/x86/kernel/preempt.S deleted +0 -25
deleted file mode 100644
index ca7f0d5..0000000
@@ -1,25 +0,0 @@
-
-#include <linux/linkage.h>
-#include <asm/dwarf2.h>
-#include <asm/asm.h>
-#include <asm/calling.h>
-
-ENTRY(___preempt_schedule)
-	CFI_STARTPROC
-	SAVE_ALL
-	call preempt_schedule
-	RESTORE_ALL
-	ret
-	CFI_ENDPROC
-
-#ifdef CONFIG_CONTEXT_TRACKING
-
-ENTRY(___preempt_schedule_context)
-	CFI_STARTPROC
-	SAVE_ALL
-	call preempt_schedule_context
-	RESTORE_ALL
-	ret
-	CFI_ENDPROC
-
-#endif
arch/x86/lib/thunk_32.S +16 -4
index 28f85c9..e9acf5f 100644
@@ -7,16 +7,19 @@
 	#include <linux/linkage.h>
 	#include <asm/asm.h>
 
-#ifdef CONFIG_TRACE_IRQFLAGS
 	/* put return address in eax (arg1) */
-	.macro thunk_ra name,func
+	.macro THUNK name, func, put_ret_addr_in_eax=0
 	.globl \name
 \name:
 	pushl %eax
 	pushl %ecx
 	pushl %edx
+
+	.if \put_ret_addr_in_eax
 	/* Place EIP in the arg1 */
 	movl 3*4(%esp), %eax
+	.endif
+
 	call \func
 	popl %edx
 	popl %ecx

@@ -25,6 +28,15 @@
 	_ASM_NOKPROBE(\name)
 	.endm
 
-	thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller
-	thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller
+#ifdef CONFIG_TRACE_IRQFLAGS
+	THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
+	THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
+#endif
+
+#ifdef CONFIG_PREEMPT
+	THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+	THUNK ___preempt_schedule_context, preempt_schedule_context
 #endif
+#endif
+
arch/x86/lib/thunk_64.S +7 -0
index 92d9fea..b30b5eb 100644
@@ -38,6 +38,13 @@
 	THUNK lockdep_sys_exit_thunk,lockdep_sys_exit
 #endif
 
+#ifdef CONFIG_PREEMPT
+	THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+	THUNK ___preempt_schedule_context, preempt_schedule_context
+#endif
+#endif
+
 	/* SAVE_ARGS below is used only for the .cfi directives it contains. */
 	CFI_STARTPROC
 	SAVE_ARGS
Signature · 1 line
1.5.5.1
Acked-by on v1 2/2
#7 lore raw

Re: [PATCH 2/2] x86, lib/Makefile: remove the unnecessary "+= thunk_64.o"

In reply to Oleg Nesterov
Quoted 3 lines · Oleg Nesterov

Trivial. We have "lib-y += thunk_$(BITS).o" at the start, no need to add thunk_64.o if !CONFIG_X86_32.

Acked-by Andy Lutomirski <[email protected]>
Quoted 22 lines
Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/x86/lib/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/lib/Makefile +1 -2
index 4d4f96a..66127b9 100644
@@ -39,7 +39,7 @@ endif
 else
         obj-y += iomap_copy_64.o
         lib-y += csum-partial_64.o csum-copy_64.o csum-wrappers_64.o
-        lib-y += thunk_64.o clear_page_64.o copy_page_64.o
+        lib-y += clear_page_64.o copy_page_64.o
         lib-y += memmove_64.o memset_64.o
         lib-y += copy_user_64.o copy_user_nocache_64.o
        lib-y += cmpxchg16b_emu.o
--

1.5.5.1

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
Reviewed-by on v1 1/2
#5 lore raw
In reply to Oleg Nesterov
Quoted 6 lines · Oleg Nesterov

___preempt_schedule() does SAVE_ALL/RESTORE_ALL but this is suboptimal, we do not need to save/restore the callee-saved register. And we already have arch/x86/lib/thunk_*.S which implements the similar asm wrappers, so it makes sense to redefine ___preempt_schedule() as "THUNK ..." and remove preempt.S altogether.

Reviewed-by Andy Lutomirski <[email protected]>
Quoted 120 lines
Signed-off-by Oleg Nesterov <[email protected]>
---
 arch/x86/kernel/Makefile  |    2 --
 arch/x86/kernel/preempt.S |   25 -------------------------
 arch/x86/lib/thunk_32.S   |   20 ++++++++++++++++----
 arch/x86/lib/thunk_64.S   |    7 +++++++
 4 files changed, 23 insertions(+), 31 deletions(-)
 delete mode 100644 arch/x86/kernel/preempt.S
arch/x86/kernel/Makefile +0 -2
index b5ea75c..58cf60b 100644
@@ -39,8 +39,6 @@ obj-y                 += tsc.o tsc_msr.o io_delay.o rtc.o
 obj-y                  += pci-iommu_table.o
 obj-y                  += resource.o
 
-obj-$(CONFIG_PREEMPT)  += preempt.o
-
 obj-y                          += process.o
 obj-y                          += i387.o xsave.o
 obj-y                          += ptrace.o
arch/x86/kernel/preempt.S deleted +0 -25
deleted file mode 100644
index ca7f0d5..0000000
@@ -1,25 +0,0 @@
-
-#include <linux/linkage.h>
-#include <asm/dwarf2.h>
-#include <asm/asm.h>
-#include <asm/calling.h>
-
-ENTRY(___preempt_schedule)
-       CFI_STARTPROC
-       SAVE_ALL
-       call preempt_schedule
-       RESTORE_ALL
-       ret
-       CFI_ENDPROC
-
-#ifdef CONFIG_CONTEXT_TRACKING
-
-ENTRY(___preempt_schedule_context)
-       CFI_STARTPROC
-       SAVE_ALL
-       call preempt_schedule_context
-       RESTORE_ALL
-       ret
-       CFI_ENDPROC
-
-#endif
arch/x86/lib/thunk_32.S +16 -4
index 28f85c9..7f1641a 100644
@@ -7,16 +7,19 @@
        #include <linux/linkage.h>
        #include <asm/asm.h>
 
-#ifdef CONFIG_TRACE_IRQFLAGS
        /* put return address in eax (arg1) */
-       .macro thunk_ra name,func
+       .macro thunk_ra name, func, put_ret_addr_in_eax=0
        .globl \name
 \name:
        pushl %eax
        pushl %ecx
        pushl %edx
+
+       .if \put_ret_addr_in_eax
        /* Place EIP in the arg1 */
        movl 3*4(%esp), %eax
+       .endif
+
        call \func
        popl %edx
        popl %ecx

@@ -25,6 +28,15 @@
        _ASM_NOKPROBE(\name)
        .endm
 
-       thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller
-       thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller
+#ifdef CONFIG_TRACE_IRQFLAGS
+       thunk_ra trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
+       thunk_ra trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
+#endif
+
+#ifdef CONFIG_PREEMPT
+       THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+       THUNK ___preempt_schedule_context, preempt_schedule_context
 #endif
+#endif
+
arch/x86/lib/thunk_64.S +7 -1
index 92d9fea..b30b5eb 100644
@@ -38,6 +38,13 @@
        THUNK lockdep_sys_exit_thunk,lockdep_sys_exit
 #endif
 
+#ifdef CONFIG_PREEMPT
+       THUNK ___preempt_schedule, preempt_schedule
+#ifdef CONFIG_CONTEXT_TRACKING
+       THUNK ___preempt_schedule_context, preempt_schedule_context
+#endif
+#endif
+
        /* SAVE_ARGS below is used only for the .cfi directives it contains. */
        CFI_STARTPROC
        SAVE_ARGS
--

1.5.5.1

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC