[PATCH v6 0/8] implement DYNAMIC_DEBUG_RELATIVE_POINTERS

[PATCH v6 0/8] implement DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

Similar to CONFIG_GENERIC_BUG_RELATIVE_POINTERS that replaces (8 byte)
const char* members by (4 byte) signed offsets from the bug_entry,
this implements the similar thing for struct _ddebug, the descriptors
underlying pr_debug() and friends in a CONFIG_DYNAMIC_DEBUG kernel.

Since struct _ddebug has four string members, we save 16 byte per
instance. The total savings seem to be comparable to what is saved by
an architecture selecting GENERIC_BUG_RELATIVE_POINTERS (see patch 8
for some numbers for a common distro config).

While refreshing these patches, which were orignally just targeted at
x86-64, it occured to me that despite the implementation relying on
inline asm, there's nothing x86 specific about it, and indeed it seems
to work out-of-the-box for ppc64 and arm64 as well, but those have
only been compile-tested.

The first 6 patches are rather pedestrian preparations. The fun stuff
is in patch 7, and the last patch is just the minimal boilerplate to
hook up the asm-generic header and selecting
HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS on x86-64. Doing arm64 and ppc64
would be similar 2-line patches.

Unfortunately, the "fun stuff" includes some magic that breaks the
build in some corner cases with older gcc (and some not-so-magic that
clang only supports since version 9 for non-x86 targets). So in this
v6, the config option DYNAMIC_DEBUG_RELATIVE_POINTERS has been made
opt-in, depending on the arch-selected
HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS, instead of the arch selecting it
directly. Also, I've set a lower bound for the gcc and clang versions,
so that hopefully nobody should be able to select the option and have
the build break.


Rasmus Villemoes (8):
  linux/device.h: use unique identifier for each struct _ddebug
  linux/net.h: use unique identifier for each struct _ddebug
  linux/printk.h: use unique identifier for each struct _ddebug
  dynamic_debug: introduce accessors for string members of struct
    _ddebug
  dynamic_debug: drop use of bitfields in struct _ddebug
  dynamic_debug: introduce CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
  dynamic_debug: add asm-generic implementation for
    DYNAMIC_DEBUG_RELATIVE_POINTERS
  x86-64: select HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS

 arch/x86/Kconfig                            |   1 +
 arch/x86/entry/vdso/vdso32/vclock_gettime.c |   1 +
 arch/x86/include/asm/Kbuild                 |   1 +
 include/asm-generic/dynamic_debug.h         | 116 ++++++++++++++++++++
 include/linux/device.h                      |   4 +-
 include/linux/dynamic_debug.h               |  26 +++--
 include/linux/jump_label.h                  |   2 +
 include/linux/net.h                         |   4 +-
 include/linux/printk.h                      |   4 +-
 lib/Kconfig.debug                           |  16 +++
 lib/dynamic_debug.c                         | 111 ++++++++++++++-----
 11 files changed, 246 insertions(+), 40 deletions(-)
 create mode 100644 include/asm-generic/dynamic_debug.h

-- 
2.20.1

[PATCH v6 1/8] linux/device.h: use unique identifier for each struct _ddebug

[Rasmus Villemoes]

Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for dev_dbg_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.

