* [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-17 21:19 [PATCH 0/5] x86 optimizations Peter Zijlstra
@ 2017-03-17 21:19 ` Peter Zijlstra
2017-03-21 14:03 ` Josh Poimboeuf
2017-03-17 21:19 ` [PATCH 2/5] bug: Add _ONCE logic to report_bug() Peter Zijlstra
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-17 21:19 UTC (permalink / raw)
To: mingo, tglx, hpa
Cc: linux-kernel, torvalds, arjan, bp, richard.weinberger, jpoimboe, peterz
[-- Attachment #1: peterz-x86-warn-ud2.patch --]
[-- Type: text/plain, Size: 7468 bytes --]
By using "UD0" for WARNs we remove the function call and its possible
__FILE__ and __LINE__ immediate arguments from the instruction stream.
Total image size will not change much, what we win in the instruction
stream we'll loose because of the __bug_table entries. Still, saves on
I$ footprint and the total image size does go down a bit.
text data bss dec hex filename
10702123 4530992 843776 16076891 f5505b defconfig-build/vmlinux.0
10682460 4530992 843776 16057228 f5038c defconfig-build/vmlinux.1
(um didn't seem to use GENERIC_BUG at all, so remove it)
Cc: Richard Weinberger <richard.weinberger@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/um/Kconfig.common | 5 ---
arch/x86/include/asm/bug.h | 68 ++++++++++++++++++++++++++++++-----------
arch/x86/kernel/dumpstack.c | 3 -
arch/x86/kernel/dumpstack_32.c | 12 -------
arch/x86/kernel/dumpstack_64.c | 10 ------
arch/x86/kernel/traps.c | 50 ++++++++++++++++++++++++++----
arch/x86/um/Makefile | 2 -
arch/x86/um/bug.c | 21 ------------
8 files changed, 96 insertions(+), 75 deletions(-)
--- a/arch/um/Kconfig.common
+++ b/arch/um/Kconfig.common
@@ -50,11 +50,6 @@ config GENERIC_CALIBRATE_DELAY
bool
default y
-config GENERIC_BUG
- bool
- default y
- depends on BUG
-
config HZ
int
default 100
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -1,36 +1,70 @@
#ifndef _ASM_X86_BUG_H
#define _ASM_X86_BUG_H
+#include <linux/stringify.h>
+
#define HAVE_ARCH_BUG
-#ifdef CONFIG_DEBUG_BUGVERBOSE
+/*
+ * Since some emulators terminate on UD2, we cannot use it for WARN.
+ * Since various instruction decoders disagree on the length of UD1,
+ * we cannot use it either. So use UD0 for WARN.
+ *
+ * (binutils knows about "ud1" but {en,de}codes it as 2 bytes, whereas
+ * our kernel decoder thinks it takes a ModRM byte, which seems consistent
+ * with various things like the Intel SDM instruction encoding rules)
+ */
+
+#define ASM_UD0 ".byte 0x0f, 0xff"
+#define ASM_UD1 ".byte 0x0f, 0xb9" /* + ModRM */
+#define ASM_UD2 ".byte 0x0f, 0x0b"
#ifdef CONFIG_X86_32
-# define __BUG_C0 "2:\t.long 1b, %c0\n"
+# define __BUG_REL(val) ".long " __stringify(val)
#else
-# define __BUG_C0 "2:\t.long 1b - 2b, %c0 - 2b\n"
+# define __BUG_REL(val) ".long " __stringify(val) " - 2b"
#endif
-#define BUG() \
-do { \
- asm volatile("1:\tud2\n" \
- ".pushsection __bug_table,\"a\"\n" \
- __BUG_C0 \
- "\t.word %c1, 0\n" \
- "\t.org 2b+%c2\n" \
- ".popsection" \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (sizeof(struct bug_entry))); \
- unreachable(); \
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+#define _BUG_FLAGS(ins, flags) \
+do { \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"a\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
+ "\t.word %c1" "\t# bug_entry::line\n" \
+ "\t.word %c2" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c3\n" \
+ ".popsection" \
+ : : "i" (__FILE__), "i" (__LINE__), \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
} while (0)
-#else
+#else /* !CONFIG_DEBUG_BUGVERBOSE */
+
+#define _BUG_FLAGS(ins, flags) \
+do { \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"a\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t.word %c0" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c1\n" \
+ ".popsection" \
+ : : "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
+} while (0)
+
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
#define BUG() \
do { \
- asm volatile("ud2"); \
+ _BUG_FLAGS(ASM_UD2, 0); \
unreachable(); \
} while (0)
-#endif
+
+#define __WARN_TAINT(taint) _BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
#include <asm-generic/bug.h>
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -289,9 +289,6 @@ void die(const char *str, struct pt_regs
unsigned long flags = oops_begin();
int sig = SIGSEGV;
- if (!user_mode(regs))
- report_bug(regs->ip, regs);
-
if (__die(str, regs, err))
sig = 0;
oops_end(flags, regs, sig);
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -162,15 +162,3 @@ void show_regs(struct pt_regs *regs)
}
pr_cont("\n");
}
-
-int is_valid_bugaddr(unsigned long ip)
-{
- unsigned short ud2;
-
- if (ip < PAGE_OFFSET)
- return 0;
- if (probe_kernel_address((unsigned short *)ip, ud2))
- return 0;
-
- return ud2 == 0x0b0f;
-}
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -178,13 +178,3 @@ void show_regs(struct pt_regs *regs)
}
pr_cont("\n");
}
-
-int is_valid_bugaddr(unsigned long ip)
-{
- unsigned short ud2;
-
- if (__copy_from_user(&ud2, (const void __user *) ip, sizeof(ud2)))
- return 0;
-
- return ud2 == 0x0b0f;
-}
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -169,6 +169,41 @@ void ist_end_non_atomic(void)
preempt_disable();
}
+int is_valid_bugaddr(unsigned long addr)
+{
+ unsigned short ud;
+
+#ifdef CONFIG_X86_32
+ if (addr < PAGE_OFFSET)
+ return 0;
+#else
+ if ((long)addr > 0)
+ return 0;
+#endif
+ if (probe_kernel_address((unsigned short *)addr, ud))
+ return 0;
+
+ return ud == 0x0b0f || ud == 0xff0f;
+}
+
+static int fixup_bug(struct pt_regs *regs, int trapnr)
+{
+ if (trapnr != X86_TRAP_UD)
+ return 0;
+
+ switch (report_bug(regs->ip, regs)) {
+ case BUG_TRAP_TYPE_NONE:
+ case BUG_TRAP_TYPE_BUG:
+ break;
+
+ case BUG_TRAP_TYPE_WARN:
+ regs->ip += 2;
+ return 1;
+ }
+
+ return 0;
+}
+
static nokprobe_inline int
do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
struct pt_regs *regs, long error_code)
@@ -187,12 +222,15 @@ do_trap_no_signal(struct task_struct *ts
}
if (!user_mode(regs)) {
- if (!fixup_exception(regs, trapnr)) {
- tsk->thread.error_code = error_code;
- tsk->thread.trap_nr = trapnr;
- die(str, regs, error_code);
- }
- return 0;
+ if (fixup_exception(regs, trapnr))
+ return 0;
+
+ if (fixup_bug(regs, trapnr))
+ return 0;
+
+ tsk->thread.error_code = error_code;
+ tsk->thread.trap_nr = trapnr;
+ die(str, regs, error_code);
}
return -1;
--- a/arch/x86/um/Makefile
+++ b/arch/x86/um/Makefile
@@ -8,7 +8,7 @@ else
BITS := 64
endif
-obj-y = bug.o bugs_$(BITS).o delay.o fault.o ldt.o \
+obj-y = bugs_$(BITS).o delay.o fault.o ldt.o \
ptrace_$(BITS).o ptrace_user.o setjmp_$(BITS).o signal.o \
stub_$(BITS).o stub_segv.o \
sys_call_table_$(BITS).o sysrq_$(BITS).o tls_$(BITS).o \
--- a/arch/x86/um/bug.c
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2006 Jeff Dike (jdike@addtoit.com)
- * Licensed under the GPL V2
- */
-
-#include <linux/uaccess.h>
-
-/*
- * Mostly copied from i386/x86_86 - eliminated the eip < PAGE_OFFSET because
- * that's not relevant in skas mode.
- */
-
-int is_valid_bugaddr(unsigned long eip)
-{
- unsigned short ud2;
-
- if (probe_kernel_address((unsigned short __user *)eip, ud2))
- return 0;
-
- return ud2 == 0x0b0f;
-}
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-17 21:19 ` [PATCH 1/5] x86: Implement __WARN using UD0 Peter Zijlstra
@ 2017-03-21 14:03 ` Josh Poimboeuf
2017-03-21 15:14 ` Peter Zijlstra
2017-03-22 8:47 ` Peter Zijlstra
0 siblings, 2 replies; 13+ messages in thread
From: Josh Poimboeuf @ 2017-03-21 14:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: mingo, tglx, hpa, linux-kernel, torvalds, arjan, bp, richard.weinberger
On Fri, Mar 17, 2017 at 10:19:19PM +0100, Peter Zijlstra wrote:
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -1,36 +1,70 @@
> #ifndef _ASM_X86_BUG_H
> #define _ASM_X86_BUG_H
>
> +#include <linux/stringify.h>
> +
> #define HAVE_ARCH_BUG
>
> -#ifdef CONFIG_DEBUG_BUGVERBOSE
> +/*
> + * Since some emulators terminate on UD2, we cannot use it for WARN.
> + * Since various instruction decoders disagree on the length of UD1,
> + * we cannot use it either. So use UD0 for WARN.
> + *
> + * (binutils knows about "ud1" but {en,de}codes it as 2 bytes, whereas
> + * our kernel decoder thinks it takes a ModRM byte, which seems consistent
> + * with various things like the Intel SDM instruction encoding rules)
> + */
> +
> +#define ASM_UD0 ".byte 0x0f, 0xff"
> +#define ASM_UD1 ".byte 0x0f, 0xb9" /* + ModRM */
> +#define ASM_UD2 ".byte 0x0f, 0x0b"
Thas ASM_UD1 macro isn't used anywhere.
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -169,6 +169,41 @@ void ist_end_non_atomic(void)
> preempt_disable();
> }
>
> +int is_valid_bugaddr(unsigned long addr)
> +{
> + unsigned short ud;
> +
> +#ifdef CONFIG_X86_32
> + if (addr < PAGE_OFFSET)
> + return 0;
> +#else
> + if ((long)addr > 0)
> + return 0;
> +#endif
I think comparing with TASK_SIZE would be more correct and it wouldn't
need an ifdef.
> + if (probe_kernel_address((unsigned short *)addr, ud))
> + return 0;
> +
> + return ud == 0x0b0f || ud == 0xff0f;
> +}
This code would be easier to grok if these were defines IMO.
Also, now that some of the BUG-specific functions are now also related
to WARN, they should probably be renamed to describe their new purpose,
like:
"report_bug" -> "report_bug_or_warning"
"fixup_bug" -> "fixup_bug_or_warning"
On a related note, if warn and bug are going to continue to use two
separate ud instructions for the foreseeable future, report_bug() could
be cleaned up a bit: e.g., for a ud0 instruction, it doesn't make sense
to call find_bug().
> +
> +static int fixup_bug(struct pt_regs *regs, int trapnr)
> +{
> + if (trapnr != X86_TRAP_UD)
> + return 0;
> +
> + switch (report_bug(regs->ip, regs)) {
> + case BUG_TRAP_TYPE_NONE:
> + case BUG_TRAP_TYPE_BUG:
> + break;
> +
> + case BUG_TRAP_TYPE_WARN:
> + regs->ip += 2;
> + return 1;
For self-documentation purposes, maybe use a define for the length of
the ud0 instruction?
--
Josh
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-21 14:03 ` Josh Poimboeuf
@ 2017-03-21 15:14 ` Peter Zijlstra
2017-03-21 15:17 ` Arjan van de Ven
2017-03-21 15:32 ` Josh Poimboeuf
2017-03-22 8:47 ` Peter Zijlstra
1 sibling, 2 replies; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-21 15:14 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: mingo, tglx, hpa, linux-kernel, torvalds, arjan, bp, richard.weinberger
On Tue, Mar 21, 2017 at 09:03:40AM -0500, Josh Poimboeuf wrote:
> On Fri, Mar 17, 2017 at 10:19:19PM +0100, Peter Zijlstra wrote:
> > +/*
> > + * Since some emulators terminate on UD2, we cannot use it for WARN.
> > + * Since various instruction decoders disagree on the length of UD1,
> > + * we cannot use it either. So use UD0 for WARN.
> > + *
> > + * (binutils knows about "ud1" but {en,de}codes it as 2 bytes, whereas
> > + * our kernel decoder thinks it takes a ModRM byte, which seems consistent
> > + * with various things like the Intel SDM instruction encoding rules)
> > + */
> > +
> > +#define ASM_UD0 ".byte 0x0f, 0xff"
> > +#define ASM_UD1 ".byte 0x0f, 0xb9" /* + ModRM */
> > +#define ASM_UD2 ".byte 0x0f, 0x0b"
>
> Thas ASM_UD1 macro isn't used anywhere.
I have it there for completeness sake, and documentation purposes.
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -169,6 +169,41 @@ void ist_end_non_atomic(void)
> > preempt_disable();
> > }
> >
> > +int is_valid_bugaddr(unsigned long addr)
> > +{
> > + unsigned short ud;
> > +
> > +#ifdef CONFIG_X86_32
> > + if (addr < PAGE_OFFSET)
> > + return 0;
> > +#else
> > + if ((long)addr > 0)
> > + return 0;
> > +#endif
>
> I think comparing with TASK_SIZE would be more correct and it wouldn't
> need an ifdef.
Dunno about more correct (the TIF_ADDR32 case is irrelevant here), but
yes, that would do away with the #ifdef.
> > + if (probe_kernel_address((unsigned short *)addr, ud))
> > + return 0;
> > +
> > + return ud == 0x0b0f || ud == 0xff0f;
> > +}
>
> This code would be easier to grok if these were defines IMO.
Would be nice if I could use the same defines; but I can't see how I can
make that happen :/ But sure; I suppose I can add more defines.
> Also, now that some of the BUG-specific functions are now also related
They already were, its just that x86 only now will use the WARN part of
it. I don't feel that should be part of this patch.
> to WARN, they should probably be renamed to describe their new purpose,
> like:
>
> "report_bug" -> "report_bug_or_warning"
> "fixup_bug" -> "fixup_bug_or_warning"
>
> On a related note, if warn and bug are going to continue to use two
> separate ud instructions for the foreseeable future, report_bug() could
> be cleaned up a bit: e.g., for a ud0 instruction, it doesn't make sense
> to call find_bug().
I'm sure you'll break $random arch if you go futz with that. Also, I
think you mean UD2, since that's BUG. We actually need the bug_entry for
WARNs (aka UD0).
Also, you're now optimizing the BUG() code; I don't think anybody cares
about saving a few cycles there. It shouldn't happen in the first place.
> > +static int fixup_bug(struct pt_regs *regs, int trapnr)
> > +{
> > + if (trapnr != X86_TRAP_UD)
> > + return 0;
> > +
> > + switch (report_bug(regs->ip, regs)) {
> > + case BUG_TRAP_TYPE_NONE:
> > + case BUG_TRAP_TYPE_BUG:
> > + break;
> > +
> > + case BUG_TRAP_TYPE_WARN:
> > + regs->ip += 2;
> > + return 1;
>
> For self-documentation purposes, maybe use a define for the length of
> the ud0 instruction?
Well, UD0 and UD2 really. LENGTH_UD0_OR_UD2 is a bit of a fail, name
wise.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-21 15:14 ` Peter Zijlstra
@ 2017-03-21 15:17 ` Arjan van de Ven
2017-03-21 15:32 ` Josh Poimboeuf
1 sibling, 0 replies; 13+ messages in thread
From: Arjan van de Ven @ 2017-03-21 15:17 UTC (permalink / raw)
To: Peter Zijlstra, Josh Poimboeuf
Cc: mingo, tglx, hpa, linux-kernel, torvalds, bp, richard.weinberger
On 3/21/2017 8:14 AM, Peter Zijlstra wrote:
> For self-documentation purposes, maybe use a define for the length of
> the ud0 instruction?
#define TWO 2
;-)
some things make sense as a define, others don't
(adding a comment, maybe)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-21 15:14 ` Peter Zijlstra
2017-03-21 15:17 ` Arjan van de Ven
@ 2017-03-21 15:32 ` Josh Poimboeuf
2017-03-21 15:41 ` Peter Zijlstra
1 sibling, 1 reply; 13+ messages in thread
From: Josh Poimboeuf @ 2017-03-21 15:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: mingo, tglx, hpa, linux-kernel, torvalds, arjan, bp, richard.weinberger
On Tue, Mar 21, 2017 at 04:14:46PM +0100, Peter Zijlstra wrote:
> > to WARN, they should probably be renamed to describe their new purpose,
> > like:
> >
> > "report_bug" -> "report_bug_or_warning"
> > "fixup_bug" -> "fixup_bug_or_warning"
> >
> > On a related note, if warn and bug are going to continue to use two
> > separate ud instructions for the foreseeable future, report_bug() could
> > be cleaned up a bit: e.g., for a ud0 instruction, it doesn't make sense
> > to call find_bug().
>
> I'm sure you'll break $random arch if you go futz with that. Also, I
> think you mean UD2, since that's BUG. We actually need the bug_entry for
> WARNs (aka UD0).
>
> Also, you're now optimizing the BUG() code; I don't think anybody cares
> about saving a few cycles there. It shouldn't happen in the first place.
My thinking was to make report_bug() a little less obtuse, but yeah,
that would break other arches, so never mind...
> > > +static int fixup_bug(struct pt_regs *regs, int trapnr)
> > > +{
> > > + if (trapnr != X86_TRAP_UD)
> > > + return 0;
> > > +
> > > + switch (report_bug(regs->ip, regs)) {
> > > + case BUG_TRAP_TYPE_NONE:
> > > + case BUG_TRAP_TYPE_BUG:
> > > + break;
> > > +
> > > + case BUG_TRAP_TYPE_WARN:
> > > + regs->ip += 2;
> > > + return 1;
> >
> > For self-documentation purposes, maybe use a define for the length of
> > the ud0 instruction?
>
> Well, UD0 and UD2 really. LENGTH_UD0_OR_UD2 is a bit of a fail, name
> wise.
Why UD2? Warnings are UD0-only, no? What about UD0_LEN? Or at least a
comment would be helpful I think.
--
Josh
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-21 15:32 ` Josh Poimboeuf
@ 2017-03-21 15:41 ` Peter Zijlstra
0 siblings, 0 replies; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-21 15:41 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: mingo, tglx, hpa, linux-kernel, torvalds, arjan, bp, richard.weinberger
On Tue, Mar 21, 2017 at 10:32:52AM -0500, Josh Poimboeuf wrote:
> On Tue, Mar 21, 2017 at 04:14:46PM +0100, Peter Zijlstra wrote:
> > > > +static int fixup_bug(struct pt_regs *regs, int trapnr)
> > > > +{
> > > > + if (trapnr != X86_TRAP_UD)
> > > > + return 0;
> > > > +
> > > > + switch (report_bug(regs->ip, regs)) {
> > > > + case BUG_TRAP_TYPE_NONE:
> > > > + case BUG_TRAP_TYPE_BUG:
> > > > + break;
> > > > +
> > > > + case BUG_TRAP_TYPE_WARN:
> > > > + regs->ip += 2;
> > > > + return 1;
> > >
> > > For self-documentation purposes, maybe use a define for the length of
> > > the ud0 instruction?
> >
> > Well, UD0 and UD2 really. LENGTH_UD0_OR_UD2 is a bit of a fail, name
> > wise.
>
> Why UD2? Warnings are UD0-only, no? What about UD0_LEN? Or at least a
> comment would be helpful I think.
Oh, right, we only resume on WARN,.. Doh.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-21 14:03 ` Josh Poimboeuf
2017-03-21 15:14 ` Peter Zijlstra
@ 2017-03-22 8:47 ` Peter Zijlstra
2017-03-22 14:18 ` Josh Poimboeuf
1 sibling, 1 reply; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-22 8:47 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: mingo, tglx, hpa, linux-kernel, torvalds, arjan, bp, richard.weinberger
A little like so then.
---
Subject: x86: Implement __WARN using UD0
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 2 14:43:51 CET 2017
By using "UD0" for WARNs we remove the function call and its possible
__FILE__ and __LINE__ immediate arguments from the instruction stream.
Total image size will not change much, what we win in the instruction
stream we'll loose because of the __bug_table entries. Still, saves on
I$ footprint and the total image size does go down a bit.
text data bss dec hex filename
10702123 4530992 843776 16076891 f5505b defconfig-build/vmlinux.0
10682460 4530992 843776 16057228 f5038c defconfig-build/vmlinux.1
(um didn't seem to use GENERIC_BUG at all, so remove it)
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Richard Weinberger <richard.weinberger@gmail.com>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/um/Kconfig.common | 5 --
arch/x86/include/asm/bug.h | 73 +++++++++++++++++++++++++++++++----------
arch/x86/kernel/dumpstack.c | 3 -
arch/x86/kernel/dumpstack_32.c | 12 ------
arch/x86/kernel/dumpstack_64.c | 10 -----
arch/x86/kernel/traps.c | 46 ++++++++++++++++++++++---
arch/x86/um/Makefile | 2 -
arch/x86/um/bug.c | 21 -----------
8 files changed, 97 insertions(+), 75 deletions(-)
--- a/arch/um/Kconfig.common
+++ b/arch/um/Kconfig.common
@@ -50,11 +50,6 @@ config GENERIC_CALIBRATE_DELAY
bool
default y
-config GENERIC_BUG
- bool
- default y
- depends on BUG
-
config HZ
int
default 100
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -1,36 +1,75 @@
#ifndef _ASM_X86_BUG_H
#define _ASM_X86_BUG_H
+#include <linux/stringify.h>
+
#define HAVE_ARCH_BUG
-#ifdef CONFIG_DEBUG_BUGVERBOSE
+/*
+ * Since some emulators terminate on UD2, we cannot use it for WARN.
+ * Since various instruction decoders disagree on the length of UD1,
+ * we cannot use it either. So use UD0 for WARN.
+ *
+ * (binutils knows about "ud1" but {en,de}codes it as 2 bytes, whereas
+ * our kernel decoder thinks it takes a ModRM byte, which seems consistent
+ * with various things like the Intel SDM instruction encoding rules)
+ */
+
+#define ASM_UD0 ".byte 0x0f, 0xff"
+#define ASM_UD1 ".byte 0x0f, 0xb9" /* + ModRM */
+#define ASM_UD2 ".byte 0x0f, 0x0b"
+
+#define INSN_UD0 0xff0f
+#define INSN_UD2 0x0b0f
+
+#define LEN_UD0 2
#ifdef CONFIG_X86_32
-# define __BUG_C0 "2:\t.long 1b, %c0\n"
+# define __BUG_REL(val) ".long " __stringify(val)
#else
-# define __BUG_C0 "2:\t.long 1b - 2b, %c0 - 2b\n"
+# define __BUG_REL(val) ".long " __stringify(val) " - 2b"
#endif
-#define BUG() \
-do { \
- asm volatile("1:\tud2\n" \
- ".pushsection __bug_table,\"a\"\n" \
- __BUG_C0 \
- "\t.word %c1, 0\n" \
- "\t.org 2b+%c2\n" \
- ".popsection" \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (sizeof(struct bug_entry))); \
- unreachable(); \
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+#define _BUG_FLAGS(ins, flags) \
+do { \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"a\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
+ "\t.word %c1" "\t# bug_entry::line\n" \
+ "\t.word %c2" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c3\n" \
+ ".popsection" \
+ : : "i" (__FILE__), "i" (__LINE__), \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
} while (0)
-#else
+#else /* !CONFIG_DEBUG_BUGVERBOSE */
+
+#define _BUG_FLAGS(ins, flags) \
+do { \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"a\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t.word %c0" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c1\n" \
+ ".popsection" \
+ : : "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
+} while (0)
+
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
#define BUG() \
do { \
- asm volatile("ud2"); \
+ _BUG_FLAGS(ASM_UD2, 0); \
unreachable(); \
} while (0)
-#endif
+
+#define __WARN_TAINT(taint) _BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
#include <asm-generic/bug.h>
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -289,9 +289,6 @@ void die(const char *str, struct pt_regs
unsigned long flags = oops_begin();
int sig = SIGSEGV;
- if (!user_mode(regs))
- report_bug(regs->ip, regs);
-
if (__die(str, regs, err))
sig = 0;
oops_end(flags, regs, sig);
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -162,15 +162,3 @@ void show_regs(struct pt_regs *regs)
}
pr_cont("\n");
}
-
-int is_valid_bugaddr(unsigned long ip)
-{
- unsigned short ud2;
-
- if (ip < PAGE_OFFSET)
- return 0;
- if (probe_kernel_address((unsigned short *)ip, ud2))
- return 0;
-
- return ud2 == 0x0b0f;
-}
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -178,13 +178,3 @@ void show_regs(struct pt_regs *regs)
}
pr_cont("\n");
}
-
-int is_valid_bugaddr(unsigned long ip)
-{
- unsigned short ud2;
-
- if (__copy_from_user(&ud2, (const void __user *) ip, sizeof(ud2)))
- return 0;
-
- return ud2 == 0x0b0f;
-}
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -169,6 +169,37 @@ void ist_end_non_atomic(void)
preempt_disable();
}
+int is_valid_bugaddr(unsigned long addr)
+{
+ unsigned short ud;
+
+ if (addr < TASK_SIZE_MAX)
+ return 0;
+
+ if (probe_kernel_address((unsigned short *)addr, ud))
+ return 0;
+
+ return ud == INSN_UD0 || ud == INSN_UD2;
+}
+
+static int fixup_bug(struct pt_regs *regs, int trapnr)
+{
+ if (trapnr != X86_TRAP_UD)
+ return 0;
+
+ switch (report_bug(regs->ip, regs)) {
+ case BUG_TRAP_TYPE_NONE:
+ case BUG_TRAP_TYPE_BUG:
+ break;
+
+ case BUG_TRAP_TYPE_WARN:
+ regs->ip += LEN_UD0;
+ return 1;
+ }
+
+ return 0;
+}
+
static nokprobe_inline int
do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
struct pt_regs *regs, long error_code)
@@ -187,12 +218,15 @@ do_trap_no_signal(struct task_struct *ts
}
if (!user_mode(regs)) {
- if (!fixup_exception(regs, trapnr)) {
- tsk->thread.error_code = error_code;
- tsk->thread.trap_nr = trapnr;
- die(str, regs, error_code);
- }
- return 0;
+ if (fixup_exception(regs, trapnr))
+ return 0;
+
+ if (fixup_bug(regs, trapnr))
+ return 0;
+
+ tsk->thread.error_code = error_code;
+ tsk->thread.trap_nr = trapnr;
+ die(str, regs, error_code);
}
return -1;
--- a/arch/x86/um/Makefile
+++ b/arch/x86/um/Makefile
@@ -8,7 +8,7 @@ else
BITS := 64
endif
-obj-y = bug.o bugs_$(BITS).o delay.o fault.o ldt.o \
+obj-y = bugs_$(BITS).o delay.o fault.o ldt.o \
ptrace_$(BITS).o ptrace_user.o setjmp_$(BITS).o signal.o \
stub_$(BITS).o stub_segv.o \
sys_call_table_$(BITS).o sysrq_$(BITS).o tls_$(BITS).o \
--- a/arch/x86/um/bug.c
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2006 Jeff Dike (jdike@addtoit.com)
- * Licensed under the GPL V2
- */
-
-#include <linux/uaccess.h>
-
-/*
- * Mostly copied from i386/x86_86 - eliminated the eip < PAGE_OFFSET because
- * that's not relevant in skas mode.
- */
-
-int is_valid_bugaddr(unsigned long eip)
-{
- unsigned short ud2;
-
- if (probe_kernel_address((unsigned short __user *)eip, ud2))
- return 0;
-
- return ud2 == 0x0b0f;
-}
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] x86: Implement __WARN using UD0
2017-03-22 8:47 ` Peter Zijlstra
@ 2017-03-22 14:18 ` Josh Poimboeuf
0 siblings, 0 replies; 13+ messages in thread
From: Josh Poimboeuf @ 2017-03-22 14:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: mingo, tglx, hpa, linux-kernel, torvalds, arjan, bp, richard.weinberger
On Wed, Mar 22, 2017 at 09:47:06AM +0100, Peter Zijlstra wrote:
>
> A little like so then.
>
> ---
> Subject: x86: Implement __WARN using UD0
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Feb 2 14:43:51 CET 2017
>
> By using "UD0" for WARNs we remove the function call and its possible
> __FILE__ and __LINE__ immediate arguments from the instruction stream.
>
> Total image size will not change much, what we win in the instruction
> stream we'll loose because of the __bug_table entries. Still, saves on
> I$ footprint and the total image size does go down a bit.
>
> text data bss dec hex filename
> 10702123 4530992 843776 16076891 f5505b defconfig-build/vmlinux.0
> 10682460 4530992 843776 16057228 f5038c defconfig-build/vmlinux.1
>
> (um didn't seem to use GENERIC_BUG at all, so remove it)
>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Richard Weinberger <richard.weinberger@gmail.com>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Looks good, thanks!
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
--
Josh
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] bug: Add _ONCE logic to report_bug()
2017-03-17 21:19 [PATCH 0/5] x86 optimizations Peter Zijlstra
2017-03-17 21:19 ` [PATCH 1/5] x86: Implement __WARN using UD0 Peter Zijlstra
@ 2017-03-17 21:19 ` Peter Zijlstra
2017-03-17 21:19 ` [PATCH 3/5] atomic: Introduce atomic_try_cmpxchg() Peter Zijlstra
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-17 21:19 UTC (permalink / raw)
To: mingo, tglx, hpa
Cc: linux-kernel, torvalds, arjan, bp, richard.weinberger, jpoimboe, peterz
[-- Attachment #1: peterz-warn-once.patch --]
[-- Type: text/plain, Size: 8867 bytes --]
Josh suggested moving the _ONCE logic inside the trap handler, using a
bit in the bug_entry::flags field, avoiding the need for the extra
variable.
Sadly this only works for WARN_ON_ONCE(), since the others have
printk() statements prior to triggering the trap.
Still, this saves a fair amount of text and data:
text data bss dec hex filename
10682460 4530992 843776 16057228 f5038c defconfig-build/vmlinux.1
10665111 4530096 843776 16038983 f4bc47 defconfig-build/vmlinux.2
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm64/include/asm/bug.h | 2 +-
arch/parisc/include/asm/bug.h | 8 ++++----
arch/powerpc/include/asm/bug.h | 4 ++--
arch/s390/include/asm/bug.h | 4 ++--
arch/sh/include/asm/bug.h | 4 ++--
arch/x86/include/asm/bug.h | 2 +-
include/asm-generic/bug.h | 18 +++++++++++++++++-
include/asm-generic/vmlinux.lds.h | 3 +--
include/linux/bug.h | 2 +-
lib/bug.c | 28 ++++++++++++++++++++--------
10 files changed, 51 insertions(+), 24 deletions(-)
--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -55,7 +55,7 @@ _BUGVERBOSE_LOCATION(__FILE__, __LINE__)
unreachable(); \
} while (0)
-#define __WARN_TAINT(taint) _BUG_FLAGS(BUGFLAG_TAINT(taint))
+#define __WARN_FLAGS(flags) _BUG_FLAGS(BUGFLAG_WARNING|(flags))
#endif /* ! CONFIG_GENERIC_BUG */
--- a/arch/parisc/include/asm/bug.h
+++ b/arch/parisc/include/asm/bug.h
@@ -46,7 +46,7 @@
#endif
#ifdef CONFIG_DEBUG_BUGVERBOSE
-#define __WARN_TAINT(taint) \
+#define __WARN_FLAGS(flags) \
do { \
asm volatile("\n" \
"1:\t" PARISC_BUG_BREAK_ASM "\n" \
@@ -56,11 +56,11 @@
"\t.org 2b+%c3\n" \
"\t.popsection" \
: : "i" (__FILE__), "i" (__LINE__), \
- "i" (BUGFLAG_TAINT(taint)), \
+ "i" (BUGFLAG_WARNING|(flags)), \
"i" (sizeof(struct bug_entry)) ); \
} while(0)
#else
-#define __WARN_TAINT(taint) \
+#define __WARN_FLAGS(flags) \
do { \
asm volatile("\n" \
"1:\t" PARISC_BUG_BREAK_ASM "\n" \
@@ -69,7 +69,7 @@
"\t.short %c0\n" \
"\t.org 2b+%c1\n" \
"\t.popsection" \
- : : "i" (BUGFLAG_TAINT(taint)), \
+ : : "i" (BUGFLAG_WARNING|(flags)), \
"i" (sizeof(struct bug_entry)) ); \
} while(0)
#endif
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -85,12 +85,12 @@
} \
} while (0)
-#define __WARN_TAINT(taint) do { \
+#define __WARN_FLAGS(flags) do { \
__asm__ __volatile__( \
"1: twi 31,0,0\n" \
_EMIT_BUG_ENTRY \
: : "i" (__FILE__), "i" (__LINE__), \
- "i" (BUGFLAG_TAINT(taint)), \
+ "i" (BUGFLAG_WARNING|(flags)), \
"i" (sizeof(struct bug_entry))); \
} while (0)
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -46,8 +46,8 @@
unreachable(); \
} while (0)
-#define __WARN_TAINT(taint) do { \
- __EMIT_BUG(BUGFLAG_TAINT(taint)); \
+#define __WARN_FLAGS(flags) do { \
+ __EMIT_BUG(BUGFLAG_WARNING|(flags)); \
} while (0)
#define WARN_ON(x) ({ \
--- a/arch/sh/include/asm/bug.h
+++ b/arch/sh/include/asm/bug.h
@@ -50,7 +50,7 @@ do { \
"i" (sizeof(struct bug_entry))); \
} while (0)
-#define __WARN_TAINT(taint) \
+#define __WARN_FLAGS(flags) \
do { \
__asm__ __volatile__ ( \
"1:\t.short %O0\n" \
@@ -59,7 +59,7 @@ do { \
: "n" (TRAPA_BUG_OPCODE), \
"i" (__FILE__), \
"i" (__LINE__), \
- "i" (BUGFLAG_TAINT(taint)), \
+ "i" (BUGFLAG_WARNING|(flags)), \
"i" (sizeof(struct bug_entry))); \
} while (0)
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -64,7 +64,7 @@ do { \
unreachable(); \
} while (0)
-#define __WARN_TAINT(taint) _BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
+#define __WARN_FLAGS(flags) _BUG_FLAGS(ASM_UD0, BUGFLAG_WARNING|(flags))
#include <asm-generic/bug.h>
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -5,6 +5,8 @@
#ifdef CONFIG_GENERIC_BUG
#define BUGFLAG_WARNING (1 << 0)
+#define BUGFLAG_ONCE (1 << 1)
+#define BUGFLAG_DONE (1 << 2)
#define BUGFLAG_TAINT(taint) (BUGFLAG_WARNING | ((taint) << 8))
#define BUG_GET_TAINT(bug) ((bug)->flags >> 8)
#endif
@@ -55,6 +57,18 @@ struct bug_entry {
#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
#endif
+#ifdef __WARN_FLAGS
+#define __WARN_TAINT(taint) __WARN_FLAGS(BUGFLAG_TAINT(taint))
+#define __WARN_ONCE_TAINT(taint) __WARN_FLAGS(BUGFLAG_ONCE|BUGFLAG_TAINT(taint))
+
+#define WARN_ON_ONCE(condition) ({ \
+ int __ret_warn_on = !!(condition); \
+ if (unlikely(__ret_warn_on)) \
+ __WARN_ONCE_TAINT(TAINT_WARN); \
+ unlikely(__ret_warn_on); \
+})
+#endif
+
/*
* WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
* significant issues that need prompt attention if they should ever
@@ -97,7 +111,7 @@ void __warn(const char *file, int line,
#endif
#ifndef WARN
-#define WARN(condition, format...) ({ \
+#define WARN(condition, format...) ({ \
int __ret_warn_on = !!(condition); \
if (unlikely(__ret_warn_on)) \
__WARN_printf(format); \
@@ -112,6 +126,7 @@ void __warn(const char *file, int line,
unlikely(__ret_warn_on); \
})
+#ifndef WARN_ON_ONCE
#define WARN_ON_ONCE(condition) ({ \
static bool __section(.data.unlikely) __warned; \
int __ret_warn_once = !!(condition); \
@@ -122,6 +137,7 @@ void __warn(const char *file, int line,
} \
unlikely(__ret_warn_once); \
})
+#endif
#define WARN_ONCE(condition, format...) ({ \
static bool __section(.data.unlikely) __warned; \
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -286,8 +286,6 @@
*(.rodata1) \
} \
\
- BUG_TABLE \
- \
/* PCI quirks */ \
.pci_fixup : AT(ADDR(.pci_fixup) - LOAD_OFFSET) { \
VMLINUX_SYMBOL(__start_pci_fixups_early) = .; \
@@ -855,6 +853,7 @@
READ_MOSTLY_DATA(cacheline) \
DATA_DATA \
CONSTRUCTORS \
+ BUG_TABLE \
}
#define INIT_TEXT_SECTION(inittext_align) \
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -105,7 +105,7 @@ static inline int is_warning_bug(const s
return bug->flags & BUGFLAG_WARNING;
}
-const struct bug_entry *find_bug(unsigned long bugaddr);
+struct bug_entry *find_bug(unsigned long bugaddr);
enum bug_trap_type report_bug(unsigned long bug_addr, struct pt_regs *regs);
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -47,7 +47,7 @@
#include <linux/sched.h>
#include <linux/rculist.h>
-extern const struct bug_entry __start___bug_table[], __stop___bug_table[];
+extern struct bug_entry __start___bug_table[], __stop___bug_table[];
static inline unsigned long bug_addr(const struct bug_entry *bug)
{
@@ -62,10 +62,10 @@ static inline unsigned long bug_addr(con
/* Updates are protected by module mutex */
static LIST_HEAD(module_bug_list);
-static const struct bug_entry *module_find_bug(unsigned long bugaddr)
+static struct bug_entry *module_find_bug(unsigned long bugaddr)
{
struct module *mod;
- const struct bug_entry *bug = NULL;
+ struct bug_entry *bug = NULL;
rcu_read_lock_sched();
list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
@@ -122,15 +122,15 @@ void module_bug_cleanup(struct module *m
#else
-static inline const struct bug_entry *module_find_bug(unsigned long bugaddr)
+static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
{
return NULL;
}
#endif
-const struct bug_entry *find_bug(unsigned long bugaddr)
+struct bug_entry *find_bug(unsigned long bugaddr)
{
- const struct bug_entry *bug;
+ struct bug_entry *bug;
for (bug = __start___bug_table; bug < __stop___bug_table; ++bug)
if (bugaddr == bug_addr(bug))
@@ -141,9 +141,9 @@ const struct bug_entry *find_bug(unsigne
enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
{
- const struct bug_entry *bug;
+ struct bug_entry *bug;
const char *file;
- unsigned line, warning;
+ unsigned line, warning, once, done;
if (!is_valid_bugaddr(bugaddr))
return BUG_TRAP_TYPE_NONE;
@@ -164,6 +164,18 @@ enum bug_trap_type report_bug(unsigned l
line = bug->line;
#endif
warning = (bug->flags & BUGFLAG_WARNING) != 0;
+ once = (bug->flags & BUGFLAG_ONCE) != 0;
+ done = (bug->flags & BUGFLAG_DONE) != 0;
+
+ if (warning && once) {
+ if (done)
+ return BUG_TRAP_TYPE_WARN;
+
+ /*
+ * Since this is the only store, concurrency is not an issue.
+ */
+ bug->flags |= BUGFLAG_DONE;
+ }
}
if (warning) {
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 3/5] atomic: Introduce atomic_try_cmpxchg()
2017-03-17 21:19 [PATCH 0/5] x86 optimizations Peter Zijlstra
2017-03-17 21:19 ` [PATCH 1/5] x86: Implement __WARN using UD0 Peter Zijlstra
2017-03-17 21:19 ` [PATCH 2/5] bug: Add _ONCE logic to report_bug() Peter Zijlstra
@ 2017-03-17 21:19 ` Peter Zijlstra
2017-03-17 21:19 ` [PATCH 4/5] refcount: Use atomic_try_cmpxchg() Peter Zijlstra
2017-03-17 21:19 ` [PATCH 5/5] x86,atomic: Use atomic_try_cmpxchg Peter Zijlstra
4 siblings, 0 replies; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-17 21:19 UTC (permalink / raw)
To: mingo, tglx, hpa
Cc: linux-kernel, torvalds, arjan, bp, richard.weinberger, jpoimboe, peterz
[-- Attachment #1: peterz-try_atomic.patch --]
[-- Type: text/plain, Size: 6325 bytes --]
Add a new cmpxchg interface:
bool try_cmpxchg(u{8,16,32,64} *ptr, u{8,16,32,64} *val, u{8,16,32,64} new);
Where the boolean returns the result of the compare; and thus if the
exchange happened; and in case of failure, the new value of *ptr is
returned in *val.
This allows simplification/improvement of loops like:
for (;;) {
new = val $op $imm;
old = cmpxchg(ptr, val, new);
if (old == val)
break;
val = old;
}
into:
do {
new = val $op $imm;
} while (!try_cmpxchg(ptr, &val, new));
while also generating better code (GCC6 and onwards).
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/include/asm/atomic.h | 6 +++
arch/x86/include/asm/atomic64_64.h | 6 +++
arch/x86/include/asm/cmpxchg.h | 69 +++++++++++++++++++++++++++++++++++++
include/linux/atomic.h | 42 ++++++++++++++++++++++
4 files changed, 123 insertions(+)
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -186,6 +186,12 @@ static __always_inline int atomic_cmpxch
return cmpxchg(&v->counter, old, new);
}
+#define atomic_try_cmpxchg atomic_try_cmpxchg
+static __always_inline bool atomic_try_cmpxchg(atomic_t *v, int *old, int new)
+{
+ return try_cmpxchg(&v->counter, old, new);
+}
+
static inline int atomic_xchg(atomic_t *v, int new)
{
return xchg(&v->counter, new);
--- a/arch/x86/include/asm/atomic64_64.h
+++ b/arch/x86/include/asm/atomic64_64.h
@@ -176,6 +176,12 @@ static inline long atomic64_cmpxchg(atom
return cmpxchg(&v->counter, old, new);
}
+#define atomic64_try_cmpxchg atomic64_try_cmpxchg
+static __always_inline bool atomic64_try_cmpxchg(atomic64_t *v, long *old, long new)
+{
+ return try_cmpxchg(&v->counter, old, new);
+}
+
static inline long atomic64_xchg(atomic64_t *v, long new)
{
return xchg(&v->counter, new);
--- a/arch/x86/include/asm/cmpxchg.h
+++ b/arch/x86/include/asm/cmpxchg.h
@@ -153,6 +153,75 @@ extern void __add_wrong_size(void)
#define cmpxchg_local(ptr, old, new) \
__cmpxchg_local(ptr, old, new, sizeof(*(ptr)))
+
+#define __raw_try_cmpxchg(_ptr, _pold, _new, size, lock) \
+({ \
+ bool success; \
+ __typeof__(_ptr) _old = (_pold); \
+ __typeof__(*(_ptr)) __old = *_old; \
+ __typeof__(*(_ptr)) __new = (_new); \
+ switch (size) { \
+ case __X86_CASE_B: \
+ { \
+ volatile u8 *__ptr = (volatile u8 *)(_ptr); \
+ asm volatile(lock "cmpxchgb %[new], %[ptr]" \
+ CC_SET(z) \
+ : CC_OUT(z) (success), \
+ [ptr] "+m" (*__ptr), \
+ [old] "+a" (__old) \
+ : [new] "q" (__new) \
+ : "memory"); \
+ break; \
+ } \
+ case __X86_CASE_W: \
+ { \
+ volatile u16 *__ptr = (volatile u16 *)(_ptr); \
+ asm volatile(lock "cmpxchgw %[new], %[ptr]" \
+ CC_SET(z) \
+ : CC_OUT(z) (success), \
+ [ptr] "+m" (*__ptr), \
+ [old] "+a" (__old) \
+ : [new] "r" (__new) \
+ : "memory"); \
+ break; \
+ } \
+ case __X86_CASE_L: \
+ { \
+ volatile u32 *__ptr = (volatile u32 *)(_ptr); \
+ asm volatile(lock "cmpxchgl %[new], %[ptr]" \
+ CC_SET(z) \
+ : CC_OUT(z) (success), \
+ [ptr] "+m" (*__ptr), \
+ [old] "+a" (__old) \
+ : [new] "r" (__new) \
+ : "memory"); \
+ break; \
+ } \
+ case __X86_CASE_Q: \
+ { \
+ volatile u64 *__ptr = (volatile u64 *)(_ptr); \
+ asm volatile(lock "cmpxchgq %[new], %[ptr]" \
+ CC_SET(z) \
+ : CC_OUT(z) (success), \
+ [ptr] "+m" (*__ptr), \
+ [old] "+a" (__old) \
+ : [new] "r" (__new) \
+ : "memory"); \
+ break; \
+ } \
+ default: \
+ __cmpxchg_wrong_size(); \
+ } \
+ *_old = __old; \
+ success; \
+})
+
+#define __try_cmpxchg(ptr, pold, new, size) \
+ __raw_try_cmpxchg((ptr), (pold), (new), (size), LOCK_PREFIX)
+
+#define try_cmpxchg(ptr, pold, new) \
+ __try_cmpxchg((ptr), (pold), (new), sizeof(*(ptr)))
+
/*
* xadd() adds "inc" to "*ptr" and atomically returns the previous
* value of "*ptr".
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -423,6 +423,27 @@
#endif
#endif /* atomic_cmpxchg_relaxed */
+#ifndef atomic_try_cmpxchg
+
+#define __atomic_try_cmpxchg(type, _p, _po, _n) \
+({ \
+ typeof(_po) __po = (_po); \
+ typeof(*(_po)) __o = *__po; \
+ *__po = atomic_cmpxchg##type((_p), __o, (_n)); \
+ (*__po == __o); \
+})
+
+#define atomic_try_cmpxchg(_p, _po, _n) __atomic_try_cmpxchg(, _p, _po, _n)
+#define atomic_try_cmpxchg_relaxed(_p, _po, _n) __atomic_try_cmpxchg(_relaxed, _p, _po, _n)
+#define atomic_try_cmpxchg_acquire(_p, _po, _n) __atomic_try_cmpxchg(_acquire, _p, _po, _n)
+#define atomic_try_cmpxchg_release(_p, _po, _n) __atomic_try_cmpxchg(_release, _p, _po, _n)
+
+#else /* atomic_try_cmpxchg */
+#define atomic_try_cmpxchg_relaxed atomic_try_cmpxchg
+#define atomic_try_cmpxchg_acquire atomic_try_cmpxchg
+#define atomic_try_cmpxchg_release atomic_try_cmpxchg
+#endif /* atomic_try_cmpxchg */
+
/* cmpxchg_relaxed */
#ifndef cmpxchg_relaxed
#define cmpxchg_relaxed cmpxchg
@@ -996,6 +1017,27 @@ static inline int atomic_dec_if_positive
#endif
#endif /* atomic64_cmpxchg_relaxed */
+#ifndef atomic64_try_cmpxchg
+
+#define __atomic64_try_cmpxchg(type, _p, _po, _n) \
+({ \
+ typeof(_po) __po = (_po); \
+ typeof(*(_po)) __o = *__po; \
+ *__po = atomic64_cmpxchg##type((_p), __o, (_n)); \
+ (*__po == __o); \
+})
+
+#define atomic64_try_cmpxchg(_p, _po, _n) __atomic64_try_cmpxchg(, _p, _po, _n)
+#define atomic64_try_cmpxchg_relaxed(_p, _po, _n) __atomic64_try_cmpxchg(_relaxed, _p, _po, _n)
+#define atomic64_try_cmpxchg_acquire(_p, _po, _n) __atomic64_try_cmpxchg(_acquire, _p, _po, _n)
+#define atomic64_try_cmpxchg_release(_p, _po, _n) __atomic64_try_cmpxchg(_release, _p, _po, _n)
+
+#else /* atomic64_try_cmpxchg */
+#define atomic64_try_cmpxchg_relaxed atomic64_try_cmpxchg
+#define atomic64_try_cmpxchg_acquire atomic64_try_cmpxchg
+#define atomic64_try_cmpxchg_release atomic64_try_cmpxchg
+#endif /* atomic64_try_cmpxchg */
+
#ifndef atomic64_andnot
static inline void atomic64_andnot(long long i, atomic64_t *v)
{
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 4/5] refcount: Use atomic_try_cmpxchg()
2017-03-17 21:19 [PATCH 0/5] x86 optimizations Peter Zijlstra
` (2 preceding siblings ...)
2017-03-17 21:19 ` [PATCH 3/5] atomic: Introduce atomic_try_cmpxchg() Peter Zijlstra
@ 2017-03-17 21:19 ` Peter Zijlstra
2017-03-17 21:19 ` [PATCH 5/5] x86,atomic: Use atomic_try_cmpxchg Peter Zijlstra
4 siblings, 0 replies; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-17 21:19 UTC (permalink / raw)
To: mingo, tglx, hpa
Cc: linux-kernel, torvalds, arjan, bp, richard.weinberger, jpoimboe, peterz
[-- Attachment #1: peterz-ref-6.patch --]
[-- Type: text/plain, Size: 3081 bytes --]
Generates better code (GCC-6.2.1):
text data bss dec hex filename
1576 7 0 1583 62f defconfig-build/lib/refcount.o.pre
1488 7 0 1495 5d7 defconfig-build/lib/refcount.o.post
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
lib/refcount.c | 47 +++++++++++++++--------------------------------
1 file changed, 15 insertions(+), 32 deletions(-)
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -57,9 +57,9 @@
*/
bool refcount_add_not_zero(unsigned int i, refcount_t *r)
{
- unsigned int old, new, val = atomic_read(&r->refs);
+ unsigned int new, val = atomic_read(&r->refs);
- for (;;) {
+ do {
if (!val)
return false;
@@ -69,12 +69,8 @@ bool refcount_add_not_zero(unsigned int
new = val + i;
if (new < val)
new = UINT_MAX;
- old = atomic_cmpxchg_relaxed(&r->refs, val, new);
- if (old == val)
- break;
- val = old;
- }
+ } while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new));
WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
@@ -118,9 +114,9 @@ EXPORT_SYMBOL_GPL(refcount_add);
*/
bool refcount_inc_not_zero(refcount_t *r)
{
- unsigned int old, new, val = atomic_read(&r->refs);
+ unsigned int new, val = atomic_read(&r->refs);
- for (;;) {
+ do {
new = val + 1;
if (!val)
@@ -129,12 +125,7 @@ bool refcount_inc_not_zero(refcount_t *r
if (unlikely(!new))
return true;
- old = atomic_cmpxchg_relaxed(&r->refs, val, new);
- if (old == val)
- break;
-
- val = old;
- }
+ } while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new));
WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
@@ -182,9 +173,9 @@ EXPORT_SYMBOL_GPL(refcount_inc);
*/
bool refcount_sub_and_test(unsigned int i, refcount_t *r)
{
- unsigned int old, new, val = atomic_read(&r->refs);
+ unsigned int new, val = atomic_read(&r->refs);
- for (;;) {
+ do {
if (unlikely(val == UINT_MAX))
return false;
@@ -194,12 +185,7 @@ bool refcount_sub_and_test(unsigned int
return false;
}
- old = atomic_cmpxchg_release(&r->refs, val, new);
- if (old == val)
- break;
-
- val = old;
- }
+ } while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
return !new;
}
@@ -258,7 +244,9 @@ EXPORT_SYMBOL_GPL(refcount_dec);
*/
bool refcount_dec_if_one(refcount_t *r)
{
- return atomic_cmpxchg_release(&r->refs, 1, 0) == 1;
+ int val = 1;
+
+ return atomic_try_cmpxchg_release(&r->refs, &val, 0);
}
EXPORT_SYMBOL_GPL(refcount_dec_if_one);
@@ -275,9 +263,9 @@ EXPORT_SYMBOL_GPL(refcount_dec_if_one);
*/
bool refcount_dec_not_one(refcount_t *r)
{
- unsigned int old, new, val = atomic_read(&r->refs);
+ unsigned int new, val = atomic_read(&r->refs);
- for (;;) {
+ do {
if (unlikely(val == UINT_MAX))
return true;
@@ -290,12 +278,7 @@ bool refcount_dec_not_one(refcount_t *r)
return true;
}
- old = atomic_cmpxchg_release(&r->refs, val, new);
- if (old == val)
- break;
-
- val = old;
- }
+ } while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
return true;
}
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 5/5] x86,atomic: Use atomic_try_cmpxchg
2017-03-17 21:19 [PATCH 0/5] x86 optimizations Peter Zijlstra
` (3 preceding siblings ...)
2017-03-17 21:19 ` [PATCH 4/5] refcount: Use atomic_try_cmpxchg() Peter Zijlstra
@ 2017-03-17 21:19 ` Peter Zijlstra
4 siblings, 0 replies; 13+ messages in thread
From: Peter Zijlstra @ 2017-03-17 21:19 UTC (permalink / raw)
To: mingo, tglx, hpa
Cc: linux-kernel, torvalds, arjan, bp, richard.weinberger, jpoimboe, peterz
[-- Attachment #1: peterz-x86-atomic-try_cmpxchg.patch --]
[-- Type: text/plain, Size: 3259 bytes --]
text data bss dec hex filename
10665111 4530096 843776 16038983 f4bc47 defconfig-build/vmlinux.3
10655703 4530096 843776 16029575 f49787 defconfig-build/vmlinux.4
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/include/asm/atomic.h | 27 ++++++++----------------
arch/x86/include/asm/atomic64_64.h | 40 ++++++++++++-------------------------
2 files changed, 22 insertions(+), 45 deletions(-)
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -207,16 +207,12 @@ static inline void atomic_##op(int i, at
}
#define ATOMIC_FETCH_OP(op, c_op) \
-static inline int atomic_fetch_##op(int i, atomic_t *v) \
+static inline int atomic_fetch_##op(int i, atomic_t *v) \
{ \
- int old, val = atomic_read(v); \
- for (;;) { \
- old = atomic_cmpxchg(v, val, val c_op i); \
- if (old == val) \
- break; \
- val = old; \
- } \
- return old; \
+ int val = atomic_read(v); \
+ do { \
+ } while (!atomic_try_cmpxchg(v, &val, val c_op i)); \
+ return val; \
}
#define ATOMIC_OPS(op, c_op) \
@@ -242,16 +238,11 @@ ATOMIC_OPS(xor, ^)
*/
static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
{
- int c, old;
- c = atomic_read(v);
- for (;;) {
- if (unlikely(c == (u)))
+ int c = atomic_read(v);
+ do {
+ if (unlikely(c == u))
break;
- old = atomic_cmpxchg((v), c, c + (a));
- if (likely(old == c))
- break;
- c = old;
- }
+ } while (!atomic_try_cmpxchg(v, &c, c + a));
return c;
}
--- a/arch/x86/include/asm/atomic64_64.h
+++ b/arch/x86/include/asm/atomic64_64.h
@@ -198,17 +198,12 @@ static inline long atomic64_xchg(atomic6
*/
static inline bool atomic64_add_unless(atomic64_t *v, long a, long u)
{
- long c, old;
- c = atomic64_read(v);
- for (;;) {
- if (unlikely(c == (u)))
- break;
- old = atomic64_cmpxchg((v), c, c + (a));
- if (likely(old == c))
- break;
- c = old;
- }
- return c != (u);
+ long c = atomic64_read(v);
+ do {
+ if (unlikely(c == u))
+ return false;
+ } while (!atomic64_try_cmpxchg(v, &c, c + a));
+ return true;
}
#define atomic64_inc_not_zero(v) atomic64_add_unless((v), 1, 0)
@@ -222,17 +217,12 @@ static inline bool atomic64_add_unless(a
*/
static inline long atomic64_dec_if_positive(atomic64_t *v)
{
- long c, old, dec;
- c = atomic64_read(v);
- for (;;) {
+ long dec, c = atomic64_read(v);
+ do {
dec = c - 1;
if (unlikely(dec < 0))
break;
- old = atomic64_cmpxchg((v), c, dec);
- if (likely(old == c))
- break;
- c = old;
- }
+ } while (!atomic64_try_cmpxchg(v, &c, dec));
return dec;
}
@@ -248,14 +238,10 @@ static inline void atomic64_##op(long i,
#define ATOMIC64_FETCH_OP(op, c_op) \
static inline long atomic64_fetch_##op(long i, atomic64_t *v) \
{ \
- long old, val = atomic64_read(v); \
- for (;;) { \
- old = atomic64_cmpxchg(v, val, val c_op i); \
- if (old == val) \
- break; \
- val = old; \
- } \
- return old; \
+ long val = atomic64_read(v); \
+ do { \
+ } while (!atomic64_try_cmpxchg(v, &val, val c_op i)); \
+ return val; \
}
#define ATOMIC64_OPS(op, c_op) \
^ permalink raw reply [flat|nested] 13+ messages in thread