diff mbox series

[v3,1/7] Documentation: dynamic-debug: Add description of level bitmask

Message ID 20200609104604.1594-2-stanimir.varbanov@linaro.org
State New
Headers show
Series Venus dynamic debug | expand

Commit Message

Stanimir Varbanov June 9, 2020, 10:45 a.m. UTC
This adds description of the level bitmask feature.

Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

---
 Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.17.1

Comments

Matthew Wilcox June 9, 2020, 11:09 a.m. UTC | #1
On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
> +level

> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``

> +    callsite. This will allow to group debug messages and show only those of the

> +    same level.  The -p flag takes precedence over the given level. Note that we can

> +    have up to five groups of debug messages.


That doesn't sound like a "level".  printk has levels.  If you ask for
"level 3" messages, you get messages from levels 0, 1, 2, and 3.  These
seem like "types" or "groups" or something.

> +  // enable all messages in file with 0x01 level bitmask

> +  nullarbor:~ # echo -n 'file foo.c level 0x01 +p' >

> +                                <debugfs>/dynamic_debug/control
Greg KH June 9, 2020, 11:16 a.m. UTC | #2
On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
> This adds description of the level bitmask feature.
> 
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 0dc2eb8e44e5..c2b751fc8a17 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -208,6 +208,12 @@ line
>  	line -1605          // the 1605 lines from line 1 to line 1605
>  	line 1600-          // all lines from line 1600 to the end of the file
>  
> +level
> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
> +    callsite. This will allow to group debug messages and show only those of the
> +    same level.  The -p flag takes precedence over the given level. Note that we can
> +    have up to five groups of debug messages.

As was pointed out, this isn't a "level", it's some arbitrary type of
"grouping".

But step back, why?  What is wrong with the existing control of dynamic
debug messages that you want to add another type of arbitrary grouping
to it?  And who defines that grouping?  Will it be
driver/subsystem/arch/author specific?  Or kernel-wide?

This feels like it could easily get out of hand really quickly.

Why not just use tracepoints if you really want to be fine-grained?

thanks,

greg k-h
Joe Perches June 9, 2020, 4:58 p.m. UTC | #3
On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> What is wrong with the existing control of dynamic
> debug messages that you want to add another type of arbitrary grouping
> to it? 

There is no existing grouping mechanism.

Many drivers and some subsystems used an internal one
before dynamic debug.

$ git grep "MODULE_PARM.*\bdebug\b"|wc -l
501

This is an attempt to unify those homebrew mechanisms.

Stanimir attempted to add one for his driver via a
driver specific standardized format substring for level.

> And who defines that grouping?

Individual driver authors

> Will it be driver/subsystem/arch/author specific?  Or kernel-wide?

driver specific

> This feels like it could easily get out of hand really quickly.

Likely not.  A question might be how useful all these
old debugging printks are today and if it's reasonable
to just delete them.

> Why not just use tracepoints if you really want to be fine-grained?

Weight and lack of class/group capability
Edward Cree June 9, 2020, 5:42 p.m. UTC | #4
On 09/06/2020 17:58, Joe Perches wrote:
> On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
>> What is wrong with the existing control of dynamic
>> debug messages that you want to add another type of arbitrary grouping
>> to it? 
> There is no existing grouping mechanism.
>
> Many drivers and some subsystems used an internal one
> before dynamic debug.
>
> $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> 501
>
> This is an attempt to unify those homebrew mechanisms.
In network drivers, this is probablyusing the existing groupings
 defined by netif_level() - see NETIF_MSG_DRV and friends.  Note
 that those groups are orthogonal to the level, i.e. they control
 netif_err() etc. as well, not just debug messages.
Certainly in the case of sfc, and I'd imagine for many other net
 drivers too, the 'debug' modparam is setting the default for
 net_dev->msg_enable, which can be changed after probe with
 ethtool.
It doesn't look like the proposed mechanism subsumes that (we have
 rather more than 5 groups, and it's not clear how you'd connect
 it to the existing msg_enable (which uapi must be maintained); if
 you don't have a way to do this, better exclude drivers/net/ from
 your grep|wc because you won't be unifying those - in my tree
 that's 119 hits.

-ed
Greg KH June 10, 2020, 6:31 a.m. UTC | #5
On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > What is wrong with the existing control of dynamic
> > debug messages that you want to add another type of arbitrary grouping
> > to it? 
> 
> There is no existing grouping mechanism.

info/warn/err/dbg is what I am referring to.

