Skip to contentSkip to conversation

convert dynamic_debug to use jump labels

PATCH Superseded Jason Baron 3 participants 16 messages May 20, 2016 – Jul 13, 2016 1 message not in the archive

Subsystems (8)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jason Baron

author maintainer
#0 lore raw

v2 cover convert dynamic_debug to use jump labels

Hi,

In the course of adding jump label support to dynamic_debug (patch #4), I found the inclusion of <linux/bug.h> by <linux/jump_label.h> to problematic b/c it includes <linux/kernel.h> (thus, creating circular include dependencies). Thus, I've removed the dependencies from jump_label.h on bug.h and atomic.h so that it should be includable from mostly anywhere. There were a couple of arch specific changes for powerpc (patch #2) and s390 (patch #3) that fell out as well...hopefully this version compiles everywhere.

Thanks,

-Jason

Jason Baron (4):
jump_label: remove bug.h, atomic.h dependencies for HAVE_JUMP_LABEL
powerpc: add explicit #include <asm/asm-compat.h> for jump label
s390: add explicit <linux/stringify.h> for jump label
dynamic_debug: add jump label support

 arch/powerpc/include/asm/jump_label.h |  1 +
 arch/s390/include/asm/jump_label.h    |  1 +
 include/linux/dynamic_debug.h         | 60 +++++++++++++++++++++++++++++++----
 include/linux/jump_label.h            | 50 +++++++++++++++++------------
 kernel/jump_label.c                   | 48 ++++++++++++++++++++++++++++
 lib/dynamic_debug.c                   |  7 ++++
 6 files changed, 141 insertions(+), 26 deletions(-)
Signature · 1 line
2.6.1
author maintainer
#1 lore raw

PATCH v2 1/4 jump_label: remove bug.h, atomic.h dependencies for HAVE_JUMP_LABEL

The current jump_label.h includes bug.h for things such as WARN_ON(). This makes the header problematic for inclusion by kernel.h or any headers that kernel.h includes, since bug.h includes kernel.h (circular dependency). The inclusion of atomic.h is similarly problematic. Thus, this should make jump_label.h 'includable' from most places.

---
 include/linux/jump_label.h | 50 +++++++++++++++++++++++++++-------------------
 kernel/jump_label.c        | 48 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 20 deletions(-)
include/linux/jump_label.h +30 -20
index 0536524..3b9ebb9 100644
@@ -76,7 +76,6 @@
 
 #include <linux/types.h>
 #include <linux/compiler.h>
-#include <linux/bug.h>
 
 extern bool static_key_initialized;
 
@@ -115,13 +114,6 @@ enum jump_label_type {
 
 struct module;
 
-#include <linux/atomic.h>
-
-static inline int static_key_count(struct static_key *key)
-{
-	return atomic_read(&key->enabled);
-}
-
 #ifdef HAVE_JUMP_LABEL
 
 #define JUMP_TYPE_FALSE	0UL
@@ -152,16 +144,34 @@ extern int jump_label_text_reserved(void *start, void *end);
 extern void static_key_slow_inc(struct static_key *key);
 extern void static_key_slow_dec(struct static_key *key);
 extern void jump_label_apply_nops(struct module *mod);
+extern int static_key_count(struct static_key *key);
+extern void static_key_enable(struct static_key *key);
+extern void static_key_disable(struct static_key *key);
 
-#define STATIC_KEY_INIT_TRUE					\
-	{ .enabled = ATOMIC_INIT(1),				\
+/*
+ * We should be using ATOMIC_INIT() for initializing .enabled, but
+ * the inclusion of atomic.h is problematic for inclusion of jump_label.h
+ * in 'low-level' headers. Thus, we are initializing .enabled with a
+ * raw value, but have added a BUILD_BUG_ON() to catch any issues in
+ * jump_label_init() see: kernel/jump_label.c.
+ */
+#define STATIC_KEY_INIT_TRUE			\
+	{ .enabled = { 1 },			\
 	  .entries = (void *)JUMP_TYPE_TRUE }
-#define STATIC_KEY_INIT_FALSE					\
-	{ .enabled = ATOMIC_INIT(0),				\
+#define STATIC_KEY_INIT_FALSE			\
+	{ .enabled = { 0 },			\
 	  .entries = (void *)JUMP_TYPE_FALSE }
 
 #else  /* !HAVE_JUMP_LABEL */
 
+#include <linux/atomic.h>
+#include <linux/bug.h>
+
+static inline int static_key_count(struct static_key *key)
+{
+	return atomic_read(&key->enabled);
+}
+
 static __always_inline void jump_label_init(void)
 {
 	static_key_initialized = true;
@@ -206,14 +216,6 @@ static inline int jump_label_apply_nops(struct module *mod)
 	return 0;
 }
 
-#define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
-#define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }
-
-#endif	/* HAVE_JUMP_LABEL */
-
-#define STATIC_KEY_INIT STATIC_KEY_INIT_FALSE
-#define jump_label_enabled static_key_enabled
-
 static inline void static_key_enable(struct static_key *key)
 {
 	int count = static_key_count(key);
@@ -234,6 +236,14 @@ static inline void static_key_disable(struct static_key *key)
 		static_key_slow_dec(key);
 }
 
+#define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
+#define STATIC_KEY_INIT_FALSE	{ .enabled = ATOMIC_INIT(0) }
+
+#endif	/* HAVE_JUMP_LABEL */
+
+#define STATIC_KEY_INIT STATIC_KEY_INIT_FALSE
+#define jump_label_enabled static_key_enabled
+
 /* -------------------------------------------------------------------------- */
 
 /*
kernel/jump_label.c +48 -0

66 lines not on this page · raw message

Signature · 1 line
2.6.1
author maintainer
#2 lore raw

PATCH v2 2/4 powerpc: add explicit #include <asm/asm-compat.h> for jump label

In reply to Jason Baron

The stringify_in_c() macro may not be included. Make the dependency explicit.

Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
---
 arch/powerpc/include/asm/jump_label.h | 1 +
 1 file changed, 1 insertion(+)
arch/powerpc/include/asm/jump_label.h +1 -0
index 47e155f..9af103a 100644
@@ -14,6 +14,7 @@
 #include <linux/types.h>
 
 #include <asm/feature-fixups.h>
+#include <asm/asm-compat.h>
 
 #define JUMP_ENTRY_TYPE		stringify_in_c(FTR_ENTRY_LONG)
 #define JUMP_LABEL_NOP_SIZE	4
Signature · 1 line
2.6.1
author maintainer
#3 lore raw

PATCH v2 3/4 s390: add explicit <linux/stringify.h> for jump label

In reply to Jason Baron

Ensure that we always have __stringify().

Cc Martin Schwidefsky <[email protected]>
Cc Heiko Carstens <[email protected]>
---
 arch/s390/include/asm/jump_label.h | 1 +
 1 file changed, 1 insertion(+)
arch/s390/include/asm/jump_label.h +1 -0
index 7f9fd5e..9be198f5 100644
@@ -4,6 +4,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/types.h>
+#include <linux/stringify.h>
 
 #define JUMP_LABEL_NOP_SIZE 6
 #define JUMP_LABEL_NOP_OFFSET 2
Signature · 1 line
2.6.1
author maintainer
#4 lore raw

PATCH v2 4/4 dynamic_debug: add jump label support

In reply to Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

text data bss dec hex filename
8194852 4879776 925696 14000324 d5a0c4 vmlinux.pre
8187337 4960224 925696 14073257 d6bda9 vmlinux.post

---
 include/linux/dynamic_debug.h | 60 ++++++++++++++++++++++++++++++++++++++-----
 lib/dynamic_debug.c           |  7 +++++
 2 files changed, 61 insertions(+), 6 deletions(-)
include/linux/dynamic_debug.h +54 -6
index 4f1bbc6..546d680 100644
@@ -1,6 +1,10 @@
 #ifndef _DYNAMIC_DEBUG_H
 #define _DYNAMIC_DEBUG_H
 
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
+#include <linux/jump_label.h>
+#endif
+
 /*
  * An instance of this structure is created in a special
  * ELF section at every dynamic debug callsite.  At runtime,
@@ -33,6 +37,12 @@ struct _ddebug {
 #define _DPRINTK_FLAGS_DEFAULT 0
 #endif
 	unsigned int flags:8;
+#ifdef HAVE_JUMP_LABEL
+	union {
+		struct static_key_true dd_key_true;
+		struct static_key_false dd_key_false;
+	} key;
+#endif
 } __attribute__((aligned(8)));
 
 
@@ -60,7 +70,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			  const struct net_device *dev,
 			  const char *fmt, ...);
 
-#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+#define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init)	\
 	static struct _ddebug  __aligned(8)			\
 	__attribute__((section("__verbose"))) name = {		\
 		.modname = KBUILD_MODNAME,			\
@@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 		.filename = __FILE__,				\
 		.format = (fmt),				\
 		.lineno = __LINE__,				\
-		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
+		.flags = _DPRINTK_FLAGS_DEFAULT,		\
+		dd_key_init(key, init)				\
 	}
 
+#ifdef HAVE_JUMP_LABEL
+
+#define dd_key_init(key, init) key = (init)
+
+#ifdef DEBUG
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, .key.dd_key_true, \
+					  (STATIC_KEY_TRUE_INIT))
+
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	static_branch_likely(&descriptor.key.dd_key_true)
+#else
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, .key.dd_key_false, \
+					  (STATIC_KEY_FALSE_INIT))
+
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	static_branch_unlikely(&descriptor.key.dd_key_false)
+#endif
+
+#else
+
+#define dd_key_init(key, init)
+
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
+
+#ifdef DEBUG
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	likely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+#else
+#define DYNAMIC_DEBUG_BRANCH(descriptor) \
+	unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+#endif
+
+#endif
+
 #define dynamic_pr_debug(fmt, ...)				\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),	\
 				   ##__VA_ARGS__);		\
 } while (0)
@@ -82,7 +130,7 @@ do {								\
 #define dynamic_dev_dbg(dev, fmt, ...)				\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		__dynamic_dev_dbg(&descriptor, dev, fmt,	\
 				  ##__VA_ARGS__);		\
 } while (0)
@@ -90,7 +138,7 @@ do {								\
 #define dynamic_netdev_dbg(dev, fmt, ...)			\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		__dynamic_netdev_dbg(&descriptor, dev, fmt,	\
 				     ##__VA_ARGS__);		\
 } while (0)
@@ -100,7 +148,7 @@ do {								\
 do {								\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor,		\
 		__builtin_constant_p(prefix_str) ? prefix_str : "hexdump");\
-	if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))	\
+	if (DYNAMIC_DEBUG_BRANCH(descriptor))			\
 		print_hex_dump(KERN_DEBUG, prefix_str,		\
 			       prefix_type, rowsize, groupsize,	\
 			       buf, len, ascii);		\
lib/dynamic_debug.c +7 -0
index fe42b6e..da796e2 100644
@@ -188,6 +188,13 @@ static int ddebug_change(const struct ddebug_query *query,
 			newflags = (dp->flags & mask) | flags;
 			if (newflags == dp->flags)
 				continue;
+#ifdef HAVE_JUMP_LABEL
+			if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+				if (!(flags & _DPRINTK_FLAGS_PRINT))
+					static_branch_disable(&dp->key.dd_key_true);
+			} else if (flags & _DPRINTK_FLAGS_PRINT)
+				static_branch_enable(&dp->key.dd_key_true);
+#endif
 			dp->flags = newflags;
 			vpr_info("changed %s:%d [%s]%s =%s\n",
 				 trim_prefix(dp->filename), dp->lineno,
Signature · 1 line
2.6.1
#5 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename
8194852 4879776 925696 14000324 d5a0c4 vmlinux.pre
8187337 4960224 925696 14073257 d6bda9 vmlinux.post

---

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

Arnd

author maintainer
#6 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 14 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename
8194852 4879776 925696 14000324 d5a0c4 vmlinux.pre
8187337 4960224 925696 14073257 d6bda9 vmlinux.post

---

Quoted 8 lines

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

Arnd

Hi,

I stuck pr_debug() in a few functions marked with __exit, but did not reproduce yet. Can you share your .config and gcc --version.

Thanks,

-Jason

#7 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 18 lines · CEST Jason Baron
Quoted 17 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename

8194852      4879776  925696 14000324         d5a0c4 vmlinux.pre
8187337      4960224  925696 14073257         d6bda9 vmlinux.post

---

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

Quoted 7 lines
Quoted 3 lines

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

I stuck pr_debug() in a few functions marked with __exit, but did not reproduce yet. Can you share your .config and gcc --version.

I found these on ARM randconfig builds e.g. this one
http://pastebin.com/raw/KjWHxnwU

I also have some other patches applied that could have interacted with your change, so if you can't reproduce it easily, let me try it on a plain linux-next kernel.

The compiler I use is arm-linux-gnueabi-gcc (GCC) 6.0.0 20160323 (experimental)

Arnd

#8 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 27 lines · CEST Arnd Bergmann
Quoted 18 lines · CEST Jason Baron
Quoted 17 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename

8194852      4879776  925696 14000324         d5a0c4 vmlinux.pre
8187337      4960224  925696 14073257         d6bda9 vmlinux.post

---

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

Quoted 7 lines
Quoted 3 lines

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

I stuck pr_debug() in a few functions marked with __exit, but did not reproduce yet. Can you share your .config and gcc --version.

Quoted 8 lines

I found these on ARM randconfig builds e.g. this one
http://pastebin.com/raw/KjWHxnwU

I also have some other patches applied that could have interacted with your change, so if you can't reproduce it easily, let me try it on a plain linux-next kernel.

The compiler I use is arm-linux-gnueabi-gcc (GCC) 6.0.0 20160323 (experimental)

Update: on ARM, I have been able to reproduce this with gcc-4.6 and gcc-4.8, so I'm pretty confident that this is independent of the toolchain. However, I have so far failed to reproduce this on x86.

Looking at the exit_ceph() function, I get these two assembly outputs, ARM fails with the link error above:

.section .exit.text,"ax",%progbits
.align 2
.syntax unified
.arm
.fpu softvfp
.type exit_ceph, %function
exit_ceph:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 1, uses_anonymous_args = 0
mov ip, sp @,
push {fp, ip, lr, pc} @
sub fp, ip, #4 @,,
sub sp, sp, #8 @,,
.syntax divided
@ 13 "/git/arm-soc/arch/arm/include/asm/jump_label.h" 1

        1:
        nop
        .pushsection __jump_table,  "aw"
        .word 1b, .L341, descriptor.39418+20    @,
        .popsection

@ 0 "" 2
.syntax unified
.L342:

        ldr     r0, .L344       @,
        bl      unregister_filesystem   @
        bl      ceph_xattr_exit @
        bl      destroy_caches  @
        b       .L343   @
.L341:
        mov     r1, #29 @,
        ldr     r0, .L344+4     @,
        bl      ceph_file_part  @
        mov     r3, #1072       @ tmp118,
        mov     r2, #3  @,
        stm     sp, {r0, r3}    @,,
        ldr     r1, .L344+8     @,
        ldr     r3, .L344+12    @,
        ldr     r0, .L344+16    @,
        bl      __dynamic_pr_debug      @
        b       .L342   @

.L343:
sub sp, fp, #12 @,,
ldm sp, {fp, sp, pc} @
.L345:
.align 2
.L344:

        .word   .LANCHOR2+224
        .word   .LC0
        .word   .LC69
        .word   .LC1
        .word   .LANCHOR0+1088
        .size   exit_ceph, .-exit_ceph

and x86 has no link error with:

.type exit_ceph, @function
exit_ceph:
pushq %rbp #
movq %rsp, %rbp #,
#APP
# 35 "/git/arm-soc/arch/x86/include/asm/jump_label.h" 1

        1:.byte 0x0f,0x1f,0x44,0x00,0
        .pushsection __jump_table,  "aw"
         .balign 8
         .quad 1b, .L350, descriptor.39765+40 + 0       #,,
        .popsection

# 0 "" 2
#NO_APP
.L351:

        movq    $ceph_fs_type, %rdi     #,
        call    unregister_filesystem   #
        call    ceph_xattr_exit #
        call    destroy_caches  #
        popq    %rbp    #
        ret
.L350:
        movl    $29, %esi       #,
        movq    $.LC0, %rdi     #,
        call    ceph_file_part  #
        movl    $1072, %r9d     #,
        movq    %rax, %r8       #, D.41790
        movq    $.LC1, %rcx     #,
        movl    $3, %edx        #,
        movq    $.LC85, %rsi    #,
        movq    $descriptor.39765, %rdi #,
        call    __dynamic_pr_debug      #
        jmp     .L351   #
        .size   exit_ceph, .-exit_ceph

In both cases, the __jump_table section clearly has a reference to a discarded section.

Arnd

author maintainer
#9 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 35 lines · Arnd Bergmann
Quoted 34 lines · CEST Arnd Bergmann
Quoted 18 lines · CEST Jason Baron
Quoted 17 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename

8194852      4879776  925696 14000324         d5a0c4 vmlinux.pre
8187337      4960224  925696 14073257         d6bda9 vmlinux.post

---

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

Quoted 7 lines
Quoted 3 lines

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

I stuck pr_debug() in a few functions marked with __exit, but did not reproduce yet. Can you share your .config and gcc --version.

I found these on ARM randconfig builds e.g. this one
http://pastebin.com/raw/KjWHxnwU

I also have some other patches applied that could have interacted with your change, so if you can't reproduce it easily, let me try it on a plain linux-next kernel.

Quoted 9 lines
Quoted 1 line

The compiler I use is arm-linux-gnueabi-gcc (GCC) 6.0.0 20160323 (experimental)

Update: on ARM, I have been able to reproduce this with gcc-4.6 and gcc-4.8, so I'm pretty confident that this is independent of the toolchain. However, I have so far failed to reproduce this on x86.

Looking at the exit_ceph() function, I get these two assembly outputs, ARM fails with the link error above:

ok, does this fix things up?

arch/arm/kernel/vmlinux.lds.S +1 -1
@@ -44,7 +44,7 @@
 #endif
 
 #if (defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)) || \
-       defined(CONFIG_GENERIC_BUG)
+       defined(CONFIG_GENERIC_BUG) || defined(CONFIG_JUMP_LABEL)
 #define ARM_EXIT_KEEP(x)       x
 #define ARM_EXIT_DISCARD(x)
 #else

Thanks,

-Jason

#10 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 52 lines · CEST Jason Baron
Quoted 35 lines · Arnd Bergmann
Quoted 34 lines · CEST Arnd Bergmann
Quoted 18 lines · CEST Jason Baron
Quoted 17 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename

8194852      4879776  925696 14000324         d5a0c4 vmlinux.pre
8187337      4960224  925696 14073257         d6bda9 vmlinux.post

---

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

Quoted 7 lines
Quoted 3 lines

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

I stuck pr_debug() in a few functions marked with __exit, but did not reproduce yet. Can you share your .config and gcc --version.

I found these on ARM randconfig builds e.g. this one
http://pastebin.com/raw/KjWHxnwU

I also have some other patches applied that could have interacted with your change, so if you can't reproduce it easily, let me try it on a plain linux-next kernel.

Quoted 9 lines
Quoted 1 line

The compiler I use is arm-linux-gnueabi-gcc (GCC) 6.0.0 20160323 (experimental)

Update: on ARM, I have been able to reproduce this with gcc-4.6 and gcc-4.8, so I'm pretty confident that this is independent of the toolchain. However, I have so far failed to reproduce this on x86.

Looking at the exit_ceph() function, I get these two assembly outputs, ARM fails with the link error above:

ok, does this fix things up?

arch/arm/kernel/vmlinux.lds.S line 50

Quoted 8 lines
arch/arm/kernel/vmlinux.lds.S +1 -1
@@ -44,7 +44,7 @@
 #endif
 
 #if (defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)) || \
-       defined(CONFIG_GENERIC_BUG)
+       defined(CONFIG_GENERIC_BUG) || defined(CONFIG_JUMP_LABEL)
 #define ARM_EXIT_KEEP(x)       x
 #define ARM_EXIT_DISCARD(x)
 #else

Hi Jason,

sorry for missing your email earlier (and your reminder too), the thread just popped up after Chris Metcalf's reply.

Your patch above probably avoids this, but now I can't reasily test it since it's not in linux-next any more. If you have a git tree I can pull into my test setup, I'll try it out again.

Arnd

A message that is not in the archive (sent off-list or not yet received): [email protected]

#12 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 7 lines · CEST Jason Baron

Hi Arnd,

Ok, this is back in linux-next now (with hopefully a fix for arm). I was never able to quite reproduce the arm failure you saw. So if you get the chance to test this it would be great.

I've had a day's worth of randconfig tests without running into the problem so far.

However, I did get one new compiler warning that I have just bisected down to 21413cd0e4ed ("dynamic_debug: add jump label support"):

/git/arm-soc/drivers/media/dvb-frontends/cxd2841er.c: In function 'cxd2841er_tune_tc':
/git/arm-soc/drivers/media/dvb-frontends/cxd2841er.c:3253:40: error: 'carrier_offset' may be used uninitialized in this function [-Werror=maybe-uninitialized]

    if (ret)
                                        ^
     return ret;
     ~~~~~~~~~                           
/git/arm-soc/drivers/media/dvb-frontends/cxd2841er.c:3209:11: note: 'carrier_offset' was declared here
  int ret, carrier_offset;
           ^~~~~~~~~~~~~~

It's clearly a false positive warning, the code is correct, but if this is the only one that the dynamic_debug jump labels introduce, we may as well just work around it in the driver.

I think this is caused by the "unlikely" annotation in dynamic_dev_dbg(), which confuses the compiler trying to figure out whether the variable is initialized or not.

Arnd

author maintainer
#13 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

In reply to Arnd Bergmann
Quoted 14 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename
8194852 4879776 925696 14000324 d5a0c4 vmlinux.pre
8187337 4960224 925696 14073257 d6bda9 vmlinux.post

---

Quoted 8 lines

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

Arnd

Hi Arnd,

Ok, I managed to reproduce this on tile and sparc64 by adding static_branch_[un]likely() to __exit functions as you mentioned. Although I didn't find the actual broken config.

I think its only an issue on those 2 arches b/c they have jump label support and discard __exit text at build time (most arches seem to do it at run-time). Thus, we can end up with references in the __jump_table to addresses that may be in an __exit section. The jump label code already protects itself from touch code in the init sections after it has been freed. Thus, simply having functions marked with __exit in the init section is sufficient here.

I tried the following patches on tile and sparc and they at least now compile. If anybody has real h/w that would be helpful :)

Thanks,

-Jason

arch/sparc/kernel/vmlinux.lds.S +6 -0
@@ -151,6 +151,12 @@ SECTIONS
        PERCPU_SECTION(SMP_CACHE_BYTES)
 
        . = ALIGN(PAGE_SIZE);
+       .exit.text : {
+               EXIT_TEXT
+       }
+
+       . = ALIGN(PAGE_SIZE);
+
        __init_end = .;
        BSS_SECTION(0, 0, 0)
        _end = . ;
arch/tile/kernel/vmlinux.lds.S +0 -0
@@ -58,7 +58,21 @@ SECTIONS
   _etext = .;
 
   /* "Init" is divided into two areas with very different virtual

addresses. */
+ . = ALIGN(PAGE_SIZE);
+ .init.begin : AT(ADDR(.init.begin) - LOAD_OFFSET) {
+ __init_begin = .; /* paired with __init_end */
+ }
+
INIT_TEXT_SECTION(PAGE_SIZE)
+ .exit.text : AT(ADDR(.exit.text) - LOAD_OFFSET) {
+ EXIT_TEXT
+ }
+

+  . = ALIGN(PAGE_SIZE);
+  /* freed after init ends here */
+  .init.end : AT(ADDR(.init.end) - LOAD_OFFSET) {

+ __init_end = .;
+ }

/* Now we skip back to PAGE_OFFSET for the data. */ . = (. - TEXT_OFFSET + PAGE_OFFSET);

#14 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 28 lines · Jason Baron
Quoted 13 lines · Arnd Bergmann
Quoted 6 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its enabled for production builds as well. Minimize its impact by using jump labels. This reduces the text section by 7000+ bytes in the kernel image below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines

text data bss dec hex filename
8194852 4879776 925696 14000324 d5a0c4 vmlinux.pre
8187337 4960224 925696 14073257 d6bda9 vmlinux.post

---

Quoted 8 lines

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o .exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section .exit.text' of drivers/built-in.o

Arnd

Hi Arnd,

Ok, I managed to reproduce this on tile and sparc64 by adding static_branch_[un]likely() to __exit functions as you mentioned. Although I didn't find the actual broken config.

Quoted 8 lines

I think its only an issue on those 2 arches b/c they have jump label support and discard __exit text at build time (most arches seem to do it at run-time). Thus, we can end up with references in the __jump_table to addresses that may be in an __exit section. The jump label code already protects itself from touch code in the init sections after it has been freed. Thus, simply having functions marked with __exit in the init section is sufficient here.

It seems plausible to me to only include the exit text in with the init text under an #ifdef CONFIG_JUMP_TABLE (with a suitable comment) in any case, because if we don't need to include it in the image, then why do so? It adds about 7KB to the loaded size of the vmlinux image for no gain (on a typical tilegx configuration).

Quoted 15 lines
arch/tile/kernel/vmlinux.lds.S +0 -0
@@ -58,7 +58,21 @@ SECTIONS
    _etext = .;
 
    /* "Init" is divided into two areas with very different virtual

addresses. */

arch/tile/kernel/vmlinux.lds.S +7 -0
+  . = ALIGN(PAGE_SIZE);
+  .init.begin : AT(ADDR(.init.begin) - LOAD_OFFSET) {
+     __init_begin = .; /* paired with __init_end */
+  }
+
    INIT_TEXT_SECTION(PAGE_SIZE)
+  .exit.text : AT(ADDR(.exit.text) - LOAD_OFFSET) {
+       EXIT_TEXT

arch/tile/kernel/vmlinux.lds.S

Quoted 10 lines
arch/tile/kernel/vmlinux.lds.S +7 -0
+  }
+
+  . = ALIGN(PAGE_SIZE);
+  /* freed after init ends here */
+  .init.end : AT(ADDR(.init.end) - LOAD_OFFSET) {
+       __init_end = .;
+  }
 
    /* Now we skip back to PAGE_OFFSET for the data. */
    . = (. - TEXT_OFFSET + PAGE_OFFSET);

This doesn't look right to me; we already have an __init_begin symbol defined a few lines further down in vmlinux.lds.S. How does this patch work instead?

arch/tile/kernel/vmlinux.lds.S +9 -1
index 378f5d8d1ec8..5e83d2689def 100644
@@ -58,7 +58,15 @@ SECTIONS
    _etext = .;
 
    /* "Init" is divided into two areas with very different virtual addresses. */
-  INIT_TEXT_SECTION(PAGE_SIZE)
+  . = ALIGN(PAGE_SIZE);
+  .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) {
+    VMLINUX_SYMBOL(_sinittext) = .;
+    INIT_TEXT
+#ifdef CONFIG_JUMP_LABEL  /* __jump_table may reference __exit text */
+    EXIT_TEXT
+#endif
+    VMLINUX_SYMBOL(_einittext) = .;
+  }
 
    /* Now we skip back to PAGE_OFFSET for the data. */
    . = (. - TEXT_OFFSET + PAGE_OFFSET);
Signature · 2 lines
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com
author maintainer
#15 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 96 lines · Chris Metcalf
Quoted 35 lines · Jason Baron
Quoted 19 lines · Arnd Bergmann
Quoted 9 lines · CEST Jason Baron

Although dynamic debug is often only used for debug builds, sometimes its
enabled for production builds as well. Minimize its impact by using jump
labels. This reduces the text section by 7000+ bytes in the kernel image
below. It does increase data, but this should only be referenced when changing the direction of the branches, and hence usually not in cache.

Quoted 6 lines
    text       data        bss        dec        hex    filename
8194852    4879776     925696    14000324     d5a0c4    vmlinux.pre
8187337    4960224     925696    14073257     d6bda9    vmlinux.post

---

This causes problems for some of my randconfig builds, when a dynamic debug call is used inside of an __exit function:

Quoted 9 lines

.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
.exit.text' referenced in section __jump_table' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o

Arnd

Hi Arnd,

Ok, I managed to reproduce this on tile and sparc64 by adding static_branch_[un]likely() to __exit functions as you mentioned. Although I didn't find the actual broken config.

Quoted 8 lines

I think its only an issue on those 2 arches b/c they have jump label support and discard __exit text at build time (most arches seem to do it at run-time). Thus, we can end up with references in the __jump_table to addresses that may be in an __exit section. The jump label code already protects itself from touch code in the init sections after it has been freed. Thus, simply having functions marked with __exit in the init section is sufficient here.

It seems plausible to me to only include the exit text in with the init text
under an #ifdef CONFIG_JUMP_TABLE (with a suitable comment) in any case, because if we don't need to include it in the image, then why do so? It adds about 7KB to the loaded size of the vmlinux image for no gain (on a typical tilegx configuration).

Quoted 15 lines
arch/tile/kernel/vmlinux.lds.S +0 -0
@@ -58,7 +58,21 @@ SECTIONS
    _etext = .;
 
    /* "Init" is divided into two areas with very different virtual

addresses. */

arch/tile/kernel/vmlinux.lds.S +7 -0
+  . = ALIGN(PAGE_SIZE);
+  .init.begin : AT(ADDR(.init.begin) - LOAD_OFFSET) {
+     __init_begin = .; /* paired with __init_end */
+  }
+
    INIT_TEXT_SECTION(PAGE_SIZE)
+  .exit.text : AT(ADDR(.exit.text) - LOAD_OFFSET) {
+       EXIT_TEXT

arch/tile/kernel/vmlinux.lds.S

Quoted 10 lines
arch/tile/kernel/vmlinux.lds.S +7 -0
+  }
+
+  . = ALIGN(PAGE_SIZE);
+  /* freed after init ends here */
+  .init.end : AT(ADDR(.init.end) - LOAD_OFFSET) {
+       __init_end = .;
+  }
 
    /* Now we skip back to PAGE_OFFSET for the data. */
    . = (. - TEXT_OFFSET + PAGE_OFFSET);

This doesn't look right to me; we already have an __init_begin symbol defined a few lines further down in vmlinux.lds.S. How does this patch work instead?

arch/tile/kernel/vmlinux.lds.S +0 -0
index 378f5d8d1ec8..5e83d2689def 100644
@@ -58,7 +58,15 @@ SECTIONS
   _etext = .;
 
   /* "Init" is divided into two areas with very different virtual

addresses. */

arch/tile/kernel/vmlinux.lds.S +2 -1
-  INIT_TEXT_SECTION(PAGE_SIZE)
+  . = ALIGN(PAGE_SIZE);
+  .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) {

arch/tile/kernel/vmlinux.lds.S SECTIONS · line 72

Quoted 10 lines
arch/tile/kernel/vmlinux.lds.S +7 -0
+    VMLINUX_SYMBOL(_sinittext) = .;
+    INIT_TEXT
+#ifdef CONFIG_JUMP_LABEL  /* __jump_table may reference __exit text */
+    EXIT_TEXT
+#endif
+    VMLINUX_SYMBOL(_einittext) = .;
+  }
 
   /* Now we skip back to PAGE_OFFSET for the data. */
   . = (. - TEXT_OFFSET + PAGE_OFFSET);

Hi Chris,

Thanks for the patch. I can confirm that it resolves the following compilation issue:

.exit.text' referenced in section __jump_table' of fs/built-in.o:
defined in discarded section .exit.text' of fs/built-in.o .exit.text' referenced in section `__jump_table' of fs/built-in.o:
defined in discarded section `.exit.text' of fs/built-in.o

I was able to create the issue by simply adding a static_key_unlikely() branch to an __exit section that ends up being built-in (non-module).

So this issue is really independent of this patch series... Should I add your patch to my series when I re-post?

Thanks,

-Jason

#16 lore raw

Re: [PATCH v2 4/4] dynamic_debug: add jump label support

Quoted 53 lines · Jason Baron
Quoted 31 lines · Chris Metcalf
Quoted 6 lines · Jason Baron

Hi Arnd,

Ok, I managed to reproduce this on tile and sparc64 by adding static_branch_[un]likely() to __exit functions as you mentioned. Although I didn't find the actual broken config.

Quoted 8 lines

I think its only an issue on those 2 arches b/c they have jump label support and discard __exit text at build time (most arches seem to do it at run-time). Thus, we can end up with references in the __jump_table to addresses that may be in an __exit section. The jump label code already protects itself from touch code in the init sections after it has been freed. Thus, simply having functions marked with __exit in the init section is sufficient here.

How does this patch work instead?

arch/tile/kernel/vmlinux.lds.S +0 -0
index 378f5d8d1ec8..5e83d2689def 100644
@@ -58,7 +58,15 @@ SECTIONS
    _etext = .;
 
    /* "Init" is divided into two areas with very different virtual

addresses. */

arch/tile/kernel/vmlinux.lds.S +2 -1
-  INIT_TEXT_SECTION(PAGE_SIZE)
+  . = ALIGN(PAGE_SIZE);
+  .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) {

arch/tile/kernel/vmlinux.lds.S SECTIONS · line 72

Quoted 10 lines
arch/tile/kernel/vmlinux.lds.S +7 -0
+    VMLINUX_SYMBOL(_sinittext) = .;
+    INIT_TEXT
+#ifdef CONFIG_JUMP_LABEL  /* __jump_table may reference __exit text */
+    EXIT_TEXT
+#endif
+    VMLINUX_SYMBOL(_einittext) = .;
+  }
 
    /* Now we skip back to PAGE_OFFSET for the data. */
    . = (. - TEXT_OFFSET + PAGE_OFFSET);

Hi Chris,

Thanks for the patch. I can confirm that it resolves the following compilation issue:

.exit.text' referenced in section __jump_table' of fs/built-in.o:
defined in discarded section .exit.text' of fs/built-in.o .exit.text' referenced in section `__jump_table' of fs/built-in.o:
defined in discarded section `.exit.text' of fs/built-in.o

Quoted 6 lines

I was able to create the issue by simply adding a static_key_unlikely() branch to an __exit section that ends up being built-in (non-module).

So this issue is really independent of this patch series... Should I add your patch to my series when I re-post?

I'm happy to just take a version of this into the tile tree instead, since you're right, it's a pre-existing problem. I'll post the revised change shortly and cc you. Thanks!

Signature · 2 lines
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com