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

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

[Jason Baron]

Hi,

Re-post of: https://lkml.org/lkml/2011/8/25/493

changes since v1:
-ensure that dynamic_emit_prefix() adds a trailing space, where appropriate,
 and terminates strings with a '\0'.
-add a parentheses around the 'fmt' macro argument (although it has no effect)

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           |   83 ++++++++++++++++++----------------------
 2 files changed, 72 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..8aec680 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

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

[Andrew Morton]

On Tue, 30 Aug 2011 14:28:41 -0400
Jason Baron <jbaron@redhat.com> 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,				\
> +	}

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

--- a/include/linux/dynamic_debug.h~dynamic_debug-consolidate-repetitive-struct-_ddebug-descriptor-definitions-fix
+++ a/include/linux/dynamic_debug.h
@@ -54,7 +54,7 @@ extern int __dynamic_netdev_dbg(struct _
 			     const char *fmt, ...)
 	__attribute__ ((format (printf, 3, 4)));
 
-#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
 	static struct _ddebug __used __aligned(8)		\
 	__attribute__((section("__verbose"))) name = {		\
 		.modname = KBUILD_MODNAME,			\
@@ -68,7 +68,7 @@ extern int __dynamic_netdev_dbg(struct _
 
 #define dynamic_pr_debug(fmt, ...)				\
 do {								\
-	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
+	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
 	if (unlikely(descriptor.enabled))			\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),	\
 				   ##__VA_ARGS__);		\
@@ -76,7 +76,7 @@ do {								\
 
 #define dynamic_dev_dbg(dev, fmt, ...)				\
 do {								\
-	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
+	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
 	if (unlikely(descriptor.enabled))			\
 		__dynamic_dev_dbg(&descriptor, dev, fmt,	\
 				  ##__VA_ARGS__);		\
@@ -84,7 +84,7 @@ do {								\
 
 #define dynamic_netdev_dbg(dev, fmt, ...)			\
 do {								\
-	DECLARE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);	\
+	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);		\
 	if (unlikely(descriptor.enabled))			\
 		__dynamic_netdev_dbg(&descriptor, dev, fmt,	\
 				     ##__VA_ARGS__);		\
_

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

[Joe Perches]

On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> On Tue, 30 Aug 2011 14:28:41 -0400
> Jason Baron <jbaron@redhat.com> 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,				\
> > +	}
> <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.

A lot of the DEFINE_FOO(name) macros use ##name.

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

[Andrew Morton]

On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:

> On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> > On Tue, 30 Aug 2011 14:28:41 -0400
> > Jason Baron <jbaron@redhat.com> 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,				\
> > > +	}
> > <anal>That macro implements a definition, not a declaration</anal>
> 
> Andrew, that's not quite true

It's precisely true.

: Exercise 2.8
: 
: A declaration introduces a name and a type for something.  It does not
: necessarily reserve any storage.
: 
: Exercise 2.9
: 
: A definition is a declaration that also reserves storage.

(http://publications.gbdirect.co.uk/c_book/answers/chapter_2.html)

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

It's how it's sometimes used, when we screwed up.

> DECLARE_BITMAP, DECLARE_COMPLETION, DECLARE_RWSEM, etc.

Mistakes.

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

[Joe Perches]

On Thu, 2011-09-08 at 20:42 -0700, Andrew Morton wrote:
> On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:
> > On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> > > On Tue, 30 Aug 2011 14:28:41 -0400
> > > Jason Baron <jbaron@redhat.com> 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,				\
> > > > +	}
> > > <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

cheers, Joe

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

[Andrew Morton]

On Thu, 08 Sep 2011 21:02:43 -0700 Joe Perches <joe@perches.com> wrote:

> On Thu, 2011-09-08 at 20:42 -0700, Andrew Morton wrote:
> > On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:
> > > On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> > > > On Tue, 30 Aug 2011 14:28:41 -0400
> > > > Jason Baron <jbaron@redhat.com> 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,				\
> > > > > +	}
> > > > <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
> 

That doesn't address the distinction at all - it discusses syntax.

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.

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

[Joe Perches]

On Thu, 2011-09-08 at 21:20 -0700, Andrew Morton wrote:
> 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.

That does not mean that these uses are not declarations
as specified by the c99 standard.  They are.

cheers, Joe

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

[Bart Van Assche]

On Fri, Sep 9, 2011 at 6:02 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2011-09-08 at 20:42 -0700, Andrew Morton wrote:
> > On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:
> > > On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> > > > On Tue, 30 Aug 2011 14:28:41 -0400
> > > > Jason Baron <jbaron@redhat.com> 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,                               \
> > > > > +       }
> > > > <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.

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

[Jim Cromie]

On Fri, Sep 9, 2011 at 4:31 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Fri, Sep 9, 2011 at 6:02 AM, Joe Perches <joe@perches.com> wrote:
>> On Thu, 2011-09-08 at 20:42 -0700, Andrew Morton wrote:
>> > On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:
>> > > On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
>> > > > On Tue, 30 Aug 2011 14:28:41 -0400
>> > > > Jason Baron <jbaron@redhat.com> 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,                               \
>> > > > > +       }
>> > > > <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.
>


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.

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

[Joe Perches]

On Fri, 2011-09-09 at 13:23 -0600, Jim Cromie wrote:
> 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.

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

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

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

[Jim Cromie]

On Fri, Sep 9, 2011 at 3:04 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2011-09-09 at 13:23 -0600, Jim Cromie wrote:
>> 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.
>

ah yes.  I was speaking narrowly.  But youre right, it would make
sense to cover all users ad-hoc debug flags.

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.

Giving them the [A-Z1-9] flag space would probably do nicely,
it reserves [a-z] for dynamic-debug itself ([pmflt] currently,
[a_] added by my patchset)
[1-9] is a natural for levels,
[A-Z] is natural for bit-masks.


>> 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.
Indeed ;-)

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

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.

