[PATCH 0/4] dynamic debug: cleanups + compile fix

[PATCH 0/4] dynamic debug: cleanups + compile fix

[Jason Baron]

Hi,

This is a re-post of the last 3 patches, as requested by Greg:
https://lkml.org/lkml/2011/8/11/378

I've also added a compile fix that Randy Dunlap found in the -next tree.

Thanks,

-Jason


Jason Baron (4):
  dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions
  dynamic_debug: remove num_enabled accounting
  dynamic_debug: use a single printk() to emit msgs
  dynamic_debug: fix undefined reference to `__netdev_printk'

 include/linux/dynamic_debug.h |   64 ++++++++++++++++++---------------
 lib/dynamic_debug.c           |   77 ++++++++++++++++------------------------
 2 files changed, 66 insertions(+), 75 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

[Jason Baron]

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

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 include/linux/dynamic_debug.h |   64 ++++++++++++++++++++++------------------
 1 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index feaac1e..699f533 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -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
 
-- 
1.7.5.4

[PATCH 2/4] dynamic_debug: remove num_enabled accounting

[Jason Baron]

The num_enabled accounting isn't actually used anywhere - remove them.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 lib/dynamic_debug.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ee3b9ba..198d2af 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -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);
-- 
1.7.5.4

[PATCH 3/4] dynamic_debug: use a single printk() to emit msgs

[Jason Baron]

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.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 lib/dynamic_debug.c |   66 +++++++++++++++++++++------------------------------
 1 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 198d2af..1f11978 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -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;
-- 
1.7.5.4

[PATCH 4/4] dynamic_debug: fix undefined reference to `__netdev_printk'

[Jason Baron]

Dynamic debug recently added a call to __netdev_printk(). However, when
CONFIG_NET is not set, we get the following error:

lib/built-in.o: In function `__dynamic_netdev_dbg':
(.text+0x9fda): undefined reference to `__netdev_printk'

Fix this by making the call to __netdev_printk() contingent upon
CONFIG_NET. We could have fixed this by defining __netdev_printk()
to a 'no-op' in the !CONFIG_NET case. However, this is not
consistent with how the networking layer uses netdev_printk().
For example, when CONFIG_NET is not set, netdev_printk() does not
have a 'no-op' definition defined.

Signed-off-by: Jason Baron <jbaron@redhat.com>
Acked-by: Randy Dunlap <rdunlap@xenotime.net>
---
 lib/dynamic_debug.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 1f11978..bda4036 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -489,6 +489,8 @@ 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, ...)
 {
@@ -510,6 +512,8 @@ int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 }
 EXPORT_SYMBOL(__dynamic_netdev_dbg);
 
+#endif
+
 static __initdata char ddebug_setup_string[1024];
 static __init int ddebug_setup_query(char *str)
 {
-- 
1.7.5.4

Re: [PATCH 3/4] dynamic_debug: use a single printk() to emit msgs

[Bart Van Assche]

On Thu, Aug 25, 2011 at 7:34 PM, Jason Baron <jbaron@redhat.com> wrote:
> -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;
>  }

Shouldn't the new implementation guarantee that buf[] is
'\0'-terminated if pos >= PREFIX_SIZE ?

Bart.

Re: [PATCH 3/4 re-post] dynamic_debug: use a single printk() to emit msgs

[Jason Baron]

On Thu, Aug 25, 2011 at 07:42:19PM +0200, Bart Van Assche wrote:
> On Thu, Aug 25, 2011 at 7:34 PM, Jason Baron <jbaron@redhat.com> wrote:
> > -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;
> >  }
> 
> Shouldn't the new implementation guarantee that buf[] is
> '\0'-terminated if pos >= PREFIX_SIZE ?
> 
> Bart.

Yes, good catch. Here is a re-post.

thanks,

-Jason

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.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 lib/dynamic_debug.c |   69 ++++++++++++++++++++++----------------------------
 1 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 198d2af..f5def31 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -422,52 +422,49 @@ 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);
+	if (pos >= PREFIX_SIZE)
+		buf[PREFIX_SIZE - 1] = '\0';
+
+	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 +477,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 +498,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;
-- 
1.7.5.4

Re: [PATCH 1/4] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

[Bart Van Assche]

On Thu, Aug 25, 2011 at 7:34 PM, Jason Baron <jbaron@redhat.com> wrote:
>
> +#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,                               \
> +       }

A minor remark: macro arguments are usually surrounded by parentheses
in a macro definition. That's not the case for "fmt" in the above.

Bart.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/4] dynamic debug: cleanups + compile fix

[Greg KH]

On Thu, Aug 25, 2011 at 01:34:06PM -0400, Jason Baron wrote:
> Hi,
> 
> This is a re-post of the last 3 patches, as requested by Greg:
> https://lkml.org/lkml/2011/8/11/378
> 
> I've also added a compile fix that Randy Dunlap found in the -next tree.

