[PATCH v3 0/7] convert dynamic_debug to use jump labels

[PATCH v3 0/7] convert dynamic_debug to use jump labels

[Jason Baron]

Hi,

Since v2, I found that s390, sparc, and tile could end up with
link errors such as:

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

This is due to the fact the '__jump_table' may reference text
in an __exit section which is discarded during build/link time.
Thus, the sparc, arm and tile patch have been added to address
this issue.

In addition, an 'allyesconfig' on arm did not build either due
to the usage of -O0 compiler flag. This has been removed in 
this patch: http://lkml.iu.edu/hypermail/linux/kernel/1606.3/01587.html

So this series depends on that patch to build properly on arm
(which I currently see being pulled into linux-next).

-v3:
 *added sparc, arm, tile patches for cross section references
 *re-based on (4c5ea0a locking/static_key: Fix concurrent static_key_slow_inc())

Thanks,

-Jason

Chris Metcalf (1):
  tile: support static_key usage in non-module __exit sections

Jason Baron (6):
  powerpc: add explicit #include <asm/asm-compat.h> for jump label
  s390: add explicit #include <linux/stringify.h> for jump label
  sparc: support static_key usage in non-module __exit sections
  arm: jump label may reference text in __exit
  jump_label: remove bug.h, atomic.h dependencies for HAVE_JUMP_LABEL
  dynamic_debug: add jump label support

 arch/arm/kernel/vmlinux.lds.S         |  2 +-
 arch/powerpc/include/asm/jump_label.h |  1 +
 arch/s390/include/asm/jump_label.h    |  1 +
 arch/sparc/kernel/vmlinux.lds.S       |  7 ++++
 arch/tile/kernel/vmlinux.lds.S        | 12 +++++++
 include/linux/dynamic_debug.h         | 60 +++++++++++++++++++++++++++++++----
 include/linux/jump_label.h            | 46 +++++++++++++--------------
 kernel/jump_label.c                   | 53 +++++++++++++++++++++++++++++++
 lib/dynamic_debug.c                   |  7 ++++
 9 files changed, 159 insertions(+), 30 deletions(-)

-- 
2.6.1

[PATCH v3 1/7] 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 47e155f15433..9af103a23975 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

Re: [v3, 1/7] powerpc: add explicit #include <asm/asm-compat.h> for jump label

[Michael Ellerman]

On Wed, 2016-06-07 at 21:42:30 UTC, jbaron@akamai.com wrote:
> 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>

I didn't test it, but assuming you did:

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

[PATCH v3 2/7] 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 7f9fd5e3f1bf..9be198f5ee79 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 v3 3/7] sparc: support static_key usage in non-module __exit sections

[Jason Baron]

The jump table can reference text found in an __exit section. Thus,
instead of discarding it at build/link time, include EXIT_TEXT as part
of __init and release it at system boot time.

