Skip to contentSkip to conversation

various fixes v3

PATCH In review Jason Baron 6 participants 29 messages Aug 11, 2011 – Sep 19, 2011

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

cover various fixes v3

Hi,

Dynamic debug fixes and cleanups. Only changes from v2 are a re-base against Linus's latest tree and the inclusion of the re-posted version for patch #11.

Thanks,

-Jason

Joe Perches (4):
dynamic_debug: Add __dynamic_dev_dbg
dynamic_debug: Consolidate prefix output to single routine
dynamic_debug: Remove uses of KERN_CONT in dynamic_emit_prefix
dynamic_debug: Convert printks to pr_<level>

Jason Baron (7):
dynamic_debug: remove unused control variables
dynamic_debug: add Jason Baron as maintainer
dynamic_debug: make netdev_dbg() call __netdev_printk()
dynamic_debug: make netif_dbg() call __netdev_printk()
dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions
dynamic_debug: remove num_enabled accounting
dynamic_debug: use a single printk() to emit msgs

 MAINTAINERS                   |    6 ++
 drivers/base/core.c           |    5 +-
 include/linux/device.h        |    5 ++
 include/linux/dynamic_debug.h |   51 ++++++++-----
 include/linux/netdevice.h     |   10 ++--
 lib/dynamic_debug.c           |  155 ++++++++++++++++++++++++++--------------
 net/core/dev.c                |    3 +-
 7 files changed, 153 insertions(+), 82 deletions(-)
Signature · 1 line
1.7.5.4
#1 lore raw

PATCH 1/11 dynamic_debug: Add __dynamic_dev_dbg

From: Joe Perches <[email protected]>

Unlike dynamic_pr_debug, dynamic uses of dev_dbg can not currently add task_pid/KBUILD_MODNAME/__func__/__LINE__ to selected debug output.

Add a new function similar to dynamic_pr_debug to optionally emit these prefixes.

---
 drivers/base/core.c           |    5 +++--
 include/linux/device.h        |    5 +++++
 include/linux/dynamic_debug.h |   10 ++++++++--
 lib/dynamic_debug.c           |   38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 4 deletions(-)
drivers/base/core.c +3 -2
index bc8729d..82c8654 100644
@@ -1764,8 +1764,8 @@ void device_shutdown(void)
 
 #ifdef CONFIG_PRINTK
 
-static int __dev_printk(const char *level, const struct device *dev,
-			struct va_format *vaf)
+int __dev_printk(const char *level, const struct device *dev,
+		 struct va_format *vaf)
 {
 	if (!dev)
 		return printk("%s(NULL device *): %pV", level, vaf);
@@ -1773,6 +1773,7 @@ static int __dev_printk(const char *level, const struct device *dev,
 	return printk("%s%s %s: %pV",
 		      level, dev_driver_string(dev), dev_name(dev), vaf);
 }
+EXPORT_SYMBOL(__dev_printk);
 
 int dev_printk(const char *level, const struct device *dev,
 	       const char *fmt, ...)
include/linux/device.h +5 -0
index c20dfbf..4639419 100644
@@ -785,6 +785,8 @@ extern const char *dev_driver_string(const struct device *dev);
 
 #ifdef CONFIG_PRINTK
 
+extern int __dev_printk(const char *level, const struct device *dev,
+			struct va_format *vaf);
 extern int dev_printk(const char *level, const struct device *dev,
 		      const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
@@ -805,6 +807,9 @@ extern int _dev_info(const struct device *dev, const char *fmt, ...)
 
 #else
 
+static inline int __dev_printk(const char *level, const struct device *dev,
+			       struct va_format *vaf)
+	 { return 0; }
 static inline int dev_printk(const char *level, const struct device *dev,
 		      const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
include/linux/dynamic_debug.h +8 -2
index e747ecd..bdf1531 100644
@@ -47,6 +47,13 @@ extern int ddebug_remove_module(const char *mod_name);
 extern int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 
+struct device;
+
+extern int __dynamic_dev_dbg(struct _ddebug *descriptor,
+			     const struct device *dev,
+			     const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+
 #define dynamic_pr_debug(fmt, ...) do {					\
 	static struct _ddebug descriptor				\
 	__used								\
@@ -57,7 +64,6 @@ extern int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
 	} while (0)
 
-
 #define dynamic_dev_dbg(dev, fmt, ...) do {				\
 	static struct _ddebug descriptor				\
 	__used								\
@@ -65,7 +71,7 @@ extern int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
 		_DPRINTK_FLAGS_DEFAULT };				\
 	if (unlikely(descriptor.enabled))				\
-		dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__);	\
+		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
 	} while (0)
 
 #else
lib/dynamic_debug.c +38 -0
index 75ca78f..63b6f95 100644
@@ -30,6 +30,7 @@
 #include <linux/jump_label.h>
 #include <linux/hardirq.h>
 #include <linux/sched.h>
+#include <linux/device.h>
 
 extern struct _ddebug __start___verbose[];
 extern struct _ddebug __stop___verbose[];
@@ -456,6 +457,43 @@ int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 }
 EXPORT_SYMBOL(__dynamic_pr_debug);
 