Care to redo this based on the comments received so far?

thanks,

greg k-h

Re: [PATCH 0/4] dynamic debug: cleanups + compile fix

[Jason Baron]

On Fri, Aug 26, 2011 at 11:29:00AM -0700, Greg KH wrote:
> On Thu, Aug 25, 2011 at 01:34:06PM -0400, Jason Baron wrote:
> > Hi,
> > 
> > This is a re-post of the last 3 patches, as requested by Greg:
> > https://lkml.org/lkml/2011/8/11/378
> > 
> > I've also added a compile fix that Randy Dunlap found in the -next tree.
> 
> Care to redo this based on the comments received so far?
> 

Hi,

I re-posted 3/4 based on the comments already. The comment on patch #1
regarding adding macro parenthesis, I don't believe makes any functional
change...do you still need a re-post the series?

Thanks,

-Jason

Re: [PATCH 0/4] dynamic debug: cleanups + compile fix

[Greg KH]

On Fri, Aug 26, 2011 at 02:47:42PM -0400, Jason Baron wrote:
> On Fri, Aug 26, 2011 at 11:29:00AM -0700, Greg KH wrote:
> > On Thu, Aug 25, 2011 at 01:34:06PM -0400, Jason Baron wrote:
> > > Hi,
> > > 
> > > This is a re-post of the last 3 patches, as requested by Greg:
> > > https://lkml.org/lkml/2011/8/11/378
> > > 
> > > I've also added a compile fix that Randy Dunlap found in the -next tree.
> > 
> > Care to redo this based on the comments received so far?
> > 
> 
> Hi,
> 
> I re-posted 3/4 based on the comments already. The comment on patch #1
> regarding adding macro parenthesis, I don't believe makes any functional
> change...do you still need a re-post the series?

Yes, please fix patch #1 adding the needed parenthesis and resend the
whole series.

thanks,

greg k-h

Re: [PATCH 3/4] dynamic_debug: use a single printk() to emit msgs

[Jim Cromie]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

On Thu, Aug 25, 2011 at 11:34 AM, Jason Baron <jbaron@redhat.com> wrote:

> +       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);

this inserts a space after lineno, but not otherwize.

I have patch to fix it, but maybe you want to fix it it here ?

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9c8e133..122246f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -439,7 +439,9 @@ static char *dynamic_emit_prefix(const struct
_ddebug *desc, char *buf)
 	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);
+		pos += snprintf(buf + pos, LEFT(pos), "%d:", desc->lineno);
+	if (pos - strlen(KERN_DEBUG))
+		pos += snprintf(buf + pos, LEFT(pos), " ");

 	return buf;
 }
-- 
1.7.4.4


also attached.
If it doesnt apply, or you defer, I'll resend once Greg's accepted
your whole set.

[-- Attachment #2: 0003-dynamic_debug-add-space-at-end-of-non-null-dynamic-p.patch --]
[-- Type: text/x-patch, Size: 1177 bytes --]

From ec75cd50ba89bed1f0ad74b89c9a43e87b2bd767 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Tue, 2 Aug 2011 15:12:55 -0600
Subject: [PATCH 03/28] dynamic_debug: add space at end of non-null
 dynamic-prefix

dynamic_emit_prefix() currently adds a space after line-number,
but not otherwize.  For readability, add space whenever any prefix
(besides the "<7>" loglevel) has been written.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9c8e133..122246f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -439,7 +439,9 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 	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);
+		pos += snprintf(buf + pos, LEFT(pos), "%d:", desc->lineno);
+	if (pos - strlen(KERN_DEBUG))
+		pos += snprintf(buf + pos, LEFT(pos), " ");
 
 	return buf;
 }
-- 
1.7.4.4

Re: [PATCH 0/4] dynamic debug: cleanups + compile fix

[Joe Perches]

On Fri, 2011-08-26 at 12:04 -0700, Greg KH wrote:
> On Fri, Aug 26, 2011 at 02:47:42PM -0400, Jason Baron wrote:
> > On Fri, Aug 26, 2011 at 11:29:00AM -0700, Greg KH wrote:
> > > On Thu, Aug 25, 2011 at 01:34:06PM -0400, Jason Baron wrote:
> > > > Hi,
> > > > 
> > > > This is a re-post of the last 3 patches, as requested by Greg:
> > > > https://lkml.org/lkml/2011/8/11/378
> > > > 
> > > > I've also added a compile fix that Randy Dunlap found in the -next tree.
> > > 
> > > Care to redo this based on the comments received so far?
> > > 
> > 
> > Hi,
> > 
> > I re-posted 3/4 based on the comments already. The comment on patch #1
> > regarding adding macro parenthesis, I don't believe makes any functional
> > change...do you still need a re-post the series?
> 
> Yes, please fix patch #1 adding the needed parenthesis and resend the
> whole series.