Without this patch the link fails with:

    `.exit.text' referenced in section `__jump_table' of xxx.o:
    defined in discarded section `.exit.text' of xxx.o

Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/sparc/kernel/vmlinux.lds.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
index 7d02b1fef025..d79b3b734245 100644
--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -150,6 +150,13 @@ SECTIONS
 	}
 	PERCPU_SECTION(SMP_CACHE_BYTES)
 
+#ifdef CONFIG_JUMP_LABEL
+	. = ALIGN(PAGE_SIZE);
+	.exit.text : {
+		EXIT_TEXT
+	}
+#endif
+
 	. = ALIGN(PAGE_SIZE);
 	__init_end = .;
 	BSS_SECTION(0, 0, 0)
-- 
2.6.1

[PATCH v3 3/7] sparc: support static_key usage in non-module __exit sections

[Jason Baron]

The jump table can reference text found in an __exit section. Thus,
instead of discarding it at build/link time, include EXIT_TEXT as part
of __init and release it at system boot time.

Without this patch the link fails with:

    `.exit.text' referenced in section `__jump_table' of xxx.o:
    defined in discarded section `.exit.text' of xxx.o

Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/sparc/kernel/vmlinux.lds.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
index 7d02b1fef025..d79b3b734245 100644
--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -150,6 +150,13 @@ SECTIONS
 	}
 	PERCPU_SECTION(SMP_CACHE_BYTES)
 
+#ifdef CONFIG_JUMP_LABEL
+	. = ALIGN(PAGE_SIZE);
+	.exit.text : {
+		EXIT_TEXT
+	}
+#endif
+
 	. = ALIGN(PAGE_SIZE);
 	__init_end = .;
 	BSS_SECTION(0, 0, 0)
-- 
2.6.1

Re: [PATCH v3 3/7] sparc: support static_key usage in non-module __exit sections

[David Miller]

From: Jason Baron <jbaron@akamai.com>
Date: Wed,  6 Jul 2016 17:42:32 -0400

> The jump table can reference text found in an __exit section. Thus,
> instead of discarding it at build/link time, include EXIT_TEXT as part
> of __init and release it at system boot time.
> 
> Without this patch the link fails with:
> 
>     `.exit.text' referenced in section `__jump_table' of xxx.o:
>     defined in discarded section `.exit.text' of xxx.o
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: sparclinux@vger.kernel.org
> Signed-off-by: Jason Baron <jbaron@akamai.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH v3 3/7] sparc: support static_key usage in non-module __exit sections

[David Miller]

From: Jason Baron <jbaron@akamai.com>
Date: Wed,  6 Jul 2016 17:42:32 -0400

> The jump table can reference text found in an __exit section. Thus,
> instead of discarding it at build/link time, include EXIT_TEXT as part
> of __init and release it at system boot time.
> 
> Without this patch the link fails with:
> 
>     `.exit.text' referenced in section `__jump_table' of xxx.o:
>     defined in discarded section `.exit.text' of xxx.o
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: sparclinux@vger.kernel.org
> Signed-off-by: Jason Baron <jbaron@akamai.com>

Acked-by: David S. Miller <davem@davemloft.net>

[PATCH v3 4/7] tile: support static_key usage in non-module __exit sections

[Jason Baron]

From: Chris Metcalf <cmetcalf@mellanox.com>

Previously, all the __exit sections were just dropped by
the link phase.  However, if there are static_key (jump label)
constructs in __exit sections that are not modules, the link fails
with the message:

   `.exit.text' referenced in section `__jump_table' of xxx.o:
   defined in discarded section `.exit.text' of xxx.o

Support this usage by keeping the .exit.text sections in the
final image if JUMP_LABEL is defined, then discarding them
once initialization is complete.

Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/tile/kernel/vmlinux.lds.S | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/tile/kernel/vmlinux.lds.S b/arch/tile/kernel/vmlinux.lds.S
index 378f5d8d1ec8..9d449caf8910 100644
--- a/arch/tile/kernel/vmlinux.lds.S
+++ b/arch/tile/kernel/vmlinux.lds.S
@@ -60,6 +60,18 @@ SECTIONS
   /* "Init" is divided into two areas with very different virtual addresses. */
   INIT_TEXT_SECTION(PAGE_SIZE)
 
+  /*
+   * Some things, like the __jump_table, may contain symbol references
+   * to __exit text, so include such text in the final image if so.
+   * In that case we also override the _einittext from INIT_TEXT_SECTION.
+   */
+#ifdef CONFIG_JUMP_LABEL
+  .exit.text : {
+    EXIT_TEXT
+    _einittext = .;
+  }
+#endif
+
   /* Now we skip back to PAGE_OFFSET for the data. */
   . = (. - TEXT_OFFSET + PAGE_OFFSET);
   #undef LOAD_OFFSET
-- 
2.6.1

[PATCH v3 5/7] arm: jump label may reference text in __exit

[Jason Baron]

The jump table can reference text found in an __exit section. Thus,
instead of discarding it at build time, include EXIT_TEXT as part
of __init and it will be released when the system boots.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux at armlinux.org.uk
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/arm/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index e2c6da096cef..d4be685bc1eb 100644
--- 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
-- 
2.6.1

[PATCH v3 5/7] arm: jump label may reference text in __exit

[Jason Baron]

The jump table can reference text found in an __exit section. Thus,
instead of discarding it at build time, include EXIT_TEXT as part
of __init and it will be released when the system boots.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux@armlinux.org.uk
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/arm/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index e2c6da096cef..d4be685bc1eb 100644
--- 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
-- 
2.6.1

[PATCH v3 6/7] 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 | 46 ++++++++++++++++++++--------------------
 kernel/jump_label.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 23 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 68904469fba1..661af564fae8 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,20 +114,8 @@ enum jump_label_type {
 
 struct module;
 
-#include <linux/atomic.h>
-
 #ifdef HAVE_JUMP_LABEL
 
-static inline int static_key_count(struct static_key *key)
-{
-	/*
-	 * -1 means the first static_key_slow_inc() is in progress.
-	 *  static_key_enabled() must return true, so return 1 here.
-	 */
-	int n = atomic_read(&key->enabled);
-	return n >= 0 ? n : 1;
-}
-
 #define JUMP_TYPE_FALSE	0UL
 #define JUMP_TYPE_TRUE	1UL
 #define JUMP_TYPE_MASK	1UL
@@ -157,16 +144,29 @@ 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);
 
+/*
+ * 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 = ATOMIC_INIT(1),				\
+	{ .enabled = { 1 },					\
 	  .entries = (void *)JUMP_TYPE_TRUE }
 #define STATIC_KEY_INIT_FALSE					\
-	{ .enabled = ATOMIC_INIT(0),				\
+	{ .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);
@@ -216,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);
@@ -244,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 4b353e0be121..b83baa937adf 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,49 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
 
 static void jump_label_update(struct static_key *key);
 
+/*
+ * There are similar definitions for the !HAVE_JUMP_LABEL case in jump_label.h.
+ * 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)
+{
+	/*
+	 * -1 means the first static_key_slow_inc() is in progress.
+	 *  static_key_enabled() must return true, so return 1 here.
+	 */
+	int n = atomic_read(&key->enabled);
+
+	return n >= 0 ? n : 1;
+}
+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)
 {
 	int v, v1;
@@ -235,6 +279,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) in
+	 * jump_label.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 v3 7/7] 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 4f1bbc68cd1b..546d68057e3b 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 fe42b6ec3f0c..da796e2dc4f5 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 v3 7/7] dynamic_debug: add jump label support

[Joe Perches]

On Wed, 2016-07-06 at 17:42 -0400, 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

Maybe to get some of that space back on the 32 bit builds
the 8 byte alignment can be relaxed.

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
[]
> @@ -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)));

Couldn't this be:

} __aligned(__alignof__(void *));
 
> @@ -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 = {		\

And this shouldn't need the __aligned as it's already
in the structure definition.

Maybe:

	static struct _ddebug __section(__verbose) name = {

>  		.modname = KBUILD_MODNAME,			\

Re: [PATCH v3 7/7] dynamic_debug: add jump label support

[Jason Baron]

On 07/06/2016 06:52 PM, Joe Perches wrote:
> On Wed, 2016-07-06 at 17:42 -0400, 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
> 
> Maybe to get some of that space back on the 32 bit builds
> the 8 byte alignment can be relaxed.
> 
>> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> []
>> @@ -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)));
> 
> Couldn't this be:
> 
> } __aligned(__alignof__(void *));
>  
>> @@ -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 = {		\
> 
> And this shouldn't need the __aligned as it's already
> in the structure definition.
> 
> Maybe:
> 
> 	static struct _ddebug __section(__verbose) name = {
> 
>>  		.modname = KBUILD_MODNAME,			\
> 


hmmm...dropping that last bit increases the size of 'data', and
results in an oops on boot, I think b/c the __verbose section
adds gaps that the dynamic debug init code is not expecting. We
could revisit this for 32-bit, but this is somewhat fragile...

I think we could reduce the size of 'struct static_key' by
removing the 'next' field and using the 'entries' when there
are multiple modules associated with a key. This will increase
the memory usage slightly when there are multiple modules
associated with a key, but this is probably not the common case,
and certainly for dynamic debug we only have one patch target
for each key. That should reduce the 'data' increase by around
~20% and would reduce the size for other users. I can try something
along those lines in a re-post...

Thanks,

-Jason

Re: [PATCH v3 7/7] dynamic_debug: add jump label support

[Andrew Morton]

On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> 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.
> 
> ...
>
> +#ifdef HAVE_JUMP_LABEL
> +
> +#define dd_key_init(key, init) key = (init)
>
> ...
>
> +#else
> +
> +#define dd_key_init(key, init)
> +

umm, lazy.  One is an lval and returns a value and the other does
neither.  Lack of parenthesization in the first version doubtless
exposes various horrors.

Care to do something more robust and conventional here?  Presumably use
symmetrical do{}while(0) things, neither of which is an lval, both of
which don't return anything.

Re: [PATCH v3 7/7] dynamic_debug: add jump label support

[Jason Baron]

On 07/08/2016 05:41 PM, Andrew Morton wrote:
> On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> 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.
>>
>> ...
>>
>> +#ifdef HAVE_JUMP_LABEL
>> +
>> +#define dd_key_init(key, init) key = (init)
>>
>> ...
>>
>> +#else
>> +
>> +#define dd_key_init(key, init)
>> +
> umm, lazy.  One is an lval and returns a value and the other does
> neither.  Lack of parenthesization in the first version doubtless
> exposes various horrors.
>
> Care to do something more robust and conventional here?  Presumably use
> symmetrical do{}while(0) things, neither of which is an lval, both of
> which don't return anything.
>

Hi,

The 'dd_key_init()' macro is being used here to help initialize
the 'key' field in the 'struct _ddebug', and its not being used as a
statement.

In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
in the structure (to conserve space).

Thanks,

-Jason

Re: [PATCH v3 7/7] dynamic_debug: add jump label support

[Andrew Morton]

On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron <jbaron@akamai.com> wrote:

> On 07/08/2016 05:41 PM, Andrew Morton wrote:
> > On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> 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.
> >>
> >> ...
> >>
> >> +#ifdef HAVE_JUMP_LABEL
> >> +
> >> +#define dd_key_init(key, init) key = (init)
> >>
> >> ...
> >>
> >> +#else
> >> +
> >> +#define dd_key_init(key, init)
> >> +
> > umm, lazy.  One is an lval and returns a value and the other does
> > neither.  Lack of parenthesization in the first version doubtless
> > exposes various horrors.
> >
> > Care to do something more robust and conventional here?  Presumably use
> > symmetrical do{}while(0) things, neither of which is an lval, both of
> > which don't return anything.
> >
> 
> Hi,
> 
> The 'dd_key_init()' macro is being used here to help initialize
> the 'key' field in the 'struct _ddebug', and its not being used as a
> statement.
> 
> In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
> the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
> in the structure (to conserve space).

Well yeah.  And it's doing it wrongly, isn't it?

: @@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug
:  		.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)

Shouldn't it be ".key = (init)"?

Anyway, it's odd-looking.  I guess something like

#define DD_KEY_INIT(init)	.key = (init)

would be more idiomatic.

Re: [PATCH v3 7/7] dynamic_debug: add jump label support

[Jason Baron]

On 07/11/2016 03:23 PM, Andrew Morton wrote:
> On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron <jbaron@akamai.com> wrote:
>
>> On 07/08/2016 05:41 PM, Andrew Morton wrote:
>>> On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> 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.
>>>>
>>>> ...
>>>>
>>>> +#ifdef HAVE_JUMP_LABEL
>>>> +
>>>> +#define dd_key_init(key, init) key = (init)
>>>>
>>>> ...
>>>>
>>>> +#else
>>>> +
>>>> +#define dd_key_init(key, init)
>>>> +
>>> umm, lazy.  One is an lval and returns a value and the other does
>>> neither.  Lack of parenthesization in the first version doubtless
>>> exposes various horrors.
>>>
>>> Care to do something more robust and conventional here?  Presumably use
>>> symmetrical do{}while(0) things, neither of which is an lval, both of
>>> which don't return anything.
>>>
>> Hi,
>>
>> The 'dd_key_init()' macro is being used here to help initialize
>> the 'key' field in the 'struct _ddebug', and its not being used as a
>> statement.
>>
>> In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
>> the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
>> in the structure (to conserve space).
> Well yeah.  And it's doing it wrongly, isn't it?
>
> : @@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug
> :  		.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)
>
> Shouldn't it be ".key = (init)"?
>
> Anyway, it's odd-looking.  I guess something like
>
> #define DD_KEY_INIT(init)	.key = (init)
>
> would be more idiomatic.

Ok, so the 'key' field is a union and so the patch is effectively
calling (after substitution):

dd_key_init(.key.dd_key_true, STATIC_KEY_TRUE_INIT)

and:

dd_key_init(.key.dd_key_true, STATIC_KEY_FALSE_INIT)

So we could have variations such as:

#define dd_key_true_init() .key.dd_key_true = (STATIC_KEY_TRUE_INIT)
#define dd_key_false_init()

and

#define dd_key_true_init()
#define dd_key_false_init() .key.dd_key_false = (STATIC_KEY_FALSE_INIT)

and finally:

#define dd_key_true_init()
#define dd_key_false_init()

and then have both dd_key_true_init()  and dd_key_fase_init() in
the structure definition. It adds a bunch more definitions and I'm
not sure if its more readable, but maybe it would look cleaner?

Thanks,

-Jason