No functional change.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Jason Baron <jbaron@akamai.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/device.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 848fc71c6ba6..eaba6952c2b3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1563,7 +1563,7 @@ do {									\
 	dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
 #if defined(CONFIG_DYNAMIC_DEBUG)
 /* descriptor check is first to prevent flooding with "callbacks suppressed" */
-#define dev_dbg_ratelimited(dev, fmt, ...)				\
+#define _dev_dbg_ratelimited(descriptor, dev, fmt, ...)			\
 do {									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
 				      DEFAULT_RATELIMIT_INTERVAL,	\
@@ -1574,6 +1574,8 @@ do {									\
 		__dynamic_dev_dbg(&descriptor, dev, dev_fmt(fmt),	\
 				  ##__VA_ARGS__);			\
 } while (0)
+#define dev_dbg_ratelimited(dev, fmt, ...)				\
+	_dev_dbg_ratelimited(__UNIQUE_ID(ddebug), dev, fmt, ##__VA_ARGS__)
 #elif defined(DEBUG)
 #define dev_dbg_ratelimited(dev, fmt, ...)				\
 do {									\
-- 
2.20.1

[PATCH v6 2/8] linux/net.h: use unique identifier for each struct _ddebug

[Rasmus Villemoes]

Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for net_dbg_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.

No functional change.

Cc: netdev@vger.kernel.org
Acked-by: Jason Baron <jbaron@akamai.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/net.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/net.h b/include/linux/net.h
index f7d672cf25b5..a080ede95b47 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -264,7 +264,7 @@ do {								\
 #define net_info_ratelimited(fmt, ...)				\
 	net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
 #if defined(CONFIG_DYNAMIC_DEBUG)
-#define net_dbg_ratelimited(fmt, ...)					\
+#define _net_dbg_ratelimited(descriptor, fmt, ...)			\
 do {									\
 	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);			\
 	if (DYNAMIC_DEBUG_BRANCH(descriptor) &&				\
@@ -272,6 +272,8 @@ do {									\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),		\
 		                   ##__VA_ARGS__);			\
 } while (0)
+#define net_dbg_ratelimited(fmt, ...)					\
+	_net_dbg_ratelimited(__UNIQUE_ID(ddebug), fmt, ##__VA_ARGS__)
 #elif defined(DEBUG)
 #define net_dbg_ratelimited(fmt, ...)				\
 	net_ratelimited_function(pr_debug, fmt, ##__VA_ARGS__)
-- 
2.20.1

[PATCH v6 3/8] linux/printk.h: use unique identifier for each struct _ddebug

[Rasmus Villemoes]

Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for pr_debug_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.

No functional change.

Acked-by: Petr Mladek <pmladek@suse.com>
Acked-by: Jason Baron <jbaron@akamai.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/printk.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index cefd374c47b1..54499c1a74fd 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -456,7 +456,7 @@ extern int kptr_restrict;
 /* If you are writing a driver, please use dev_dbg instead */
 #if defined(CONFIG_DYNAMIC_DEBUG)
 /* descriptor check is first to prevent flooding with "callbacks suppressed" */
-#define pr_debug_ratelimited(fmt, ...)					\
+#define _pr_debug_ratelimited(descriptor, fmt, ...)			\
 do {									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
 				      DEFAULT_RATELIMIT_INTERVAL,	\
@@ -466,6 +466,8 @@ do {									\
 	    __ratelimit(&_rs))						\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);	\
 } while (0)
+#define pr_debug_ratelimited(fmt, ...)		\
+	_pr_debug_ratelimited(__UNIQUE_ID(ddebug), fmt, ##__VA_ARGS__)
 #elif defined(DEBUG)
 #define pr_debug_ratelimited(fmt, ...)					\
 	printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
-- 
2.20.1

[PATCH v6 4/8] dynamic_debug: introduce accessors for string members of struct _ddebug

[Rasmus Villemoes]

When we introduce compact versions of these pointers (a la
CONFIG_GENERIC_BUG_RELATIVE_POINTERS), all access to these members must
go via appropriate accessors. This just mass-converts dynamic_debug.c to
use the new accessors.

Acked-by: Jason Baron <jbaron@akamai.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/dynamic_debug.c | 51 ++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8a16c2d498e9..3599c4c0361a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -39,6 +39,23 @@
 
 #include <rdma/ib_verbs.h>
 
+static inline const char *dd_modname(const struct _ddebug *dd)
+{
+	return dd->modname;
+}
+static inline const char *dd_function(const struct _ddebug *dd)
+{
+	return dd->function;
+}
+static inline const char *dd_filename(const struct _ddebug *dd)
+{
+	return dd->filename;
+}
+static inline const char *dd_format(const struct _ddebug *dd)
+{
+	return dd->format;
+}
+
 extern struct _ddebug __start___verbose[];
 extern struct _ddebug __stop___verbose[];
 
@@ -160,21 +177,21 @@ static int ddebug_change(const struct ddebug_query *query,
 
 			/* match against the source filename */
 			if (query->filename &&
-			    !match_wildcard(query->filename, dp->filename) &&
+			    !match_wildcard(query->filename, dd_filename(dp)) &&
 			    !match_wildcard(query->filename,
-					   kbasename(dp->filename)) &&
+					   kbasename(dd_filename(dp))) &&
 			    !match_wildcard(query->filename,
-					   trim_prefix(dp->filename)))
+					   trim_prefix(dd_filename(dp))))
 				continue;
 
 			/* match against the function */
 			if (query->function &&
-			    !match_wildcard(query->function, dp->function))
+			    !match_wildcard(query->function, dd_function(dp)))
 				continue;
 
 			/* match against the format */
 			if (query->format &&
-			    !strstr(dp->format, query->format))
+			    !strstr(dd_format(dp), query->format))
 				continue;
 
 			/* match against the line number range */
@@ -199,8 +216,8 @@ static int ddebug_change(const struct ddebug_query *query,
 #endif
 			dp->flags = newflags;
 			vpr_info("changed %s:%d [%s]%s =%s\n",
-				 trim_prefix(dp->filename), dp->lineno,
-				 dt->mod_name, dp->function,
+				 trim_prefix(dd_filename(dp)), dp->lineno,
+				 dt->mod_name, dd_function(dp),
 				 ddebug_describe_flags(dp, flagbuf,
 						       sizeof(flagbuf)));
 		}
@@ -535,10 +552,10 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 	pos_after_tid = pos;
 	if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
 		pos += snprintf(buf + pos, remaining(pos), "%s:",
-				desc->modname);
+				dd_modname(desc));
 	if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
 		pos += snprintf(buf + pos, remaining(pos), "%s:",
-				desc->function);
+				dd_function(desc));
 	if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
 		pos += snprintf(buf + pos, remaining(pos), "%d:",
 				desc->lineno);
@@ -827,10 +844,10 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	}
 
 	seq_printf(m, "%s:%u [%s]%s =%s \"",
-		   trim_prefix(dp->filename), dp->lineno,
-		   iter->table->mod_name, dp->function,
+		   trim_prefix(dd_filename(dp)), dp->lineno,
+		   iter->table->mod_name, dd_function(dp),
 		   ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
-	seq_escape(m, dp->format, "\t\r\n\"");
+	seq_escape(m, dd_format(dp), "\t\r\n\"");
 	seq_puts(m, "\"\n");
 
 	return 0;
@@ -1024,20 +1041,20 @@ static int __init dynamic_debug_init(void)
 		return 1;
 	}
 	iter = __start___verbose;
-	modname = iter->modname;
+	modname = dd_modname(iter);
 	iter_start = iter;
 	for (; iter < __stop___verbose; iter++) {
 		entries++;
-		verbose_bytes += strlen(iter->modname) + strlen(iter->function)
-			+ strlen(iter->filename) + strlen(iter->format);
+		verbose_bytes += strlen(dd_modname(iter)) + strlen(dd_function(iter))
+			+ strlen(dd_filename(iter)) + strlen(dd_format(iter));
 
-		if (strcmp(modname, iter->modname)) {
+		if (strcmp(modname, dd_modname(iter))) {
 			modct++;
 			ret = ddebug_add_module(iter_start, n, modname);
 			if (ret)
 				goto out_err;
 			n = 0;
-			modname = iter->modname;
+			modname = dd_modname(iter);
 			iter_start = iter;
 		}
 		n++;
-- 
2.20.1

[PATCH v6 5/8] dynamic_debug: drop use of bitfields in struct _ddebug

[Rasmus Villemoes]

Properly initializing a struct containing bitfields in assembly is
hard. Instead, merge lineno and flags to a single unsigned int, which we
mask manually. This should not cause any worse code than what gcc would
need to generate for accessing the bitfields anyway.

Actually, on 64 bit, there is a four byte hole after these fields, so we
could just give flags and lineno each their own u32. But I don't think
that's worth the ifdeffery.

Acked-by: Jason Baron <jbaron@akamai.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/dynamic_debug.h | 12 ++++------
 lib/dynamic_debug.c           | 44 +++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 6c809440f319..7d6d0153096e 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -20,7 +20,6 @@ struct _ddebug {
 	const char *function;
 	const char *filename;
 	const char *format;
-	unsigned int lineno:18;
 	/*
 	 * The flags field controls the behaviour at the callsite.
 	 * The bits here are changed dynamically when the user
@@ -37,7 +36,7 @@ struct _ddebug {
 #else
 #define _DPRINTK_FLAGS_DEFAULT 0
 #endif
-	unsigned int flags:8;
+	unsigned int flags_lineno; /* flags in lower 8 bits, lineno in upper 24 */
 #ifdef CONFIG_JUMP_LABEL
 	union {
 		struct static_key_true dd_key_true;
@@ -46,7 +45,7 @@ struct _ddebug {
 #endif
 } __attribute__((aligned(8)));
 
-
+#define _DPRINTK_FLAGS_LINENO_INIT (((unsigned)__LINE__ << 8) | _DPRINTK_FLAGS_DEFAULT)
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
 int ddebug_add_module(struct _ddebug *tab, unsigned int n,
@@ -85,8 +84,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 		.function = __func__,				\
 		.filename = __FILE__,				\
 		.format = (fmt),				\
-		.lineno = __LINE__,				\
-		.flags = _DPRINTK_FLAGS_DEFAULT,		\
+		.flags_lineno = _DPRINTK_FLAGS_LINENO_INIT,	\
 		_DPRINTK_KEY_INIT				\
 	}
 
@@ -111,10 +109,10 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 
 #ifdef DEBUG
 #define DYNAMIC_DEBUG_BRANCH(descriptor) \
-	likely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+	likely(descriptor.flags_lineno & _DPRINTK_FLAGS_PRINT)
 #else
 #define DYNAMIC_DEBUG_BRANCH(descriptor) \
-	unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+	unlikely(descriptor.flags_lineno & _DPRINTK_FLAGS_PRINT)
 #endif
 
 #endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 3599c4c0361a..3a1a80041cd6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -55,6 +55,18 @@ static inline const char *dd_format(const struct _ddebug *dd)
 {
 	return dd->format;
 }
+static inline unsigned dd_lineno(const struct _ddebug *dd)
+{
+	return dd->flags_lineno >> 8;
+}
+static inline unsigned dd_flags(const struct _ddebug *dd)
+{
+	return dd->flags_lineno & 0xff;
+}
+static inline void dd_set_flags(struct _ddebug *dd, unsigned newflags)
+{
+	dd->flags_lineno = (dd_lineno(dd) << 8) | newflags;
+}
 
 extern struct _ddebug __start___verbose[];
 extern struct _ddebug __stop___verbose[];
@@ -113,7 +125,7 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
 
 	BUG_ON(maxlen < 6);
 	for (i = 0; i < ARRAY_SIZE(opt_array); ++i)
-		if (dp->flags & opt_array[i].flag)
+		if (dd_flags(dp) & opt_array[i].flag)
 			*p++ = opt_array[i].opt_char;
 	if (p == buf)
 		*p++ = '_';
@@ -159,7 +171,7 @@ static int ddebug_change(const struct ddebug_query *query,
 {
 	int i;
 	struct ddebug_table *dt;
-	unsigned int newflags;
+	unsigned int newflags, oldflags;
 	unsigned int nfound = 0;
 	char flagbuf[10];
 
@@ -196,27 +208,28 @@ static int ddebug_change(const struct ddebug_query *query,
 
 			/* match against the line number range */
 			if (query->first_lineno &&
-			    dp->lineno < query->first_lineno)
+			    dd_lineno(dp) < query->first_lineno)
 				continue;
 			if (query->last_lineno &&
-			    dp->lineno > query->last_lineno)
+			    dd_lineno(dp) > query->last_lineno)
 				continue;
 
 			nfound++;
 
-			newflags = (dp->flags & mask) | flags;
-			if (newflags == dp->flags)
+			oldflags = dd_flags(dp);
+			newflags = (oldflags & mask) | flags;
+			if (newflags == oldflags)
 				continue;
 #ifdef CONFIG_JUMP_LABEL
-			if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+			if (oldflags & _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;
+			dd_set_flags(dp, newflags);
 			vpr_info("changed %s:%d [%s]%s =%s\n",
-				 trim_prefix(dd_filename(dp)), dp->lineno,
+				 trim_prefix(dd_filename(dp)), dd_lineno(dp),
 				 dt->mod_name, dd_function(dp),
 				 ddebug_describe_flags(dp, flagbuf,
 						       sizeof(flagbuf)));
@@ -539,10 +552,11 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 {
 	int pos_after_tid;
 	int pos = 0;
+	unsigned flags = dd_flags(desc);
 
 	*buf = '\0';
 
-	if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
+	if (flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
 			pos += snprintf(buf + pos, remaining(pos), "<intr> ");
 		else
@@ -550,15 +564,15 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 					task_pid_vnr(current));
 	}
 	pos_after_tid = pos;
-	if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+	if (flags & _DPRINTK_FLAGS_INCL_MODNAME)
 		pos += snprintf(buf + pos, remaining(pos), "%s:",
 				dd_modname(desc));
-	if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+	if (flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
 		pos += snprintf(buf + pos, remaining(pos), "%s:",
 				dd_function(desc));
-	if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
+	if (flags & _DPRINTK_FLAGS_INCL_LINENO)
 		pos += snprintf(buf + pos, remaining(pos), "%d:",
-				desc->lineno);
+				dd_lineno(desc));
 	if (pos - pos_after_tid)
 		pos += snprintf(buf + pos, remaining(pos), " ");
 	if (pos >= PREFIX_SIZE)
@@ -844,7 +858,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	}
 
 	seq_printf(m, "%s:%u [%s]%s =%s \"",
-		   trim_prefix(dd_filename(dp)), dp->lineno,
+		   trim_prefix(dd_filename(dp)), dd_lineno(dp),
 		   iter->table->mod_name, dd_function(dp),
 		   ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
 	seq_escape(m, dd_format(dp), "\t\r\n\"");
-- 
2.20.1

[PATCH v6 6/8] dynamic_debug: introduce CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

Based on the same idea for struct bug_entry, one can use relative
pointers in struct _ddebug. It only makes sense for 64 bit
architectures, where one saves 16 bytes per entry (out of 40 or 56,
depending on CONFIG_JUMP_LABEL).

Unlike the bug_entry case, this turns out not to work with some older
toolchains, so the actual config option must be set by the user and
not just selected by the various architectures. Still, the
architecture must select HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS and is
then responsible for providing a suitable
DEFINE_DYNAMIC_DEBUG_METADATA macro in <asm/dynamic_debug.h>.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/dynamic_debug.h | 14 ++++++++++++++
 lib/Kconfig.debug             | 13 +++++++++++++
 lib/dynamic_debug.c           | 20 ++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 7d6d0153096e..572921e880ec 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -16,10 +16,17 @@ struct _ddebug {
 	 * These fields are used to drive the user interface
 	 * for selecting and displaying debug callsites.
 	 */
+#ifdef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
+	signed int modname_disp;
+	signed int function_disp;
+	signed int filename_disp;
+	signed int format_disp;
+#else
 	const char *modname;
 	const char *function;
 	const char *filename;
 	const char *format;
+#endif
 	/*
 	 * The flags field controls the behaviour at the callsite.
 	 * The bits here are changed dynamically when the user
@@ -77,6 +84,12 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 			 const struct ib_device *ibdev,
 			 const char *fmt, ...);
 
+#ifdef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
+#include <asm/dynamic_debug.h>
+#ifndef DEFINE_DYNAMIC_DEBUG_METADATA
+# error "asm/dynamic_debug.h must provide definition of DEFINE_DYNAMIC_DEBUG_METADATA"
+#endif
+#else
 #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
 	static struct _ddebug  __aligned(8)			\
 	__attribute__((section("__verbose"))) name = {		\
@@ -87,6 +100,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 		.flags_lineno = _DPRINTK_FLAGS_LINENO_INIT,	\
 		_DPRINTK_KEY_INIT				\
 	}
+#endif
 
 #ifdef CONFIG_JUMP_LABEL
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index cbdfae379896..f3d2e234c15f 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,19 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS
+	bool
+
+config DYNAMIC_DEBUG_RELATIVE_POINTERS
+	bool "Reduce size of dynamic debug metadata"
+	depends on DYNAMIC_DEBUG
+	depends on HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS
+	help
+	  If you say Y here, the static memory footprint of the kernel
+	  image will be reduced somewhat (about 40K for a typical
+	  distro kernel). There is no performance difference either
+	  way.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 3a1a80041cd6..f1646795692c 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -39,6 +39,24 @@
 
 #include <rdma/ib_verbs.h>
 
+#ifdef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
+static inline const char *dd_modname(const struct _ddebug *dd)
+{
+	return (const char *)dd + dd->modname_disp;
+}
+static inline const char *dd_function(const struct _ddebug *dd)
+{
+	return (const char *)dd + dd->function_disp;
+}
+static inline const char *dd_filename(const struct _ddebug *dd)
+{
+	return (const char *)dd + dd->filename_disp;
+}
+static inline const char *dd_format(const struct _ddebug *dd)
+{
+	return (const char *)dd + dd->format_disp;
+}
+#else
 static inline const char *dd_modname(const struct _ddebug *dd)
 {
 	return dd->modname;
@@ -55,6 +73,8 @@ static inline const char *dd_format(const struct _ddebug *dd)
 {
 	return dd->format;
 }
+#endif
+
 static inline unsigned dd_lineno(const struct _ddebug *dd)
 {
 	return dd->flags_lineno >> 8;
-- 
2.20.1

[PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

A 64 bit architecture can allow reducing the size of the kernel image by
selecting HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS, but it must provide
a proper DEFINE_DYNAMIC_DEBUG_METADATA macro for emitting the struct
_ddebug in assembly. However, since that does not involve any
instructions, this generic implementation should be usable by most if
not all.

It relies on

(1) standard assembly directives that should work on
all architectures
(2) the "i" constraint for an constant, and
(3) %cN emitting the constant operand N without punctuation

and of course the layout of _ddebug being what one expects.

Now, clang before 9.0 doesn't satisfy (3) for non-x86 targets.

Moreover, it turns out that gcc 4.8 in some corner cases emits (dead) code
which ends up causing the link to fail (emitting a reference to the
dynamic debug descriptor without emitting the assembly that defines
it).

Hence, make the config option available only for relatively new
compilers - I have not been able to reproduce the gcc problem with any
newer versions, and this implementation has been in -next for a whole
cycle without other problems reported.

I've succesfully built x86-64, arm64 and ppc64 defconfig +
CONFIG_DYNAMIC_DEBUG + patches to hook up this header, and boot-tested
the x86-64 one.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/asm-generic/dynamic_debug.h | 116 ++++++++++++++++++++++++++++
 include/linux/jump_label.h          |   2 +
 lib/Kconfig.debug                   |   5 +-
 3 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100644 include/asm-generic/dynamic_debug.h

diff --git a/include/asm-generic/dynamic_debug.h b/include/asm-generic/dynamic_debug.h
new file mode 100644
index 000000000000..f1dd3019cd98
--- /dev/null
+++ b/include/asm-generic/dynamic_debug.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_DYNAMIC_DEBUG_H
+#define _ASM_GENERIC_DYNAMIC_DEBUG_H
+
+#ifndef _DYNAMIC_DEBUG_H
+#error "don't include asm/dynamic_debug.h directly"
+#endif
+
+#include <linux/build_bug.h>
+#ifdef CONFIG_JUMP_LABEL
+#include <linux/jump_label.h>
+#endif
+
+/*
+ * We need to know the exact layout of struct _ddebug in order to
+ * initialize it in assembly. Check that all members are at expected
+ * offsets - if any of these fail, the arch cannot use this generic
+ * dynamic_debug.h. DYNAMIC_DEBUG_RELATIVE_POINTERS is pointless for
+ * !64BIT, so we expect the static_key to be at an 8-byte boundary
+ * since it contains stuff which is long-aligned.
+ */
+
+static_assert(BITS_PER_LONG == 64);
+static_assert(offsetof(struct _ddebug, modname_disp)  == 0);
+static_assert(offsetof(struct _ddebug, function_disp) == 4);
+static_assert(offsetof(struct _ddebug, filename_disp) == 8);
+static_assert(offsetof(struct _ddebug, format_disp)   == 12);
+static_assert(offsetof(struct _ddebug, flags_lineno)  == 16);
+
+#ifdef CONFIG_JUMP_LABEL
+static_assert(offsetof(struct _ddebug, key)        == 24);
+static_assert(offsetof(struct static_key, enabled) == 0);
+static_assert(offsetof(struct static_key, type)    == 8);
+
+/* <4 bytes padding>, .enabled, <4 bytes padding>, .type */
+# ifdef DEBUG
+# define _DPRINTK_ASM_KEY_INIT \
+	"\t.int 0\n" "\t.int 1\n" "\t.int 0\n" "\t.quad "__stringify(__JUMP_TYPE_TRUE)"\n"
+# else
+# define _DPRINTK_ASM_KEY_INIT \
+	"\t.int 0\n" "\t.int 0\n" "\t.int 0\n" "\t.quad "__stringify(__JUMP_TYPE_FALSE)"\n"
+# endif
+#else /* !CONFIG_JUMP_LABEL */
+#define _DPRINTK_ASM_KEY_INIT ""
+#endif
+
+/*
+ * There's a bit of magic involved here.
+ *
+ * First, unlike the bug table entries, we need to define an object in
+ * assembly which we can reference from C code (for use by the
+ * DYNAMIC_DEBUG_BRANCH macro), but we don't want 'name' to have
+ * external linkage (that would require use of globally unique
+ * identifiers, which we can't guarantee). Fortunately, the "extern"
+ * keyword just tells the compiler that _somebody_ provides that
+ * symbol - usually that somebody is the linker, but in this case it's
+ * the assembler, and since we do not do .globl name, the symbol gets
+ * internal linkage.
+ *
+ * So far so good. The next problem is that there's no scope in
+ * assembly, so the identifier 'name' has to be unique within each
+ * translation unit - otherwise all uses of that identifier end up
+ * referring to the same struct _ddebug instance. pr_debug and friends
+ * do this by use of indirection and __UNIQUE_ID(), and new users of
+ * DEFINE_DYNAMIC_DEBUG_METADATA() should do something similar. We
+ * need to catch cases where this is not done at build time.
+ *
+ * With assembly-level .ifndef we can ensure that we only define a
+ * given identifier once, preventing "symbol 'foo' already defined"
+ * errors. But we still need to detect and fail on multiple uses of
+ * the same identifer. The simplest, and wrong, solution to that is to
+ * add an .else .error branch to the .ifndef. The problem is that just
+ * because the DEFINE_DYNAMIC_DEBUG_METADATA() macro is only expanded
+ * once with a given identifier, the compiler may emit the assembly
+ * code multiple times, e.g. if the macro appears in an inline
+ * function. Now, in a normal case like
+ *
+ *   static inline get_next_id(void) { static int v; return ++v; }
+ *
+ * all inlined copies of get_next_id are _supposed_ to refer to the
+ * same object 'v'. So we do need to allow this chunk of assembly to
+ * appear multiple times with the same 'name', as long as they all
+ * came from the same DEFINE_DYNAMIC_DEBUG_METADATA() instance. To do
+ * that, we pass __COUNTER__ to the asm(), and set an assembler symbol
+ * name.ddebug.once to that value when we first define 'name'. When we
+ * meet a second attempt at defining 'name', we compare
+ * name.ddebug.once to %6 and error out if they are different.
+ */
+
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)			\
+	extern struct _ddebug name;					\
+	asm volatile(".ifndef " __stringify(name) "\n"			\
+		     ".pushsection __verbose,\"aw\"\n"			\
+		     ".type "__stringify(name)", STT_OBJECT\n"		\
+		     ".size "__stringify(name)", %c5\n"			\
+		     "1:\n"						\
+		     __stringify(name) ":\n"				\
+		     "\t.int %c0 - 1b /* _ddebug::modname_disp */\n"	\
+		     "\t.int %c1 - 1b /* _ddebug::function_disp */\n"	\
+		     "\t.int %c2 - 1b /* _ddebug::filename_disp */\n"	\
+		     "\t.int %c3 - 1b /* _ddebug::format_disp */\n"	\
+		     "\t.int %c4      /* _ddebug::flags_lineno */\n"	\
+		     _DPRINTK_ASM_KEY_INIT				\
+		     "\t.org 1b+%c5\n"					\
+		     ".popsection\n"					\
+		     ".set "__stringify(name)".ddebug.once, %c6\n"	\
+		     ".elseif "__stringify(name)".ddebug.once - %c6\n"	\
+		     ".line "__stringify(__LINE__) " - 1\n"             \
+		     ".error \"'"__stringify(name)"' used as _ddebug identifier more than once\"\n" \
+		     ".endif\n"						\
+		     : : "i" (KBUILD_MODNAME), "i" (__func__),		\
+		       "i" (__FILE__), "i" (fmt),			\
+		       "i" (_DPRINTK_FLAGS_LINENO_INIT),		\
+		       "i" (sizeof(struct _ddebug)), "i" (__COUNTER__))
+
+#endif /* _ASM_GENERIC_DYNAMIC_DEBUG_H */
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 3e113a1fa0f1..2b027d2ef3d0 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -190,6 +190,8 @@ struct module;
 
 #ifdef CONFIG_JUMP_LABEL
 
+#define __JUMP_TYPE_FALSE	0
+#define __JUMP_TYPE_TRUE	1
 #define JUMP_TYPE_FALSE		0UL
 #define JUMP_TYPE_TRUE		1UL
 #define JUMP_TYPE_LINKED	2UL
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index f3d2e234c15f..7f2bc0175b88 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -171,11 +171,14 @@ config DYNAMIC_DEBUG_RELATIVE_POINTERS
 	bool "Reduce size of dynamic debug metadata"
 	depends on DYNAMIC_DEBUG
 	depends on HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS
+	depends on (GCC_VERSION >= 50000 || CLANG_VERSION >= 90000)
 	help
 	  If you say Y here, the static memory footprint of the kernel
 	  image will be reduced somewhat (about 40K for a typical
 	  distro kernel). There is no performance difference either
-	  way.
+	  way. This relies on some compile-time magic, so if your
+	  toolchain fails to build the kernel with this option, just
+	  say N.
 
 endmenu # "printk and dmesg options"
 
-- 
2.20.1

[PATCH v6 8/8] x86-64: select HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

This allows reducing the size of struct _ddebug from 56 to 40
bytes. There's one such struct for each pr_debug(), netdev_debug()
etc. in a CONFIG_DYNAMIC_DEBUG kernel. An Ubuntu 4.15 kernel has about
2550 entries in the __verbose section of vmlinux, amounting to ~40K
saved. (Modules also become smaller, but it's harder to quantify how
much that yields at runtime.)

For comparison, the __bug_table section of that Ubuntu kernel is 75576
bytes, i.e. 6298 12-byte bug_entrys, so GENERIC_BUG_RELATIVE_POINTERS
saves ~50K.

Due to the build-time sanity checks in asm-generic/dynamic_debug.h, we
need to add another #undef to vclock_gettime.c.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/x86/Kconfig                            | 1 +
 arch/x86/entry/vdso/vdso32/vclock_gettime.c | 1 +
 arch/x86/include/asm/Kbuild                 | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2bbbd4d1ba31..ed44ea2b9556 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -30,6 +30,7 @@ config X86_64
 	select NEED_DMA_MAP_STATE
 	select SWIOTLB
 	select ARCH_HAS_SYSCALL_WRAPPER
+	select HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS
 
 config FORCE_DYNAMIC_FTRACE
 	def_bool y
diff --git a/arch/x86/entry/vdso/vdso32/vclock_gettime.c b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
index 9242b28418d5..9acec4426206 100644
--- a/arch/x86/entry/vdso/vdso32/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
@@ -17,6 +17,7 @@
 #undef CONFIG_ILLEGAL_POINTER_VALUE
 #undef CONFIG_SPARSEMEM_VMEMMAP
 #undef CONFIG_NR_CPUS
+#undef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
 
 #define CONFIG_X86_32 1
 #define CONFIG_PGTABLE_LEVELS 2
diff --git a/arch/x86/include/asm/Kbuild b/arch/x86/include/asm/Kbuild
index 8b52bc5ddf69..3b630b4d37d7 100644
--- a/arch/x86/include/asm/Kbuild
+++ b/arch/x86/include/asm/Kbuild
@@ -8,6 +8,7 @@ generated-y += unistd_64_x32.h
 generated-y += xen-hypercalls.h
 
 generic-y += dma-contiguous.h
+generic-y += dynamic_debug.h
 generic-y += early_ioremap.h
 generic-y += export.h
 generic-y += mcs_spinlock.h
-- 
2.20.1

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Nick Desaulniers]

On Mon, Jun 17, 2019 at 3:20 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> A 64 bit architecture can allow reducing the size of the kernel image by
> selecting HAVE_DYNAMIC_DEBUG_RELATIVE_POINTERS, but it must provide
> a proper DEFINE_DYNAMIC_DEBUG_METADATA macro for emitting the struct
> _ddebug in assembly. However, since that does not involve any
> instructions, this generic implementation should be usable by most if
> not all.
>
> It relies on
>
> (1) standard assembly directives that should work on
> all architectures
> (2) the "i" constraint for an constant, and
> (3) %cN emitting the constant operand N without punctuation
>
> and of course the layout of _ddebug being what one expects.
>
> Now, clang before 9.0 doesn't satisfy (3) for non-x86 targets.

Thanks so much for resending with this case fixed, and sorry I did not
implement (3) sooner!  I appreciate your patience.
Acked-by: Nick Desaulniers <ndesaulniers@google.com>

I'm happy to help test this series, do you have a tree I could pull
these from quickly?  Anything I should test at runtime besides a boot
test?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 2/8] linux/net.h: use unique identifier for each struct _ddebug

[David Miller]

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date: Tue, 18 Jun 2019 00:20:28 +0200

> Changes on x86-64 later in this series require that all struct _ddebug
> descriptors in a translation unit uses distinct identifiers. Realize
> that for net_dbg_ratelimited by generating such an identifier via
> __UNIQUE_ID and pass that to an extra level of macros.
> 
> No functional change.
> 
> Cc: netdev@vger.kernel.org
> Acked-by: Jason Baron <jbaron@akamai.com>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

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

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

On 18/06/2019 00.35, Nick Desaulniers wrote:
> On Mon, Jun 17, 2019 at 3:20 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> It relies on
>>
>> (1) standard assembly directives that should work on
>> all architectures
>> (2) the "i" constraint for an constant, and
>> (3) %cN emitting the constant operand N without punctuation
>>
>> and of course the layout of _ddebug being what one expects.
>>
>> Now, clang before 9.0 doesn't satisfy (3) for non-x86 targets.
> 
> Thanks so much for resending with this case fixed, and sorry I did not
> implement (3) sooner!  I appreciate your patience.
> Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> I'm happy to help test this series, do you have a tree I could pull
> these from quickly? 

I've pushed them to https://github.com/Villemoes/linux/tree/dyndebug_v6
. They rebase pretty cleanly to just about anything you might prefer
testing on. Enabling it for arm64 or ppc64 is a trivial two-liner
similar to the x86 patch (and similar to the previous patches for those
arches). Thanks for volunteering to test this :)

> Anything I should test at runtime besides a boot
> test?

Well, apart from booting, I've mostly just tested that the debugfs
control file is identical before and after enabling relative pointers,
and that enabling/disabling various pr_debug()s by writing to the
control file takes effect. I should only be changing the format for
storing the metadata in the kernel image, so I think that should be enough.

While this is still not merged, some new user of one of the string
members could creep in, but that should be caught at build time.

Rasmus

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Nick Desaulniers]

On Thu, Jun 20, 2019 at 1:46 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 18/06/2019 00.35, Nick Desaulniers wrote:
> > On Mon, Jun 17, 2019 at 3:20 PM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >>
> >> It relies on
> >>
> >> (1) standard assembly directives that should work on
> >> all architectures
> >> (2) the "i" constraint for an constant, and
> >> (3) %cN emitting the constant operand N without punctuation
> >>
> >> and of course the layout of _ddebug being what one expects.
> >>
> >> Now, clang before 9.0 doesn't satisfy (3) for non-x86 targets.
> >
> > Thanks so much for resending with this case fixed, and sorry I did not
> > implement (3) sooner!  I appreciate your patience.
> > Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > I'm happy to help test this series, do you have a tree I could pull
> > these from quickly?
>
> I've pushed them to https://github.com/Villemoes/linux/tree/dyndebug_v6
> . They rebase pretty cleanly to just about anything you might prefer
> testing on. Enabling it for arm64 or ppc64 is a trivial two-liner
> similar to the x86 patch (and similar to the previous patches for those
> arches). Thanks for volunteering to test this :)

Compile tested x86_64 allyesconfig
boot tested x86_64 defconfig+CONFIG_DYNAMIC_DEBUG

(just curious why the Kconfig changes for arm64 or ppc64 aren't
included in this set?)

>
> > Anything I should test at runtime besides a boot
> > test?
>
> Well, apart from booting, I've mostly just tested that the debugfs
> control file is identical before and after enabling relative pointers,

mainline x86_64 defconfig+CONFIG_DYNAMIC_DEBUG
$ cat /dfs/dynamic_debug/control  | wc -l
2488


mainline x86_64 defconfig+CONFIG_DYNAMIC_DEBUG+this patch series
$ cat /dfs/dynamic_debug/control  | wc -l
2486

(seems like maybe 2 are missing?  Let me try to collect a diff. Maybe
2 were removed in this series?)

> and that enabling/disabling various pr_debug()s by writing to the
> control file takes effect. I should only be changing the format for

Can you suggest one that's easy to test?

> storing the metadata in the kernel image, so I think that should be enough.
>
> While this is still not merged, some new user of one of the string
> members could creep in, but that should be caught at build time.


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

On 24/06/2019 23.53, Nick Desaulniers wrote:
> On Thu, Jun 20, 2019 at 1:46 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> I've pushed them to https://github.com/Villemoes/linux/tree/dyndebug_v6
>> . They rebase pretty cleanly to just about anything you might prefer
>> testing on. Enabling it for arm64 or ppc64 is a trivial two-liner
>> similar to the x86 patch (and similar to the previous patches for those
>> arches). Thanks for volunteering to test this :)
> 
> Compile tested x86_64 allyesconfig
> boot tested x86_64 defconfig+CONFIG_DYNAMIC_DEBUG
> 
> (just curious why the Kconfig changes for arm64 or ppc64 aren't
> included in this set?)

Partly because I can't boot test those and this has proven much more
delicate than I thought, partly because none of the maintainers for
those arches have weighed in. So I'd rather have the bare minimal land,
then send specific individual patches for arm64 and ppc64.

>>> Anything I should test at runtime besides a boot
>>> test?
>>
>> Well, apart from booting, I've mostly just tested that the debugfs
>> control file is identical before and after enabling relative pointers,
> 
> mainline x86_64 defconfig+CONFIG_DYNAMIC_DEBUG
> $ cat /dfs/dynamic_debug/control  | wc -l
> 2488
> 
> 
> mainline x86_64 defconfig+CONFIG_DYNAMIC_DEBUG+this patch series
> $ cat /dfs/dynamic_debug/control  | wc -l
> 2486
> 
> (seems like maybe 2 are missing?  Let me try to collect a diff. Maybe
> 2 were removed in this series?)

Hm, no pr_debugs should have been added or removed. Perhaps you have a
slightly different set of modules loaded? Otherwise there's something
odd going on, and a diff would be really nice. It's possible that the
order of the lines are different, so you may have to sort them to get a
meaningful diff. (A diff is nice extra sanity check even if the line
count matches, of course).

>> and that enabling/disabling various pr_debug()s by writing to the
>> control file takes effect. I should only be changing the format for
> 
> Can you suggest one that's easy to test?

The ones in lib/kobject.c are triggered fairly often I think.

Thanks,
Rasmus

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Nick Desaulniers]