The parentheses aren't need.

A macro like
#define foo(a, b) a = b

that only uses each argument once without doing arithmetic
gains nothing from parentheses.

Just like
#define true 1
gains nothing from parenthesis either.

Re: [PATCH 0/4] dynamic debug: cleanups + compile fix

[Greg KH]

On Fri, Aug 26, 2011 at 03:14:41PM -0700, Joe Perches wrote:
> On Fri, 2011-08-26 at 12:04 -0700, Greg KH wrote:
> > On Fri, Aug 26, 2011 at 02:47:42PM -0400, Jason Baron wrote:
> > > On Fri, Aug 26, 2011 at 11:29:00AM -0700, Greg KH wrote:
> > > > On Thu, Aug 25, 2011 at 01:34:06PM -0400, Jason Baron wrote:
> > > > > Hi,
> > > > > 
> > > > > This is a re-post of the last 3 patches, as requested by Greg:
> > > > > https://lkml.org/lkml/2011/8/11/378
> > > > > 
> > > > > I've also added a compile fix that Randy Dunlap found in the -next tree.
> > > > 
> > > > Care to redo this based on the comments received so far?
> > > > 
> > > 
> > > Hi,
> > > 
> > > I re-posted 3/4 based on the comments already. The comment on patch #1
> > > regarding adding macro parenthesis, I don't believe makes any functional
> > > change...do you still need a re-post the series?
> > 
> > Yes, please fix patch #1 adding the needed parenthesis and resend the
> > whole series.
> 
> The parentheses aren't need.
> 
> A macro like
> #define foo(a, b) a = b
> 
> that only uses each argument once without doing arithmetic
> gains nothing from parentheses.
> 
> Just like
> #define true 1
> gains nothing from parenthesis either.

I don't think that is the case here, right?

greg k-h

Re: [PATCH 0/4] dynamic debug: cleanups + compile fix

[Joe Perches]

On Fri, 2011-08-26 at 15:18 -0700, Greg KH wrote:
> On Fri, Aug 26, 2011 at 03:14:41PM -0700, Joe Perches wrote:
> > On Fri, 2011-08-26 at 12:04 -0700, Greg KH wrote:
> > > > > Care to redo this based on the comments received so far?
> > > > I re-posted 3/4 based on the comments already. The comment on patch #1
> > > > regarding adding macro parenthesis, I don't believe makes any functional
> > > > change...do you still need a re-post the series?
> > #define true 1
> > gains nothing from parenthesis either.
> I don't think that is the case here, right?

Nope.

It is the case, the parentheses are not necessary.

Re: [PATCH 3/4] dynamic_debug: use a single printk() to emit msgs

[Jason Baron]

On Fri, Aug 26, 2011 at 03:47:49PM -0600, Jim Cromie wrote:
> On Thu, Aug 25, 2011 at 11:34 AM, Jason Baron <jbaron@redhat.com> wrote:
> 
> > +       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);
> 
> this inserts a space after lineno, but not otherwize.
> 
> I have patch to fix it, but maybe you want to fix it it here ?
> 

Hi Jim,

Thanks for your fix...since Greg hasn't taken the series yet...I guess
I'll re-spin to include this fix. Probably wouldn't send it out until
tomorrow due to my test systems being down atm...

thanks,

-Jason


> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 9c8e133..122246f 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -439,7 +439,9 @@ static char *dynamic_emit_prefix(const struct
> _ddebug *desc, char *buf)
>  	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);
> +		pos += snprintf(buf + pos, LEFT(pos), "%d:", desc->lineno);
> +	if (pos - strlen(KERN_DEBUG))
> +		pos += snprintf(buf + pos, LEFT(pos), " ");
> 
>  	return buf;
>  }
> -- 
> 1.7.4.4
> 
> 
> also attached.
> If it doesnt apply, or you defer, I'll resend once Greg's accepted
> your whole set.

> From ec75cd50ba89bed1f0ad74b89c9a43e87b2bd767 Mon Sep 17 00:00:00 2001
> From: Jim Cromie <jim.cromie@gmail.com>
> Date: Tue, 2 Aug 2011 15:12:55 -0600
> Subject: [PATCH 03/28] dynamic_debug: add space at end of non-null
>  dynamic-prefix
> 
> dynamic_emit_prefix() currently adds a space after line-number,
> but not otherwize.  For readability, add space whenever any prefix
> (besides the "<7>" loglevel) has been written.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 9c8e133..122246f 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -439,7 +439,9 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
>  	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);
> +		pos += snprintf(buf + pos, LEFT(pos), "%d:", desc->lineno);
> +	if (pos - strlen(KERN_DEBUG))
> +		pos += snprintf(buf + pos, LEFT(pos), " ");
>  
>  	return buf;
>  }
> -- 
> 1.7.4.4
>