[PATCH v2 0/4] convert dynamic_debug to use jump labels

[PATCH v2 0/4] convert dynamic_debug to use jump labels

[Jason Baron]

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(-)

-- 
2.6.1

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

[Jason Baron]

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.

Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 include/linux/jump_label.h | 50 +++++++++++++++++++++++++++-------------------
 kernel/jump_label.c        | 48 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 0536524..3b9ebb9 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -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
+
 /* -------------------------------------------------------------------------- */
 
 /*
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 05254ee..3c67e5e 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/static_key.h>
 #include <linux/jump_label_ratelimit.h>
+#include <linux/bug.h>
 
 #ifdef HAVE_JUMP_LABEL
 
@@ -56,6 +57,44 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
 
 static void jump_label_update(struct static_key *key);
 
+/*
+ * There is a similar definition for the !HAVE_JUMP_LABEL case in jump_label.h.
+ * The reason for the copying is that the use of 'atomic_read()' requires
+ * atomic.h and its problematic for some kernel headers such as kernel.h and
+ * others. Since static_key_count() is not used in the branch statements as it
+ * is for the !HAVE_JUMP_LABEL case its ok to have it be a function here.
+ * Similarly, for 'static_key_enable()' and 'static_key_disable()', which
+ * require bug.h. This should allow jump_label.h to be included from most/all
+ * places for HAVE_JUMP_LABEL.
+ */
+int static_key_count(struct static_key *key)
+{
+	return atomic_read(&key->enabled);
+}
+EXPORT_SYMBOL_GPL(static_key_count);
+
+void static_key_enable(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (!count)
+		static_key_slow_inc(key);
+}
+EXPORT_SYMBOL_GPL(static_key_enable);
+
+void static_key_disable(struct static_key *key)
+{
+	int count = static_key_count(key);
+
+	WARN_ON_ONCE(count < 0 || count > 1);
+
+	if (count)
+		static_key_slow_dec(key);
+}
+EXPORT_SYMBOL_GPL(static_key_disable);
+
 void static_key_slow_inc(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE();
@@ -205,6 +244,15 @@ void __init jump_label_init(void)
 	struct static_key *key = NULL;
 	struct jump_entry *iter;
 
+	/*
+	 * Since we are initializing the static_key.enabled field with
+	 * with the 'raw' int values (to avoid pulling in atomic.h), let's make
+	 * sure that is safe. There are only two cases to check since we
+	 * initialize to 0 or 1.
+	 */
+	BUILD_BUG_ON((int)ATOMIC_INIT(0) != 0);
+	BUILD_BUG_ON((int)ATOMIC_INIT(1) != 1);
+
 	jump_label_lock();
 	jump_label_sort_entries(iter_start, iter_stop);
 
-- 
2.6.1

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

[Jason Baron]

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

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/powerpc/include/asm/jump_label.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
index 47e155f..9af103a 100644
--- a/arch/powerpc/include/asm/jump_label.h
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -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
-- 
2.6.1

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

[Jason Baron]

Ensure that we always have __stringify().

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/s390/include/asm/jump_label.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/include/asm/jump_label.h b/arch/s390/include/asm/jump_label.h
index 7f9fd5e..9be198f5 100644
--- a/arch/s390/include/asm/jump_label.h
+++ b/arch/s390/include/asm/jump_label.h
@@ -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
-- 
2.6.1

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

[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

Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 include/linux/dynamic_debug.h | 60 ++++++++++++++++++++++++++++++++++++++-----
 lib/dynamic_debug.c           |  7 +++++
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 4f1bbc6..546d680 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -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);		\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fe42b6e..da796e2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -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,
-- 
2.6.1

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

[Arnd Bergmann]

On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
> 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
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---

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

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

[Jason Baron]

On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>> 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
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
> 
> 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

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

[Arnd Bergmann]

On Friday, June 10, 2016 11:33:07 AM CEST Jason Baron wrote:
> On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> > On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
> >> 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
> >>
> >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >> ---
> > 
> > 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
> > 
> 
> 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

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

[Arnd Bergmann]

On Monday, June 13, 2016 6:05:22 PM CEST Arnd Bergmann wrote:
> On Friday, June 10, 2016 11:33:07 AM CEST Jason Baron wrote:
> > On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> > > On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
> > >> 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
> > >>
> > >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> > >> ---
> > > 
> > > 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
> > > 
> > 
> > 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)

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

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