On Mon, Jun 24, 2019 at 11:35 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 24/06/2019 23.53, Nick Desaulniers wrote:
> > On Thu, Jun 20, 2019 at 1:46 PM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >> Well, apart from booting, I've mostly just tested that the debugfs
> >> control file is identical before and after enabling relative pointers,
> >
> > mainline x86_64 defconfig+CONFIG_DYNAMIC_DEBUG
> > $ cat /dfs/dynamic_debug/control  | wc -l
> > 2488
> >
> >
> > mainline x86_64 defconfig+CONFIG_DYNAMIC_DEBUG+this patch series
> > $ cat /dfs/dynamic_debug/control  | wc -l
> > 2486
> >
> > (seems like maybe 2 are missing?  Let me try to collect a diff. Maybe
> > 2 were removed in this series?)
>
> Hm, no pr_debugs should have been added or removed. Perhaps you have a
> slightly different set of modules loaded? Otherwise there's something
> odd going on, and a diff would be really nice. It's possible that the
> order of the lines are different, so you may have to sort them to get a
> meaningful diff. (A diff is nice extra sanity check even if the line
> count matches, of course).

You can fetch my logs from the latest commit to this dummy branch:
https://github.com/ClangBuiltLinux/linux/commit/90096d926aaf94eb84584a4418fde7c8d42dddea

