dynamic debug: cleanups + compile fix v2

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

...

(quoted diff) +1 -0

+#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\

Review

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

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

(quoted diff) +11 -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,				\
+	}

<anal>That macro implements a definition, not a declaration</anal>

Quoted 16 lines · Andrew Morton

Review

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

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

(quoted diff) +11 -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,				\
+	}

<anal>That macro implements a definition, not a declaration</anal>

Andrew, that's not quite true

and that's how
DECLARE is normally used in linux.

DECLARE_BITMAP, DECLARE_COMPLETION, DECLARE_RWSEM, etc.

Quoted 17 lines · On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <[email protected]> wrote:

Quoted 16 lines · Andrew Morton

Review

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

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

(quoted diff) +11 -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,				\
+	}

<anal>That macro implements a definition, not a declaration</anal>

Quoted 1 line

Andrew, that's not quite true

It's precisely true.

Quoted 18 lines · Andrew Morton

Quoted 17 lines · On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <[email protected]> wrote:

Quoted 16 lines · Andrew Morton

Review

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

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

(quoted diff) +11 -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,				\
+	}

<anal>That macro implements a definition, not a declaration</anal>

Quoted 2 lines

Quoted 1 line

Andrew, that's not quite true

It's precisely true.

Not according to the c99 standard section 6.7

The reason the distinction matters (apart from being just wrong) is that sometimes we want a macro for the definition and another for the declaration. DEFINE_TRACE/DECLARE_TRACE, DECLARE_LGLOCK/DEFINE_LGLOCK, etc.

Quoted 17 lines · Andrew Morton

Quoted 16 lines · On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <[email protected]> wrote:

Quoted 15 lines · Andrew Morton

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro. +#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,                               \
+       }

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

<anal>That macro implements a definition, not a declaration</anal>

Andrew, that's not quite true

It's precisely true.

Not according to the c99 standard section 6.7

Quoted 18 lines · Joe Perches

Quoted 17 lines · Andrew Morton

Quoted 16 lines · On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <[email protected]> wrote:

Quoted 15 lines · Andrew Morton

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro. +#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,                               \
+       }

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

<anal>That macro implements a definition, not a declaration</anal>

Andrew, that's not quite true

It's precisely true.

Not according to the c99 standard section 6.7

Have you read that paragraph ? This is what I found in paragraph 6.7, which confirms Andrews interpretation:

<quote>
A declaration specifies the interpretation and attributes of a set of identifiers. A definition of an identifier is a declaration for that identifier that:
- for an object, causes storage to be reserved for that object;
- for a function, includes the function body;
- for an enumeration constant or typedef name, is the (only) declaration of the identifier.
</quote>

Bart.

It also allows for "lite" usage of dynamic-debug,
including 1..few descriptor per file or module to control all debug printing. As outlined, this "lite" usage is determined by the coder,
it would be cool if it were more configurable than that,
but I dont see how that would work atm.

Now that the worms have escaped the can, one other thought:

	unsigned int lineno:24;

allows for insanely large files. The largest in the tree is 29k, 16 bits would cover all files likely to be accepted in the future.

Since since it works with module/file/function filtering, 3-4 user flags should be plenty.

Quoted 5 lines · Jim Cromie

It also allows for "lite" usage of dynamic-debug,
including 1..few descriptor per file or module to control all debug printing. As outlined, this "lite" usage is determined by the coder,
it would be cool if it were more configurable than that,
but I dont see how that would work atm.

General problem is that there are subsystems that use bitmasks with more than a just a few bits defined.

Quoted 4 lines

Now that the worms have escaped the can, one other thought:
      unsigned int lineno:24;
allows for insanely large files.  The largest in the tree is 29k, 16 bits would cover all files likely to be accepted in the future.

I hope ever.

Quoted 2 lines

Since since it works with module/file/function filtering, 3-4 user flags should be plenty.

I rather doubt it as there are both level and mask uses.

There is no real issue with adding another flag value to the struct anyway.  It's declared __aligned(8) and it ends in an unaligned single char so there's space for another u32 without extending the structure size.

The trick is then to allow those users to define flag-chars that they like (mnemonic value), and that work with the dynamic-debug mini-language implemented in $CONTROL reader.

But that begs the question - is there a reason why __aligned(4)
wouldnt work for 32 bit machines ? It would reduce the footprint
for small machines.