> Many drivers and some subsystems used an internal one
> before dynamic debug.
> 
> $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> 501

Yes, and it's horrid and needs to be cleaned up, not added to.

In the beginning, yes, adding loads of different types of debugging
options to a driver is needed by the author, but by the time it is added
to the kernel, all of that should be able to be removed and only a
single "enable debug" should be all that is needed.

We do not need each individual driver thinking it needs to have some
sort of special classification of each type of debug message.  Just use
the framework that we have, you can enable/disable them on a
line-by-line basis as needed.

> This is an attempt to unify those homebrew mechanisms.

All of those should just be removed.

> Stanimir attempted to add one for his driver via a
> driver specific standardized format substring for level.
> 
> > And who defines that grouping?
> 
> Individual driver authors

That way lies madness, let's try to fix all of that up.

greg k-h
Greg KH June 10, 2020, 7:09 a.m. UTC | #6
On Tue, Jun 09, 2020 at 11:35:31PM -0700, Joe Perches wrote:
> On Wed, 2020-06-10 at 08:31 +0200, Greg Kroah-Hartman wrote:

> > On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:

> > > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:

> > > > What is wrong with the existing control of dynamic

> > > > debug messages that you want to add another type of arbitrary grouping

> > > > to it? 

> > > 

> > > There is no existing grouping mechanism.

> > 

> > info/warn/err/dbg is what I am referring to.

> > 

> > > Many drivers and some subsystems used an internal one

> > > before dynamic debug.

> > > 

> > > $ git grep "MODULE_PARM.*\bdebug\b"|wc -l

> > > 501

> > 

> > Yes, and it's horrid and needs to be cleaned up, not added to.

> 

> Or unified so driver authors have a standardized mechanism

> rather than reinventing or doing things differently.


But each "level" you all come up with will be intrepreted differently
per driver, causing total confusion (like we have today.)  Try to make
it better by just removing that mess.

> > In the beginning, yes, adding loads of different types of debugging

> > options to a driver is needed by the author, but by the time it is added

> > to the kernel, all of that should be able to be removed and only a

> > single "enable debug" should be all that is needed.

> 

> No one does that.


We did that for USB drivers a decade ago, it can be done.

greg k-h
Joe Perches June 10, 2020, 7:24 a.m. UTC | #7
On Wed, 2020-06-10 at 09:09 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 11:35:31PM -0700, Joe Perches wrote:

> > On Wed, 2020-06-10 at 08:31 +0200, Greg Kroah-Hartman wrote:

> > > On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:

> > > > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:

> > > > > What is wrong with the existing control of dynamic

> > > > > debug messages that you want to add another type of arbitrary grouping

> > > > > to it? 

> > > > 

> > > > There is no existing grouping mechanism.

> > > 

> > > info/warn/err/dbg is what I am referring to.


This is specifically about dbg so that's not relevant is it.
> But each "level" you all come up with will be intrepreted differently

> per driver, causing total confusion (like we have today.)  Try to make

> it better by just removing that mess.


Or add value as it allows the developer to do what's
necessary for their development.

> > > In the beginning, yes, adding loads of different types of debugging

> > > options to a driver is needed by the author, but by the time it is added

> > > to the kernel, all of that should be able to be removed and only a

> > > single "enable debug" should be all that is needed.

> > 

> > No one does that.

> 

> We did that for USB drivers a decade ago, it can be done.


And nearly no one does it.

btw: look up usbip_debug_flag and usbip_dbg_<foo> or the uhci driver
diff mbox series

Patch

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 0dc2eb8e44e5..c2b751fc8a17 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -208,6 +208,12 @@  line
 	line -1605          // the 1605 lines from line 1 to line 1605
 	line 1600-          // all lines from line 1600 to the end of the file
 
+level
+    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
+    callsite. This will allow to group debug messages and show only those of the
+    same level.  The -p flag takes precedence over the given level. Note that we can
+    have up to five groups of debug messages.
+
 The flags specification comprises a change operation followed
 by one or more flag characters.  The change operation is one
 of the characters::
@@ -346,6 +352,10 @@  Examples
   // add module, function to all enabled messages
   nullarbor:~ # echo -n '+mf' > <debugfs>/dynamic_debug/control
 
+  // enable all messages in file with 0x01 level bitmask
+  nullarbor:~ # echo -n 'file foo.c level 0x01 +p' >
+                                <debugfs>/dynamic_debug/control
+
   // boot-args example, with newlines and comments for readability
   Kernel command line: ...
     // see whats going on in dyndbg=value processing