Looking at `meld wo_patches.txt w_patches.txt`, it looks like:
1. line numbers in some translation units are adjusted.  maybe this is
intentional?
2. pci_pm_suspend_noirq seems to exist twice(?) before your patches, once after
3. xhci_urb_enqueue seems to exist three times before your patches, twice after

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Nick Desaulniers]

On Tue, Jun 25, 2019 at 3:18 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> 2. pci_pm_suspend_noirq seems to exist twice(?) before your patches, once after
> 3. xhci_urb_enqueue seems to exist three times before your patches, twice after

PEBKAC, I advanced my master branch without rebasing my local branch
with these changes.  With the local branch rebased onto master, there
are no differences.  2888 in both cases, line numbers do not differ.

I wasn't able to trip any logs in kobject:
(initramfs) echo "file kobject.c +p" > /dfs/dynamic_debug/control
(initramfs) cat /dfs/dynamic_debug/control | grep kobject
lib/kobject.c:161 [kobject]fill_kobj_path =p "kobject: '%s' (%p): %s:
path = '%s'\012"
lib/kobject.c:668 [kobject]kobject_cleanup =p "kobject: '%s' (%p): %s,
parent %p\012"
lib/kobject.c:672 [kobject]kobject_cleanup =p "kobject: '%s' (%p):
does not have a release() func"
lib/kobject.c:677 [kobject]kobject_cleanup =p "kobject: '%s' (%p):
auto cleanup 'remove' event\01"
lib/kobject.c:684 [kobject]kobject_cleanup =p "kobject: '%s' (%p):
auto cleanup kobject_del\012"
lib/kobject.c:690 [kobject]kobject_cleanup =p "kobject: '%s' (%p):
calling ktype release\012"
lib/kobject.c:696 [kobject]kobject_cleanup =p "kobject: '%s': free name\012"
lib/kobject.c:744 [kobject]dynamic_kobj_release =p "kobject: (%p): %s\012"
lib/kobject.c:253 [kobject]kobject_add_internal =p "kobject: '%s'
(%p): %s: parent: '%s', set: '%"
lib/kobject.c:915 [kobject]kset_release =p "kobject: '%s' (%p): %s\012"

