All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] x86 optimizations
@ 2017-03-17 21:19 Peter Zijlstra
  2017-03-17 21:19 ` [PATCH 1/5] x86: Implement __WARN using UD0 Peter Zijlstra
                   ` (4 more replies)
  0 siblings, 5 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


Hi,

These are the x86 optimization patches that I've been sitting on.

They shrink the kernel and generate better code.

   text    data     bss     dec     hex filename

10702123        4530992  843776 16076891         f5505b defconfig-build/vmlinux.0
10682460        4530992  843776 16057228         f5038c defconfig-build/vmlinux.1
10665111        4530096  843776 16038983         f4bc47 defconfig-build/vmlinux.2
10665111        4530096  843776 16038983         f4bc47 defconfig-build/vmlinux.5

As compiled on an x86_64 using GCC-6.3, also boot tested the lot.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [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
* [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
* 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
end of thread, other threads:[~2017-03-22 14:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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-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-21 15:41         ` Peter Zijlstra
2017-03-22  8:47     ` Peter Zijlstra
2017-03-22 14:18       ` Josh Poimboeuf
2017-03-17 21:19 ` [PATCH 2/5] bug: Add _ONCE logic to report_bug() Peter Zijlstra
2017-03-17 21:19 ` [PATCH 3/5] atomic: Introduce atomic_try_cmpxchg() 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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.