I guess I should try it. its only cpu time.

If aligned(4) works, then trimming lineno to 16 doubles the flagspace
[0-7] levels take 3 bits, leaving 13 for bitmasks.
DYNAMIC_DEBUG_CLAIM_FLAGS('ABCGXYZ')
could map the [A-Z] range into those 13, while preserving mnemonics.

with INIT_DYNAMIC_DEBUG_METADATA_FLAGGED(name, fmt, flagstr)
coders could define the flags which are added to descriptor.flags for
that callsite,
marking them for selectability via flags-filtering;

echo A+p > $CONTROL
echo B+p > $CONTROL
echo C+p > $CONTROL
echo G+p > $CONTROL
echo X+p > $CONTROL
echo Y+p > $CONTROL
echo Z+p > $CONTROL

without requiring that they be marked beforehand:

echo <<HEREDOC
module foo function bar +B
module foo function cow +C
module foo function gator +G
...
HEREDOC > $CONTROL

(Id resist alternation as a flags-filter construct,
this is already in creeping-featurism territory)

Also, what is your notion of level ?
it is independent of the error,warning,info,debug levels correct ?

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

[Joe Perches]

On Fri, 2011-09-09 at 16:06 -0600, Jim Cromie wrote:
> 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

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

> Also, what is your notion of level ?

Driver specific value.
Look at the _dbg uses in drivers/net/wireless/ath for example.
I believe there are only two types of uses in the kernel:
module debug level and module debug mask output controls.

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

Yes.  Attached to debug output only.

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

[Jason Baron]

On Fri, Sep 09, 2011 at 03:32:05PM -0700, Joe Perches wrote:
> > 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
> 
> > 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?
> 

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

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

[Jim Cromie]

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

I tried it here on 32bit (on 586 class machine), it blew up.
I imagine it could be made to work, but its not a gimme.

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

I'll try it again, once this happens.

> thanks,
>
> -Jason
>

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

[Jason Baron]