+int __dynamic_dev_dbg(struct _ddebug *descriptor,
+		      const struct device *dev, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	int res;
+
+	BUG_ON(!descriptor);
+	BUG_ON(!fmt);
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	res = printk(KERN_DEBUG);
+	if (descriptor->flags & _DPRINTK_FLAGS_INCL_TID) {
+		if (in_interrupt())
+			res += printk(KERN_CONT "<intr> ");
+		else
+			res += printk(KERN_CONT "[%d] ", task_pid_vnr(current));
+	}
+	if (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+		res += printk(KERN_CONT "%s:", descriptor->modname);
+	if (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+		res += printk(KERN_CONT "%s:", descriptor->function);
+	if (descriptor->flags & _DPRINTK_FLAGS_INCL_LINENO)
+		res += printk(KERN_CONT "%d ", descriptor->lineno);
+
+	res += __dev_printk(KERN_CONT, dev, &vaf);
+
+	va_end(args);
+
+	return res;
+}
+EXPORT_SYMBOL(__dynamic_dev_dbg);
+
 static __initdata char ddebug_setup_string[1024];
 static __init int ddebug_setup_query(char *str)
 {
Signature · 1 line
1.7.5.4
#2 lore raw

PATCH 2/11 dynamic_debug: Consolidate prefix output to single routine

In reply to Jason Baron

From: Joe Perches <[email protected]>

Adding dynamic_dev_dbg duplicated prefix output.
Consolidate that output to a single routine.

---
 lib/dynamic_debug.c |   38 ++++++++++++++++++--------------------
 1 files changed, 18 insertions(+), 20 deletions(-)
lib/dynamic_debug.c +18 -20
index 63b6f95..3721709 100644
@@ -428,15 +428,10 @@ static int ddebug_exec_query(char *query_string)
 	return 0;
 }
 
-int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
+static int dynamic_emit_prefix(const struct _ddebug *descriptor)
 {
-	va_list args;
 	int res;
 
-	BUG_ON(!descriptor);
-	BUG_ON(!fmt);
-
-	va_start(args, fmt);
 	res = printk(KERN_DEBUG);
 	if (descriptor->flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
@@ -450,7 +445,23 @@ int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 		res += printk(KERN_CONT "%s:", descriptor->function);
 	if (descriptor->flags & _DPRINTK_FLAGS_INCL_LINENO)
 		res += printk(KERN_CONT "%d ", descriptor->lineno);
+
+	return res;
+}
+
+int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
+{
+	va_list args;
+	int res;
+
+	BUG_ON(!descriptor);
+	BUG_ON(!fmt);
+
+	va_start(args, fmt);
+
+	res = dynamic_emit_prefix(descriptor);
 	res += vprintk(fmt, args);
+
 	va_end(args);
 
 	return res;
@@ -472,20 +483,7 @@ int __dynamic_dev_dbg(struct _ddebug *descriptor,
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	res = printk(KERN_DEBUG);
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_TID) {
-		if (in_interrupt())
-			res += printk(KERN_CONT "<intr> ");
-		else
-			res += printk(KERN_CONT "[%d] ", task_pid_vnr(current));
-	}
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME)
-		res += printk(KERN_CONT "%s:", descriptor->modname);
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
-		res += printk(KERN_CONT "%s:", descriptor->function);
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_LINENO)
-		res += printk(KERN_CONT "%d ", descriptor->lineno);
-
+	res = dynamic_emit_prefix(descriptor);
 	res += __dev_printk(KERN_CONT, dev, &vaf);
 
 	va_end(args);
Signature · 1 line
1.7.5.4
#3 lore raw

PATCH 3/11 dynamic_debug: Remove uses of KERN_CONT in dynamic_emit_prefix

In reply to Jason Baron

From: Joe Perches <[email protected]>

Multiple printks with KERN_CONT can be interleaved by other printks. Reduce the likelihood of that interleaving by consolidating multiple calls to printk.

---
 lib/dynamic_debug.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)
lib/dynamic_debug.c +23 -11
index 3721709..a3eb6ab 100644
@@ -430,23 +430,35 @@ static int ddebug_exec_query(char *query_string)
 
 static int dynamic_emit_prefix(const struct _ddebug *descriptor)
 {
-	int res;
+	char tid[sizeof(int) + sizeof(int)/2 + 4];
+	char lineno[sizeof(int) + sizeof(int)/2];
 
-	res = printk(KERN_DEBUG);
 	if (descriptor->flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
-			res += printk(KERN_CONT "<intr> ");
+			snprintf(tid, sizeof(tid), "%s", "<intr> ");
 		else
-			res += printk(KERN_CONT "[%d] ", task_pid_vnr(current));
+			snprintf(tid, sizeof(tid), "[%d] ",
+				 task_pid_vnr(current));
+	} else {
+		tid[0] = 0;
 	}
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME)
-		res += printk(KERN_CONT "%s:", descriptor->modname);
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
-		res += printk(KERN_CONT "%s:", descriptor->function);
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_LINENO)
-		res += printk(KERN_CONT "%d ", descriptor->lineno);
 
-	return res;
+	if (descriptor->flags & _DPRINTK_FLAGS_INCL_LINENO)
+		snprintf(lineno, sizeof(lineno), "%d", descriptor->lineno);
+	else
+		lineno[0] = 0;
+
+	return printk(KERN_DEBUG "%s%s%s%s%s%s",
+		      tid,
+		      (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME) ?
+		      descriptor->modname : "",
+		      (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME) ?
+		      ":" : "",
+		      (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME) ?
+		      descriptor->function : "",
+		      (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME) ?
+		      ":" : "",
+		      lineno);
 }
 
 int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
Signature · 1 line
1.7.5.4
#4 lore raw

PATCH 4/11 dynamic_debug: Convert printks to pr_<level>

In reply to Jason Baron

From: Joe Perches <[email protected]>

Add pr_fmt(fmt) with __func__.
Converts "ddebug:" prefix to "dynamic_debug:".

Most likely the if (verbose) outputs could also be converted from pr_info to pr_debug.

---
 lib/dynamic_debug.c |   59 ++++++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 33 deletions(-)