Also, what is your notion of level ?

it is independent of the error,warning,info,debug levels correct ?

Quoted 3 lines

The trick is then to allow those users to define flag-chars that they like (mnemonic value), and that work with the dynamic-debug mini-language implemented in $CONTROL reader.

I don't think that's a problem really.

I think a simple control/test variable as either value or mask would work fine.

Take a single bit control to either test as mask or test as level. No mnemonic letters needed.

pr_debug_mask(mask, fmt, ...)
pr_debug_level(level, fmt, ...)

could set the appropriate type bit in the struct so the test works as appropriate.

echo 'value <foo>' > <debugfs>/dynamic_debug/control

Quoted 3 lines

But that begs the question - is there a reason why __aligned(4)
wouldnt work for 32 bit machines ? It would reduce the footprint
for small machines.

No real idea why it's aligned(8). Jason?

Quoted 2 lines

No real idea why it's aligned(8).  Jason?

I think when I first implemented it, I noticed that subsystems that did similar things, set aligned(8) in include/asm-generic/vmlinux.lds.h. For example, FTRACE_EVENTS() sets ALIGN(8)...I know that not a great reason thouh :( Don't remember if I tried ALIGN(4), but if it saves space, we can look at it...

Also, as I mentioned before, we probably need an extra array of pointers into the dynamic debug structures. see: https://lkml.org/lkml/2011/1/26/463.

thanks,

-Jason

Quoted 37 lines · Bart Van Assche

Quoted 18 lines · Joe Perches

Quoted 17 lines · Andrew Morton

Quoted 16 lines · On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <[email protected]> wrote:

Quoted 15 lines · Andrew Morton

Quoted 13 lines · On Tue, 30 Aug 2011 14:28:41 -0400 Jason Baron <[email protected]> wrote:

Replace the repetitive struct _ddebug descriptor definitions with a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro. +#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,                               \
+       }

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

<anal>That macro implements a definition, not a declaration</anal>

Andrew, that's not quite true

It's precisely true.

Not according to the c99 standard section 6.7

Have you read that paragraph ? This is what I found in paragraph 6.7, which confirms Andrews interpretation:

<quote>
A declaration specifies the interpretation and attributes of a set of identifiers. A definition of an identifier is a declaration for that identifier that:
- for an object, causes storage to be reserved for that object;
- for a function, includes the function body;
- for an enumeration constant or typedef name, is the (only) declaration of the identifier.
</quote>

Quoted 2 lines

Bart.

I hesitate to churn this more (I have patchset to go on top of all this) but

Id like to see an INIT_DYNAMIC_DEBUG_METADATA,
along with ability to expose the descriptor.

This would support pr_dbg_cont(), by letting it see/reuse the same descriptor that controls the pr_debug that started the multi-call message. While this defeats the "atomicity" of buffering the entire message before calling printk, it does so only for the actual uses of KERN_CONT.

It also allows for "lite" usage of dynamic-debug,
including 1..few descriptor per file or module to control all debug printing. As outlined, this "lite" usage is determined by the coder,
it would be cool if it were more configurable than that,
but I dont see how that would work atm.

Now that the worms have escaped the can, one other thought:

	unsigned int lineno:24;

allows for insanely large files. The largest in the tree is 29k, 16 bits would cover all files likely to be accepted in the future. Even allowing for never-to-be-submitted machine-generated code, Id think 18 bits would suffice. ~250k lines should be enough ;-)

Given that my patchset adds flags-filtering
( echo mt+p > $CONTROL )
The availability of user-flags, which do nothing but mark callsites, has some value - user can mark arbitrary sets of callsites, then enable/disable them as a group or subgroup:

echo function foo +x > $CONTROL
...
echo x+p > $CONTROL
echo y-p > $CONTROL
echo z+p > $CONTROL
echo yz-p > $CONTROL

Since since it works with module/file/function filtering, 3-4 user flags should be plenty.

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.

...

+#define LEFT(wrote) ((PREFIX_SIZE - wrote) > 0) ? (PREFIX_SIZE - wrote) : 0

Dynamic debug recently added support for netdev_printk. It uses __netdev_printk() to support this functionality. 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, CONFIG_NET is not set, netdev_printk() does not have a 'no-op' definition defined.

Signed-off-by Jason Baron <[email protected]>

Acked-by Randy Dunlap <[email protected]>