[Jason Baron]

On 06/13/2016 04:23 PM, Arnd Bergmann wrote:
> On Monday, June 13, 2016 6:05:22 PM CEST Arnd Bergmann wrote:
>> On Friday, June 10, 2016 11:33:07 AM CEST Jason Baron wrote:
>>> On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
>>>> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>>>>> 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
>>>>>
>>>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>>>> ---
>>>>
>>>> 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
>>>>
>>>
>>> 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)
> 
> 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?

--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -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

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

[Arnd Bergmann]

On Monday, June 13, 2016 4:32:15 PM CEST Jason Baron wrote:
> On 06/13/2016 04:23 PM, Arnd Bergmann wrote:
> > On Monday, June 13, 2016 6:05:22 PM CEST Arnd Bergmann wrote:
> >> On Friday, June 10, 2016 11:33:07 AM CEST Jason Baron wrote:
> >>> On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> >>>> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
> >>>>> 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
> >>>>>
> >>>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >>>>> ---
> >>>>
> >>>> 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
> >>>>
> >>>
> >>> 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)
> > 
> > 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?
> 
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -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

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

[Arnd Bergmann]

On Wednesday, July 13, 2016 11:41:50 AM CEST Jason Baron wrote:
> 
> 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

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

[Jason Baron]

On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>> 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
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
> 
> 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

--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -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 = . ;

--- a/arch/tile/kernel/vmlinux.lds.S
+++ b/arch/tile/kernel/vmlinux.lds.S
@@ -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);

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

[Chris Metcalf]

On 6/10/2016 5:28 PM, Jason Baron wrote:
> On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
>> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>>> 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
>>>
>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>> ---
>> 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.

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).

> --- a/arch/tile/kernel/vmlinux.lds.S
> +++ b/arch/tile/kernel/vmlinux.lds.S
> @@ -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);

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?

diff --git a/arch/tile/kernel/vmlinux.lds.S b/arch/tile/kernel/vmlinux.lds.S
index 378f5d8d1ec8..5e83d2689def 100644
--- a/arch/tile/kernel/vmlinux.lds.S
+++ b/arch/tile/kernel/vmlinux.lds.S
@@ -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);

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

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

[Jason Baron]

On 07/01/2016 03:30 PM, Chris Metcalf wrote:
> On 6/10/2016 5:28 PM, Jason Baron wrote:
>> On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
>>> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>>>> 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
>>>>
>>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>>> ---
>>> 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.
> 
> 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).
> 
>> --- a/arch/tile/kernel/vmlinux.lds.S
>> +++ b/arch/tile/kernel/vmlinux.lds.S
>> @@ -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);
> 
> 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?
> 
> diff --git a/arch/tile/kernel/vmlinux.lds.S
> b/arch/tile/kernel/vmlinux.lds.S
> index 378f5d8d1ec8..5e83d2689def 100644
> --- a/arch/tile/kernel/vmlinux.lds.S
> +++ b/arch/tile/kernel/vmlinux.lds.S
> @@ -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);
> 


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

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

[Chris Metcalf]

On 7/5/2016 4:57 PM, Jason Baron wrote:
> On 07/01/2016 03:30 PM, Chris Metcalf wrote:
>> On 6/10/2016 5:28 PM, Jason Baron wrote:
>>> 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.
>> How does this patch work instead?
>>
>> diff --git a/arch/tile/kernel/vmlinux.lds.S
>> b/arch/tile/kernel/vmlinux.lds.S
>> index 378f5d8d1ec8..5e83d2689def 100644
>> --- a/arch/tile/kernel/vmlinux.lds.S
>> +++ b/arch/tile/kernel/vmlinux.lds.S
>> @@ -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);
>>
>
> 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?

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!

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com