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

Jason Baron
2011-08-15T20:44:48Z

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