| Message ID | 20200609104604.1594-2-stanimir.varbanov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Venus dynamic debug | expand |
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
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
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
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
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
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
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 --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
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