lib/dynamic_debug.c +26 -33
index a3eb6ab..4fc03dd 100644
@@ -10,6 +10,8 @@
  * Copyright (C) 2011 Bart Van Assche.  All Rights Reserved.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -160,8 +162,7 @@ static void ddebug_change(const struct ddebug_query *query,
 			else
 				dp->enabled = 0;
 			if (verbose)
-				printk(KERN_INFO
-					"ddebug: changed %s:%d [%s]%s %s\n",
+				pr_info("changed %s:%d [%s]%s %s\n",
 					dp->filename, dp->lineno,
 					dt->mod_name, dp->function,
 					ddebug_describe_flags(dp, flagbuf,
@@ -171,7 +172,7 @@ static void ddebug_change(const struct ddebug_query *query,
 	mutex_unlock(&ddebug_lock);
 
 	if (!nfound && verbose)
-		printk(KERN_INFO "ddebug: no matches for query\n");
+		pr_info("no matches for query\n");
 }
 
 /*
@@ -216,10 +217,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
 
 	if (verbose) {
 		int i;
-		printk(KERN_INFO "%s: split into words:", __func__);
+		pr_info("split into words:");
 		for (i = 0 ; i < nwords ; i++)
-			printk(" \"%s\"", words[i]);
-		printk("\n");
+			pr_cont(" \"%s\"", words[i]);
+		pr_cont("\n");
 	}
 
 	return nwords;
@@ -331,16 +332,15 @@ static int ddebug_parse_query(char *words[], int nwords,
 			}
 		} else {
 			if (verbose)
-				printk(KERN_ERR "%s: unknown keyword \"%s\"\n",
-					__func__, words[i]);
+				pr_err("unknown keyword \"%s\"\n", words[i]);
 			return -EINVAL;
 		}
 	}
 
 	if (verbose)
-		printk(KERN_INFO "%s: q->function=\"%s\" q->filename=\"%s\" "
-		       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
-			__func__, query->function, query->filename,
+		pr_info("q->function=\"%s\" q->filename=\"%s\" "
+			"q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
+			query->function, query->filename,
 			query->module, query->format, query->first_lineno,
 			query->last_lineno);
 
@@ -369,7 +369,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 		return -EINVAL;
 	}
 	if (verbose)
-		printk(KERN_INFO "%s: op='%c'\n", __func__, op);
+		pr_info("op='%c'\n", op);
 
 	for ( ; *str ; ++str) {
 		for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
@@ -384,7 +384,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 	if (flags == 0)
 		return -EINVAL;
 	if (verbose)
-		printk(KERN_INFO "%s: flags=0x%x\n", __func__, flags);
+		pr_info("flags=0x%x\n", flags);
 
 	/* calculate final *flagsp, *maskp according to mask and op */
 	switch (op) {
@@ -402,8 +402,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 		break;
 	}
 	if (verbose)
-		printk(KERN_INFO "%s: *flagsp=0x%x *maskp=0x%x\n",
-			__func__, *flagsp, *maskp);
+		pr_info("*flagsp=0x%x *maskp=0x%x\n", *flagsp, *maskp);
 	return 0;
 }
 
@@ -508,7 +507,7 @@ static __initdata char ddebug_setup_string[1024];
 static __init int ddebug_setup_query(char *str)
 {
 	if (strlen(str) >= 1024) {
-		pr_warning("ddebug boot param string too large\n");
+		pr_warn("ddebug boot param string too large\n");
 		return 0;
 	}
 	strcpy(ddebug_setup_string, str);
@@ -536,8 +535,7 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
 		return -EFAULT;
 	tmpbuf[len] = '\0';
 	if (verbose)
-		printk(KERN_INFO "%s: read %d bytes from userspace\n",
-			__func__, (int)len);
+		pr_info("read %d bytes from userspace\n", (int)len);
 
 	ret = ddebug_exec_query(tmpbuf);
 	if (ret)
@@ -600,8 +598,7 @@ static void *ddebug_proc_start(struct seq_file *m, loff_t *pos)
 	int n = *pos;
 
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p *pos=%lld\n",
-			__func__, m, (unsigned long long)*pos);
+		pr_info("called m=%p *pos=%lld\n", m, (unsigned long long)*pos);
 
 	mutex_lock(&ddebug_lock);
 
@@ -626,8 +623,8 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
 	struct _ddebug *dp;
 
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p p=%p *pos=%lld\n",
-			__func__, m, p, (unsigned long long)*pos);
+		pr_info("called m=%p p=%p *pos=%lld\n",
+			m, p, (unsigned long long)*pos);
 
 	if (p == SEQ_START_TOKEN)
 		dp = ddebug_iter_first(iter);
@@ -650,8 +647,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	char flagsbuf[8];
 
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p p=%p\n",
-			__func__, m, p);
+		pr_info("called m=%p p=%p\n", m, p);
 
 	if (p == SEQ_START_TOKEN) {
 		seq_puts(m,
@@ -676,8 +672,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 static void ddebug_proc_stop(struct seq_file *m, void *p)
 {
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p p=%p\n",
-			__func__, m, p);
+		pr_info("called m=%p p=%p\n", m, p);
 	mutex_unlock(&ddebug_lock);
 }
 
@@ -700,7 +695,7 @@ static int ddebug_proc_open(struct inode *inode, struct file *file)
 	int err;
 
 	if (verbose)
-		printk(KERN_INFO "%s: called\n", __func__);
+		pr_info("called\n");
 
 	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
 	if (iter == NULL)
@@ -752,8 +747,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 	mutex_unlock(&ddebug_lock);
 
 	if (verbose)
-		printk(KERN_INFO "%u debug prints in module %s\n",
-				 n, dt->mod_name);
+		pr_info("%u debug prints in module %s\n", n, dt->mod_name);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ddebug_add_module);
@@ -775,8 +769,7 @@ int ddebug_remove_module(const char *mod_name)
 	int ret = -ENOENT;
 
 	if (verbose)
-		printk(KERN_INFO "%s: removing module \"%s\"\n",
-				__func__, mod_name);
+		pr_info("removing module \"%s\"\n", mod_name);
 
 	mutex_lock(&ddebug_lock);
 	list_for_each_entry_safe(dt, nextdt, &ddebug_tables, link) {
@@ -852,8 +845,8 @@ static int __init dynamic_debug_init(void)
 	if (ddebug_setup_string[0] != '\0') {
 		ret = ddebug_exec_query(ddebug_setup_string);
 		if (ret)
-			pr_warning("Invalid ddebug boot param %s",
-				   ddebug_setup_string);
+			pr_warn("Invalid ddebug boot param %s",
+				ddebug_setup_string);
 		else
 			pr_info("ddebug initialized with string %s",
 				ddebug_setup_string);
Signature · 1 line
1.7.5.4
#5 lore raw

PATCH 5/11 dynamic_debug: remove unused control variables

In reply to Jason Baron

From: Jason Baron <[email protected]>

Remove no longer used dynamic debug control variables. The definitions were removed a while ago, but we forgot to clean up the extern references.

---
 include/linux/dynamic_debug.h |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)
include/linux/dynamic_debug.h +0 -7
index bdf1531..843cb9e 100644
@@ -1,13 +1,6 @@
 #ifndef _DYNAMIC_DEBUG_H
 #define _DYNAMIC_DEBUG_H
 
-/* dynamic_printk_enabled, and dynamic_printk_enabled2 are bitmasks in which
- * bit n is set to 1 if any modname hashes into the bucket n, 0 otherwise. They
- * use independent hash functions, to reduce the chance of false positives.
- */
-extern long long dynamic_debug_enabled;
-extern long long dynamic_debug_enabled2;
-
 /*
  * An instance of this structure is created in a special
  * ELF section at every dynamic debug callsite.  At runtime,
Signature · 1 line
1.7.5.4
#6 lore raw

PATCH 6/11 dynamic_debug: add Jason Baron as maintainer

In reply to Jason Baron

From: Jason Baron <[email protected]>

Add a maintainers entry for dynamic debug. Hopefully nobody will object to me as maintainer...

---
 MAINTAINERS |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
MAINTAINERS +6 -0
index 1f8267f..b7c52f3 100644
@@ -2297,6 +2297,12 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/net/wan/dscc4.c
 
+DYNAMIC DEBUG
+M:	Jason Baron <[email protected]>
+S:	Maintained
+F:	lib/dynamic_debug.c
+F:	include/linux/dynamic_debug.h
+
 DZ DECSTATION DZ11 SERIAL DRIVER
 M:	"Maciej W. Rozycki" <[email protected]>
 S:	Maintained
Signature · 1 line
1.7.5.4
#7 lore raw

PATCH 7/11 dynamic_debug: make netdev_dbg() call __netdev_printk()

In reply to Jason Baron

From: Jason Baron <[email protected]>

Previously, if dynamic debug was enabled netdev_dbg() was using dynamic_dev_dbg() to print out the underlying msg. Fix this by making sure netdev_dbg() uses __netdev_printk().

Cc David S. Miller <[email protected]>
---
 include/linux/dynamic_debug.h |   17 +++++++++++++++++
 include/linux/netdevice.h     |    6 ++++--
 lib/dynamic_debug.c           |   25 +++++++++++++++++++++++++
 net/core/dev.c                |    3 ++-
 4 files changed, 48 insertions(+), 3 deletions(-)
include/linux/dynamic_debug.h +17 -0
index 843cb9e..feaac1e 100644
@@ -47,6 +47,13 @@ extern int __dynamic_dev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+struct net_device;
+
+extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
+			     const struct net_device *dev,
+			     const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+
 #define dynamic_pr_debug(fmt, ...) do {					\
 	static struct _ddebug descriptor				\
 	__used								\
@@ -67,6 +74,16 @@ extern int __dynamic_dev_dbg(struct _ddebug *descriptor,
 		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
 	} while (0)
 
+#define dynamic_netdev_dbg(dev, fmt, ...) do {				\
+	static struct _ddebug descriptor				\
+	__used								\
+	__attribute__((section("__verbose"), aligned(8))) =		\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
+		_DPRINTK_FLAGS_DEFAULT };				\
+	if (unlikely(descriptor.enabled))				\
+		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
+	} while (0)
+
 #else
 
 static inline int ddebug_remove_module(const char *mod)
include/linux/netdevice.h +4 -2
index ddee79b..9333a03 100644
@@ -2617,6 +2617,9 @@ static inline const char *netdev_name(const struct net_device *dev)
 	return dev->name;
 }
 
+extern int __netdev_printk(const char *level, const struct net_device *dev,
+			struct va_format *vaf);
+
 extern int netdev_printk(const char *level, const struct net_device *dev,
 			 const char *format, ...)
 	__attribute__ ((format (printf, 3, 4)));
@@ -2644,8 +2647,7 @@ extern int netdev_info(const struct net_device *dev, const char *format, ...)
 #elif defined(CONFIG_DYNAMIC_DEBUG)
 #define netdev_dbg(__dev, format, args...)			\
 do {								\
-	dynamic_dev_dbg((__dev)->dev.parent, "%s: " format,	\
-			netdev_name(__dev), ##args);		\
+	dynamic_netdev_dbg(__dev, format, ##args);		\
 } while (0)
 #else
 #define netdev_dbg(__dev, format, args...)			\
lib/dynamic_debug.c +25 -0
index 4fc03dd..ee3b9ba 100644
@@ -33,6 +33,7 @@
 #include <linux/hardirq.h>
 #include <linux/sched.h>
 #include <linux/device.h>
+#include <linux/netdevice.h>
 
 extern struct _ddebug __start___verbose[];
 extern struct _ddebug __stop___verbose[];
@@ -503,6 +504,30 @@ int __dynamic_dev_dbg(struct _ddebug *descriptor,
 }
 EXPORT_SYMBOL(__dynamic_dev_dbg);
 
+int __dynamic_netdev_dbg(struct _ddebug *descriptor,
+		      const struct net_device *dev, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	int res;
+
+	BUG_ON(!descriptor);
+	BUG_ON(!fmt);
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	res = dynamic_emit_prefix(descriptor);
+	res += __netdev_printk(KERN_CONT, dev, &vaf);
+
+	va_end(args);
+
+	return res;
+}
+EXPORT_SYMBOL(__dynamic_netdev_dbg);
+
 static __initdata char ddebug_setup_string[1024];
 static __init int ddebug_setup_query(char *str)
 {
net/core/dev.c +2 -1
index 17d67b5..c47a7bc 100644
@@ -6290,7 +6290,7 @@ const char *netdev_drivername(const struct net_device *dev)
 	return empty;
 }
 
-static int __netdev_printk(const char *level, const struct net_device *dev,
+int __netdev_printk(const char *level, const struct net_device *dev,
 			   struct va_format *vaf)
 {
 	int r;
@@ -6305,6 +6305,7 @@ static int __netdev_printk(const char *level, const struct net_device *dev,
 
 	return r;
 }
+EXPORT_SYMBOL(__netdev_printk);
 
 int netdev_printk(const char *level, const struct net_device *dev,
 		  const char *format, ...)
Signature · 1 line
1.7.5.4
#8 lore raw

PATCH 1/1 dynamic_debug: call __netdev_printk only for CONFIG_NET

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

---

Quoted 10 lines · Jason Baron

From: Jason Baron <[email protected]>

Previously, if dynamic debug was enabled netdev_dbg() was using dynamic_dev_dbg() to print out the underlying msg. Fix this by making sure netdev_dbg() uses __netdev_printk().

Cc David S. Miller <[email protected]>

---

lib/dynamic_debug.c +2 -0
@@ -504,6 +504,7 @@ int __dynamic_dev_dbg(struct _ddebug *descriptor,
 }
 EXPORT_SYMBOL(__dynamic_dev_dbg);
 
+#ifdef CONFIG_NET
 int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 		      const struct net_device *dev, const char *fmt, ...)
 {
@@ -527,6 +528,7 @@ int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 	return res;
 }
 EXPORT_SYMBOL(__dynamic_netdev_dbg);
+#endif
 
 static __initdata char ddebug_setup_string[1024];
 static __init int ddebug_setup_query(char *str)
#9 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Hi,

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Hopefully, it will be pulled in soon.

Thanks,

-Jason

#10 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

Quoted 6 lines · Jason Baron
Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Quoted 7 lines

Hi,

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Hopefully, it will be pulled in soon.

As that thread again spun off into confusion, can you please resend the end result?

thanks,

greg k-h

#11 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

Quoted 9 lines · Greg KH
Quoted 8 lines · Jason Baron
Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Hi,

Quoted 8 lines
Quoted 5 lines

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Hopefully, it will be pulled in soon.

As that thread again spun off into confusion, can you please resend the end result?

That spinning confusion had nothing to do with the posted & correct patch which could have been applied several weeks ago.

I'm curious: Do you delete most of your email on a routine basis?

Signature · 2 lines
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
#12 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

Quoted 16 lines · Randy Dunlap
Quoted 15 lines · Greg KH
Quoted 13 lines · Jason Baron
Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Hi,

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Hopefully, it will be pulled in soon.

Quoted 7 lines
Quoted 2 lines

As that thread again spun off into confusion, can you please resend the end result?

That spinning confusion had nothing to do with the posted & correct patch which could have been applied several weeks ago.

I'm curious: Do you delete most of your email on a routine basis?

No, only after going through pending patches do I purge them. And when a series of patches generates a thread like this one, where people are arguing over the way the macros are named, and no one seems to agree, I will take it as the fact that this series was contentious and needs to be resent after taking into consideration the original complaints.

For me to keep all email threads, based on the amount of email I get[1], would be ludicrous.

greg k-h

[1]: http://www.kroah.com/log/linux/get_lots_of_email.html

#13 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

Quoted 23 lines · Greg KH
Quoted 22 lines · Randy Dunlap
Quoted 9 lines · Greg KH
Quoted 8 lines · Jason Baron
Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Hi,

Quoted 8 lines
Quoted 5 lines

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Hopefully, it will be pulled in soon.

As that thread again spun off into confusion, can you please resend the end result?

That spinning confusion had nothing to do with the posted & correct patch which could have been applied several weeks ago.

Quoted 7 lines
Quoted 1 line

I'm curious: Do you delete most of your email on a routine basis?

No, only after going through pending patches do I purge them. And when a series of patches generates a thread like this one, where people are arguing over the way the macros are named, and no one seems to agree, I will take it as the fact that this series was contentious and needs to be resent after taking into consideration the original complaints.

Sheesh. The naming of the macros has nothing to do with this build fix.

Quoted 6 lines

For me to keep all email threads, based on the amount of email I get[1], would be ludicrous.

greg k-h

[1]: http://www.kroah.com/log/linux/get_lots_of_email.html

Signature · 2 lines
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
#14 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

In reply to Greg KH
Quoted 13 lines · Greg KH
Quoted 12 lines · Jason Baron
Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Hi,

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Quoted 8 lines
Quoted 1 line

Hopefully, it will be pulled in soon.

As that thread again spun off into confusion, can you please resend the end result?

thanks,

greg k-h

Hi,

Andrew Morton has pulled these into his -mm tree...so I think the series should be all set.

Thanks,

-Jason

#15 lore raw

Re: [PATCH ] dynamic_debug: call __netdev_printk only for CONFIG_NET

Quoted 19 lines · Jason Baron
Quoted 18 lines · Greg KH
Quoted 6 lines · Jason Baron
Quoted 4 lines · Arnd Bergmann

__netdev_printk is only defined when CONFIG_NET is set. Since we only need __dynamic_netdev_dbg for network drivers, we can make it conditional on the same Kconfig symbol.

Quoted 7 lines

Hi,

Yes, I've posted a fix for this:

https://lkml.org/lkml/2011/8/30/297

Hopefully, it will be pulled in soon.

As that thread again spun off into confusion, can you please resend the end result?

Quoted 8 lines
Quoted 3 lines

thanks,

greg k-h

Hi,

Andrew Morton has pulled these into his -mm tree...so I think the series should be all set.

Good. The build fix is still needed in today's linux-next (20110919).

Signature · 2 lines
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
#16 lore raw

PATCH 8/11 dynamic_debug: make netif_dbg() call __netdev_printk()

In reply to Jason Baron

From: Jason Baron <[email protected]>

Previously, netif_dbg() was using dynamic_dev_dbg() to perform the underlying printk. Fix it to use __netdev_printk(), instead.

Cc David S. Miller <[email protected]>
---
 include/linux/netdevice.h |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
include/linux/netdevice.h +1 -3
index 9333a03..2797260 100644
@@ -2714,9 +2714,7 @@ do {								\
 #define netif_dbg(priv, type, netdev, format, args...)		\
 do {								\
 	if (netif_msg_##type(priv))				\
-		dynamic_dev_dbg((netdev)->dev.parent,		\
-				"%s: " format,			\
-				netdev_name(netdev), ##args);	\
+		dynamic_netdev_dbg(netdev, format, ##args);	\
 } while (0)
 #else
 #define netif_dbg(priv, type, dev, format, args...)			\
Signature · 1 line
1.7.5.4
#17 lore raw

PATCH 9/11 dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

In reply to Jason Baron

From: Jason Baron <[email protected]>

Replace the repetitive struct _ddebug descriptor definitions with a new DYNAMIC_DEBUG_META_DATA(fmt) macro.

---
 include/linux/dynamic_debug.h |   33 ++++++++++++++-------------------
 1 files changed, 14 insertions(+), 19 deletions(-)
include/linux/dynamic_debug.h +14 -19
index feaac1e..bc75472 100644
@@ -54,33 +54,28 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+#define DYNAMIC_DEBUG_METADATA(fmt)				\
+	static struct _ddebug descriptor			\
+	__used							\
+	__attribute__((section("__verbose"), aligned(8))) =	\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
+		_DPRINTK_FLAGS_DEFAULT };
+
 #define dynamic_pr_debug(fmt, ...) do {					\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DYNAMIC_DEBUG_METADATA(fmt);					\
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
+		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);\
 	} while (0)
 
 #define dynamic_dev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DYNAMIC_DEBUG_METADATA(fmt);					\
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
+		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)
 
-#define dynamic_netdev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
-	if (unlikely(descriptor.enabled))				\
+#define dynamic_netdev_dbg(dev, fmt, ...) do {				   \
+	DYNAMIC_DEBUG_METADATA(fmt);					   \
+	if (unlikely(descriptor.enabled))				   \
 		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)
 
Signature · 1 line
1.7.5.4
#18 lore raw

Re: [PATCH 09/11] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

Quoted 3 lines · Jason Baron

From: Jason Baron <[email protected]>
Replace the repetitive struct _ddebug descriptor definitions with a new DYNAMIC_DEBUG_META_DATA(fmt) macro.

Hey Jason.

I think some improvements can be made to this one.

include/linux/dynamic_debug.h

Quoted 1 line
include/linux/dynamic_debug.h +0 -0

[]

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 63

Quoted 11 lines
include/linux/dynamic_debug.h +7 -0
@@ -54,33 +54,28 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+#define DYNAMIC_DEBUG_METADATA(fmt)				\
+	static struct _ddebug descriptor			\
+	__used							\
+	__attribute__((section("__verbose"), aligned(8))) =	\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
+		_DPRINTK_FLAGS_DEFAULT };
+

I think this is unclear and should be:

#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
	static struct _ddebug name				\
	__used							\
	__attribute__((section("__verbose"), aligned(8))) =	\
	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
		_DPRINTK_FLAGS_DEFAULT }

(extra semicolon at end removed)

so the uses become:

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 71

Quoted 7 lines
include/linux/dynamic_debug.h +1 -5
 #define dynamic_dev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DYNAMIC_DEBUG_METADATA(fmt);					\
	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 74

Quoted 4 lines
include/linux/dynamic_debug.h +1 -1
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
+		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)

so then there aren't any magic variable names.

cheers, Joe

#19 lore raw

Re: [PATCH 09/11 re-post] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

Quoted 45 lines · Joe Perches
Quoted 3 lines

From: Jason Baron <[email protected]>
Replace the repetitive struct _ddebug descriptor definitions with a new DYNAMIC_DEBUG_META_DATA(fmt) macro.

Hey Jason.

I think some improvements can be made to this one.

include/linux/dynamic_debug.h

Quoted 1 line
include/linux/dynamic_debug.h +0 -0

[]

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 63

Quoted 11 lines
include/linux/dynamic_debug.h +7 -0
@@ -54,33 +54,28 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+#define DYNAMIC_DEBUG_METADATA(fmt)				\
+	static struct _ddebug descriptor			\
+	__used							\
+	__attribute__((section("__verbose"), aligned(8))) =	\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
+		_DPRINTK_FLAGS_DEFAULT };
+

I think this is unclear and should be:

#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
	static struct _ddebug name				\
	__used							\
	__attribute__((section("__verbose"), aligned(8))) =	\
	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
		_DPRINTK_FLAGS_DEFAULT }

(extra semicolon at end removed)

so the uses become:

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 65

Quoted 7 lines
include/linux/dynamic_debug.h +1 -5
 #define dynamic_dev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DYNAMIC_DEBUG_METADATA(fmt);					\
	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);
Quoted 9 lines

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 68

Quoted 4 lines
include/linux/dynamic_debug.h +1 -1
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
+		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)

so then there aren't any magic variable names.

cheers, Joe

Ok, Thanks for the review. Here's a re-post of it:

From: Jason Baron <[email protected]>

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt) macro.

---

 include/linux/dynamic_debug.h |   33 ++++++++++++++-------------------
 1 files changed, 14 insertions(+), 19 deletions(-)
include/linux/dynamic_debug.h +14 -19
index feaac1e..d1a78b2 100644
@@ -54,33 +54,28 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+	static struct _ddebug name				\
+	__used							\
+	__attribute__((section("__verbose"), aligned(8))) =	\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
+		_DPRINTK_FLAGS_DEFAULT }
+
 #define dynamic_pr_debug(fmt, ...) do {					\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
+		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);\
 	} while (0)
 
 #define dynamic_dev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
+		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)
 
-#define dynamic_netdev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
-	if (unlikely(descriptor.enabled))				\
+#define dynamic_netdev_dbg(dev, fmt, ...) do {				   \
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		   \
+	if (unlikely(descriptor.enabled))				   \
 		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)
 
#20 lore raw

Re: [PATCH 09/11 re-post] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

Quoted 2 lines · Jason Baron

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt) macro.

[]

include/linux/dynamic_debug.h

Quoted 1 line
include/linux/dynamic_debug.h +0 -0

[]

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 62

Quoted 10 lines
include/linux/dynamic_debug.h +6 -0
@@ -54,33 +54,28 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+	static struct _ddebug name				\
+	__used							\
+	__attribute__((section("__verbose"), aligned(8))) =	\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
+		_DPRINTK_FLAGS_DEFAULT }

Hey again Jason.

Quoted 1 line

From a kernel coding style consistency POV, shouldn't

this use c99 named initializers?

#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)			\
	static struct _ddebug name = {					\
		.modname = KBUILD_MODNAME,				\
		.function = __func__,					\
		.filename = __FILE__,					\
		.format = fmt,						\
		.lineno = __LINE__,					\
		.flags =  _DPRINTK_FLAGS_DEFAULT,			\
		.enabled = false,					\
	} __used __aligned(8) __attribute__((section("__verbose")))

If the pr_debug/dynamic_debug inversion is done,
the .enabled flag should be set on as well when
-DDEBUG is set, correct?

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 68

Quoted 11 lines

#define dynamic_pr_debug(fmt, ...) do { \

include/linux/dynamic_debug.h +2 -6
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
+		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);\
 	} while (0)

Perhaps

#define dynamic_pr_debug(fmt, ...) \

do {									\
	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
	if (unlikely(descriptor.enabled))				\
		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),		\
				   ##__VA_ARGS__);			\
} while (0)

Lastly, are the unlikely()s really useful?

#21 lore raw

Re: [PATCH 09/11 re-post take #2] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

Hi Joe,

Quoted 19 lines · Joe Perches
Quoted 2 lines · Jason Baron

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt) macro.

[]

include/linux/dynamic_debug.h

Quoted 1 line
include/linux/dynamic_debug.h +0 -0

[]

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 62

Quoted 10 lines
include/linux/dynamic_debug.h +6 -0
@@ -54,33 +54,28 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+	static struct _ddebug name				\
+	__used							\
+	__attribute__((section("__verbose"), aligned(8))) =	\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,	\
+		_DPRINTK_FLAGS_DEFAULT }

Hey again Jason.

Quoted 2 lines

From a kernel coding style consistency POV, shouldn't this use c99 named initializers?

yes, that would be better than what we have now.

Quoted 12 lines
#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)			\
	static struct _ddebug name = {					\
		.modname = KBUILD_MODNAME,				\
		.function = __func__,					\
		.filename = __FILE__,					\
		.format = fmt,						\
		.lineno = __LINE__,					\
		.flags =  _DPRINTK_FLAGS_DEFAULT,			\
		.enabled = false,					\
	} __used __aligned(8) __attribute__((section("__verbose")))
Quoted 3 lines

If the pr_debug/dynamic_debug inversion is done,
the .enabled flag should be set on as well when
-DDEBUG is set, correct?

right, but I'm leaving this to a subsequent patch.

Quoted 15 lines

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 67

Quoted 12 lines
include/linux/dynamic_debug.h +2 -6
 #define dynamic_pr_debug(fmt, ...) do {					\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
 	if (unlikely(descriptor.enabled))				\
-		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
+		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);\
 	} while (0)

Perhaps

Quoted 7 lines

#define dynamic_pr_debug(fmt, ...) \

do {									\
	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
	if (unlikely(descriptor.enabled))				\
		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),		\
				   ##__VA_ARGS__);			\
} while (0)

ok.

Quoted 3 lines

Lastly, are the unlikely()s really useful?

I think the unlikely's make sense, since we don't expect these to be enabled by default.

So, I've re-spun the patch, with the above comments (below). Thanks!

-Jason

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro.

---
 include/linux/dynamic_debug.h |   64 ++++++++++++++++++++++------------------
 1 files changed, 35 insertions(+), 29 deletions(-)
include/linux/dynamic_debug.h +35 -29
index feaac1e..699f533 100644
@@ -54,35 +54,41 @@ extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
-#define dynamic_pr_debug(fmt, ...) do {					\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
-	if (unlikely(descriptor.enabled))				\
-		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
-	} while (0)
-
-#define dynamic_dev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
-	if (unlikely(descriptor.enabled))				\
-		__dynamic_dev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);	\
-	} while (0)
-
-#define dynamic_netdev_dbg(dev, fmt, ...) do {				\
-	static struct _ddebug descriptor				\
-	__used								\
-	__attribute__((section("__verbose"), aligned(8))) =		\
-	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
-		_DPRINTK_FLAGS_DEFAULT };				\
-	if (unlikely(descriptor.enabled))				\
-		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
-	} while (0)
+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+	static struct _ddebug __used __aligned(8)		\
+	__attribute__((section("__verbose"))) name = {		\
+		.modname = KBUILD_MODNAME,			\
+		.function = __func__,				\
+		.filename = __FILE__,				\
+		.format = fmt,					\
+		.lineno = __LINE__,				\
+		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
+		.enabled = false,				\
+	}
+
+#define dynamic_pr_debug(fmt, ...)				\
+do {								\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
+	if (unlikely(descriptor.enabled))			\
+		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),	\
+				   ##__VA_ARGS__);		\
+} while (0)
+
+#define dynamic_dev_dbg(dev, fmt, ...)				\
+do {								\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
+	if (unlikely(descriptor.enabled))			\
+		__dynamic_dev_dbg(&descriptor, dev, fmt,	\
+				  ##__VA_ARGS__);		\
+} while (0)
+
+#define dynamic_netdev_dbg(dev, fmt, ...)			\
+do {								\
+	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
+	if (unlikely(descriptor.enabled))			\
+		__dynamic_netdev_dbg(&descriptor, dev, fmt,	\
+				     ##__VA_ARGS__);		\
+} while (0)
 
 #else
 
Signature · 1 line
1.7.5.4
#22 lore raw

Re: [PATCH 09/11 re-post take #2] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

Quoted 2 lines · Jason Baron

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro.

[]

include/linux/dynamic_debug.h

Quoted 1 line
include/linux/dynamic_debug.h +0 -0

[]

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 59

Quoted 3 lines
include/linux/dynamic_debug.h +3 -0
+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+	static struct _ddebug __used __aligned(8)		\
+	__attribute__((section("__verbose"))) name = {		\

[]

Jason, just one more thing...

Because the original struct _ddebug definition
above this uses __attribute__((aligned(8))),

struct _ddebug {
	[...]
} __attribute__((aligned(8)));

( and I suppose that should be __aligned(8) instead )

the __aligned(8) use in DECLARE_DYNAMIC_DEBUG_METADATA is not necessary.

cheers, Joe

#23 lore raw

Re: [PATCH 09/11 re-post take #2] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

Quoted 20 lines · Joe Perches
Quoted 2 lines · Jason Baron

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro.

[]

include/linux/dynamic_debug.h

Quoted 1 line
include/linux/dynamic_debug.h +0 -0

[]

include/linux/dynamic_debug.h extern int __dynamic_netdev_dbg(struct _ddebug *descriptor, · line 59

Quoted 3 lines
include/linux/dynamic_debug.h +3 -0
+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+	static struct _ddebug __used __aligned(8)		\
+	__attribute__((section("__verbose"))) name = {		\

[]

Jason, just one more thing...

Because the original struct _ddebug definition
above this uses __attribute__((aligned(8))),

struct _ddebug {
	[...]
} __attribute__((aligned(8)));
Quoted 7 lines

( and I suppose that should be __aligned(8) instead )

the __aligned(8) use in DECLARE_DYNAMIC_DEBUG_METADATA is not necessary.

cheers, Joe

Hi Joe,

hmmm....what we probably need here, is an extra array of pointers into the dynamic debug structures. The reason being is that 'aligned' only specifies the minimum alignment, so the dynamic debug structures could be aligned to higher multiples, thus leaving extra padding. Since the dynamic debug code relies on a contiguous array this could lead to NULl pointer exceptions. This has been implemented for tracepoints, see:
https://lkml.org/lkml/2011/1/26/463. I haven't seen this issue in practice though...

So I'm going to leave the extra alignment specifications for now (they can't hurt), and at some point we probably need to look at the approach mentioned above.

Thanks,

-Jason

#24 lore raw

PATCH 10/11 dynamic_debug: remove num_enabled accounting

In reply to Jason Baron

From: Jason Baron <[email protected]>

The num_enabled accouting aren't actually used anywhere - remove them.

---
 lib/dynamic_debug.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)
lib/dynamic_debug.c +0 -7
index ee3b9ba..198d2af 100644
@@ -42,7 +42,6 @@ struct ddebug_table {
 	struct list_head link;
 	char *mod_name;
 	unsigned int num_ddebugs;
-	unsigned int num_enabled;
 	struct _ddebug *ddebugs;
 };
 
@@ -152,11 +151,6 @@ static void ddebug_change(const struct ddebug_query *query,
 			newflags = (dp->flags & mask) | flags;
 			if (newflags == dp->flags)
 				continue;
-
-			if (!newflags)
-				dt->num_enabled--;
-			else if (!dp->flags)
-				dt->num_enabled++;
 			dp->flags = newflags;
 			if (newflags)
 				dp->enabled = 1;
@@ -764,7 +758,6 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 	}
 	dt->mod_name = new_name;
 	dt->num_ddebugs = n;
-	dt->num_enabled = 0;
 	dt->ddebugs = tab;
 
 	mutex_lock(&ddebug_lock);
Signature · 1 line
1.7.5.4
#25 lore raw

PATCH 11/11 dynamic_debug: use a single printk() to emit msgs

In reply to Jason Baron

From: Jason Baron <[email protected]>

We were using KERN_CONT to combine msgs with their prefix. However, KERN_CONT is not smp safe, in the sense that it can interleave messages. This interleaving can result in printks coming out at the wrong loglevel. With the high frequency of printks, that dynamic debug can produce, this is not desirable.

Thus, make dynamic_emit_prefix(), fill a char buf[64], instead of doing a printk directly. If we enable printing out of function, module, line, or pid info, they are placed in this 64 byte buffer. In my testing 64 bytes was enough size to fulfill all requests. Even if its not, we can match up the printk itself to see where its from, so to me this is no big deal.

---
 lib/dynamic_debug.c |   66 +++++++++++++++++++++------------------------------
 1 files changed, 27 insertions(+), 39 deletions(-)
lib/dynamic_debug.c +27 -39
index 198d2af..1f11978 100644
@@ -422,52 +422,46 @@ static int ddebug_exec_query(char *query_string)
 	return 0;
 }
 
-static int dynamic_emit_prefix(const struct _ddebug *descriptor)
+#define PREFIX_SIZE 64
+#define LEFT(wrote) ((PREFIX_SIZE - wrote) > 0) ? (PREFIX_SIZE - wrote) : 0
+
+static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 {
-	char tid[sizeof(int) + sizeof(int)/2 + 4];
-	char lineno[sizeof(int) + sizeof(int)/2];
+	int pos = 0;
 
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_TID) {
+	pos += snprintf(buf + pos, LEFT(pos), "%s", KERN_DEBUG);
+	if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
-			snprintf(tid, sizeof(tid), "%s", "<intr> ");
+			pos += snprintf(buf + pos, LEFT(pos), "%s ",
+						"<intr>");
 		else
-			snprintf(tid, sizeof(tid), "[%d] ",
-				 task_pid_vnr(current));
-	} else {
-		tid[0] = 0;
+			pos += snprintf(buf + pos, LEFT(pos), "[%d] ",
+						task_pid_vnr(current));
 	}
+	if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+		pos += snprintf(buf + pos, LEFT(pos), "%s:", desc->modname);
+	if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+		pos += snprintf(buf + pos, LEFT(pos), "%s:", desc->function);
+	if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
+		pos += snprintf(buf + pos, LEFT(pos), "%d ", desc->lineno);
 
-	if (descriptor->flags & _DPRINTK_FLAGS_INCL_LINENO)
-		snprintf(lineno, sizeof(lineno), "%d", descriptor->lineno);
-	else
-		lineno[0] = 0;
-
-	return printk(KERN_DEBUG "%s%s%s%s%s%s",
-		      tid,
-		      (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME) ?
-		      descriptor->modname : "",
-		      (descriptor->flags & _DPRINTK_FLAGS_INCL_MODNAME) ?
-		      ":" : "",
-		      (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME) ?
-		      descriptor->function : "",
-		      (descriptor->flags & _DPRINTK_FLAGS_INCL_FUNCNAME) ?
-		      ":" : "",
-		      lineno);
+	return buf;
 }
 
 int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 {
 	va_list args;
 	int res;
+	struct va_format vaf;
+	char buf[PREFIX_SIZE];
 
 	BUG_ON(!descriptor);
 	BUG_ON(!fmt);
 
 	va_start(args, fmt);
-
-	res = dynamic_emit_prefix(descriptor);
-	res += vprintk(fmt, args);
-
+	vaf.fmt = fmt;
+	vaf.va = &args;
+	res = printk("%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
 	va_end(args);
 
 	return res;
@@ -480,18 +474,15 @@ int __dynamic_dev_dbg(struct _ddebug *descriptor,
 	struct va_format vaf;
 	va_list args;
 	int res;
+	char buf[PREFIX_SIZE];
 
 	BUG_ON(!descriptor);
 	BUG_ON(!fmt);
 
 	va_start(args, fmt);
-
 	vaf.fmt = fmt;
 	vaf.va = &args;
-
-	res = dynamic_emit_prefix(descriptor);
-	res += __dev_printk(KERN_CONT, dev, &vaf);
-
+	res = __dev_printk(dynamic_emit_prefix(descriptor, buf), dev, &vaf);
 	va_end(args);
 
 	return res;
@@ -504,18 +495,15 @@ int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 	struct va_format vaf;
 	va_list args;
 	int res;
+	char buf[PREFIX_SIZE];
 
 	BUG_ON(!descriptor);
 	BUG_ON(!fmt);
 
 	va_start(args, fmt);
-
 	vaf.fmt = fmt;
 	vaf.va = &args;
-
-	res = dynamic_emit_prefix(descriptor);
-	res += __netdev_printk(KERN_CONT, dev, &vaf);
-
+	res = __netdev_printk(dynamic_emit_prefix(descriptor, buf), dev, &vaf);
 	va_end(args);
 
 	return res;
Signature · 1 line
1.7.5.4
#26 lore raw
In reply to Jason Baron
Quoted 4 lines · Jason Baron

Hi,

Dynamic debug fixes and cleanups. Only changes from v2 are a re-base against Linus's latest tree and the inclusion of the re-posted version for patch #11.

I applied the first 8, as it seemed like there was lots of discussion about #9.

Care to send the rest when you all have those worked out?

thanks,

greg k-h

#27 lore raw
Quoted 11 lines · Greg KH
Quoted 4 lines · Jason Baron

Hi,

Dynamic debug fixes and cleanups. Only changes from v2 are a re-base against Linus's latest tree and the inclusion of the re-posted version for patch #11.

I applied the first 8, as it seemed like there was lots of discussion about #9.

Care to send the rest when you all have those worked out?

Hi Greg,

The comments on #9 were mostly around formatting. The 'take #2' version is fine: https://lkml.org/lkml/2011/8/15/395. Do you need me to re-post it? If not please also take 10, 11.

Thanks,

-Jason

#28 lore raw
Quoted 10 lines · Jason Baron
Quoted 9 lines · Greg KH
Quoted 4 lines · Jason Baron

Hi,

Dynamic debug fixes and cleanups. Only changes from v2 are a re-base against Linus's latest tree and the inclusion of the re-posted version for patch #11.

I applied the first 8, as it seemed like there was lots of discussion about #9.

Quoted 8 lines
Quoted 2 lines

Care to send the rest when you all have those worked out?

Hi Greg,

The comments on #9 were mostly around formatting. The 'take #2' version is fine: https://lkml.org/lkml/2011/8/15/395. Do you need me to re-post it? If not please also take 10, 11.

As stated above, pleae resend what you want me to apply.

thanks,

greg k-h