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

[Jason Baron <jbaron@redhat.com>]

Hi Joe,

On Thu, Aug 11, 2011 at 11:39:24PM -0700, Joe Perches wrote:
> On Thu, 2011-08-11 at 16:52 -0400, Jason Baron wrote:
> > Replace the repetitive struct _ddebug descriptor definitions with
> > a new DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt) macro.
> []
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> []
> > @@ -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.
> 
> From a kernel coding style consistency POV, shouldn't
> this use c99 named initializers?

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

> 
> #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?

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

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

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

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