On Fri, Sep 09, 2011 at 01:23:43PM -0600, Jim Cromie wrote:
> On Fri, Sep 9, 2011 at 4:31 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> > On Fri, Sep 9, 2011 at 6:02 AM, Joe Perches <joe@perches.com> wrote:
> >> On Thu, 2011-09-08 at 20:42 -0700, Andrew Morton wrote:
> >> > On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:
> >> > > On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> >> > > > On Tue, 30 Aug 2011 14:28:41 -0400
> >> > > > Jason Baron <jbaron@redhat.com> 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,                               \
> >> > > > > +       }
> >> > > > <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.
> >
> 
> 
> 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.
> 
> 

We can expose the descriptor, but I that can wait for a folow-up
patchset, especially, since we don't have any consumers at the moment.

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

makes sense...this can also be done is follow-on patchset...

thanks,

-Jason

[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 |   72 +++++++++++++++++++++++---------------------------
 1 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 198d2af..7314e5e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -422,52 +422,52 @@ 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_after_tid;
+	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));
 	}
+	pos_after_tid = pos;
+	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 (pos - pos_after_tid)
+		pos += snprintf(buf + pos, LEFT(pos), " ");
+	if (pos >= PREFIX_SIZE)
+		buf[PREFIX_SIZE - 1] = '\0';
 
-	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 +480,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 +501,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 3/4] dynamic_debug: use a single printk() to emit msgs

[Andrew Morton]

On Tue, 30 Aug 2011 14:28:50 -0400
Jason Baron <jbaron@redhat.com> wrote:

> 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

This macro will misbehave if passed an expression with side effects and
is rather ugly and didn't actually need to be implemented as a macro at
all.

This?

--- a/lib/dynamic_debug.c~dynamic_debug-use-a-single-printk-to-emit-messages-fix
+++ a/lib/dynamic_debug.c
@@ -423,31 +423,39 @@ static int ddebug_exec_query(char *query
 }
 
 #define PREFIX_SIZE 64
-#define LEFT(wrote) ((PREFIX_SIZE - wrote) > 0) ? (PREFIX_SIZE - wrote) : 0
+
+static int remaining(int wrote)
+{
+	if (PREFIX_SIZE - wrote > 0)
+		return PREFIX_SIZE - wrote;
+	return 0;
+}
 
 static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 {
 	int pos_after_tid;
 	int pos = 0;
 
-	pos += snprintf(buf + pos, LEFT(pos), "%s", KERN_DEBUG);
+	pos += snprintf(buf + pos, remaining(pos), "%s", KERN_DEBUG);
 	if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
-			pos += snprintf(buf + pos, LEFT(pos), "%s ",
+			pos += snprintf(buf + pos, remaining(pos), "%s ",
 						"<intr>");
 		else
-			pos += snprintf(buf + pos, LEFT(pos), "[%d] ",
+			pos += snprintf(buf + pos, remaining(pos), "[%d] ",
 						task_pid_vnr(current));
 	}
 	pos_after_tid = pos;
 	if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
-		pos += snprintf(buf + pos, LEFT(pos), "%s:", desc->modname);
+		pos += snprintf(buf + pos, remaining(pos), "%s:",
+					desc->modname);
 	if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
-		pos += snprintf(buf + pos, LEFT(pos), "%s:", desc->function);
+		pos += snprintf(buf + pos, remaining(pos), "%s:",
+					desc->function);
 	if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
-		pos += snprintf(buf + pos, LEFT(pos), "%d:", desc->lineno);
+		pos += snprintf(buf + pos, remaining(pos), "%d:", desc->lineno);
 	if (pos - pos_after_tid)
-		pos += snprintf(buf + pos, LEFT(pos), " ");
+		pos += snprintf(buf + pos, remaining(pos), " ");
 	if (pos >= PREFIX_SIZE)
 		buf[PREFIX_SIZE - 1] = '\0';
 
_

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

[Jason Baron]

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 <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 7314e5e..4138574 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -495,6 +495,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, ...)
 {
@@ -516,6 +518,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 4/4] dynamic_debug: fix undefined reference to `__netdev_printk'

[Arnd Bergmann]

On Tuesday 30 August 2011, Jason Baron wrote:
> 
> 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 <jbaron@redhat.com>
> Acked-by: Randy Dunlap <rdunlap@xenotime.net>

Found it too, came up with the same fix

Acked-by: Arnd Bergmann <arnd@arndb.de>