The prints should show up in dmesg right, assuming you do something to
trigger them?  Can you provide more details for a test case that's
easy to trip? What's an easy case to reproduce from a limited
buildroot env (basic shell/toybox)?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Rasmus Villemoes]

On 27/06/2019 01.16, Nick Desaulniers wrote:
> On Tue, Jun 25, 2019 at 3:18 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> 
> The prints should show up in dmesg right, assuming you do something to
> trigger them?  Can you provide more details for a test case that's
> easy to trip? What's an easy case to reproduce from a limited
> buildroot env (basic shell/toybox)?
> 

Hm, I seemed to remember that those kobject events triggered all the
time. Oh well, try this one:

echo 'file ping.c +p' > control
ping localhost
dmesg | grep ping

Rasmus

Re: [PATCH v6 7/8] dynamic_debug: add asm-generic implementation for DYNAMIC_DEBUG_RELATIVE_POINTERS

[Nick Desaulniers]

On Wed, Jun 26, 2019 at 4:52 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 27/06/2019 01.16, Nick Desaulniers wrote:
> > On Tue, Jun 25, 2019 at 3:18 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> >
> > The prints should show up in dmesg right, assuming you do something to
> > trigger them?  Can you provide more details for a test case that's
> > easy to trip? What's an easy case to reproduce from a limited
> > buildroot env (basic shell/toybox)?
> >
>
> Hm, I seemed to remember that those kobject events triggered all the
> time. Oh well, try this one:
>
> echo 'file ping.c +p' > control
> ping localhost
> dmesg | grep ping

I don't have guest networking setup from QEMU to host, so there's no
network available to ping. :(

but:

(initramfs) echo 'file drivers/tty/*.c +p' > /dfs/dynamic_debug/control
(initramfs) grep tty /dfs/dynamic_debug/control
...
drivers/tty/serial/8250/8250_core.c:113 [8250]serial8250_interrupt =p
"%s(%d): start\012"
drivers/tty/serial/8250/8250_core.c:139 [8250]serial8250_interrupt =p
"%s(%d): end\012"
...
(initramfs) dmesg
...
[  134.895846] serial8250_interrupt(4): start
[  134.895967] serial8250_interrupt(4): end
[  134.895970] serial8250_interrupt(4): start
[  134.895981] serial8250_interrupt(4): end
[  134.895998] serial8250_interrupt(4): start
[  134.896053] serial8250_interrupt(4): end

I then verified that nothing new appears in dmesg related to these
traces after running:
(initramfs) echo 'file drivers/tty/*.c -p' > /dfs/dynamic_debug/control

so if that's good enough, then for the series:
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Thanks,
~Nick Desaulniers