| Message ID | 20200609104604.1594-2-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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, 2020-06-09 at 18:42 +0100, Edward Cree wrote: > 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. These are _not_ netif_<level> control flags. Some are though. For instance: $ git grep "MODULE_PARM.*\bdebug\b" drivers/net | head -10 drivers/net/ethernet/3com/3c509.c:MODULE_PARM_DESC(debug, "debug level (0-6)"); drivers/net/ethernet/3com/3c515.c:MODULE_PARM_DESC(debug, "3c515 debug level (0-6)"); drivers/net/ethernet/3com/3c59x.c:MODULE_PARM_DESC(debug, "3c59x debug level (0-6)"); drivers/net/ethernet/adaptec/starfire.c:MODULE_PARM_DESC(debug, "Debug level (0-6)"); drivers/net/ethernet/allwinner/sun4i-emac.c:MODULE_PARM_DESC(debug, "debug message flags"); drivers/net/ethernet/altera/altera_tse_main.c:MODULE_PARM_DESC(debug, "Message Level (-1: default, 0: no output, 16: all)"); drivers/net/ethernet/amazon/ena/ena_netdev.c:MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); drivers/net/ethernet/amd/atarilance.c:MODULE_PARM_DESC(lance_debug, "atarilance debug level (0-3)"); drivers/net/ethernet/amd/lance.c:MODULE_PARM_DESC(lance_debug, "LANCE/PCnet debug level (0-7)"); drivers/net/ethernet/amd/pcnet32.c:MODULE_PARM_DESC(debug, DRV_NAME " debug level"); These are all level/class output controls. > 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. True. > 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. Likely not. I agree it'd be useful to attach the modparam control flag to the dynamic debug use somehow. cheers, Joe
On 09/06/2020 18:56, Joe Perches wrote: > These are _not_ netif_<level> control flags. Some are though. > For instance: > > $ git grep "MODULE_PARM.*\bdebug\b" drivers/net | head -10 > [...] > > These are all level/class output controls. TIL, thanks! I should have looked deeperrather than assuming they were all like ours. Though judging just by that grep output, it also looks like quite a lot of those won't fit into 5 groups either, so some rethink may still be needed... -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 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. > 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.
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
Hi Greg, On 6/9/20 2:16 PM, Greg Kroah-Hartman wrote: > 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". Yes, it is grouping of KERN_DEBUG level messages by importance (my fault, I put incorrect name). What is important is driver author decision. Usually when the driver is huge and has a lot of debug messages it is not practical to enable all of them to chasing a particular bug or issue. You know that debugging (printk) add delays which could hide or rise additional issue(s) which would complicate debug and waste time. For the Venus driver I have defined three groups of KERN_DEBUG - low, medium and high (again the driver author(s) will decide what the importance is depending on his past experience). There is another point where the debugging is made by person who is not familiar with the driver code. In that case he/she cannot enable lines or range of lines because he don't know the details. Here the grouping by importance could help. > > 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 Wed, Jun 10, 2020 at 01:29:20PM +0300, Stanimir Varbanov wrote: > Hi Greg, > > On 6/9/20 2:16 PM, Greg Kroah-Hartman wrote: > > 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". > > Yes, it is grouping of KERN_DEBUG level messages by importance (my > fault, I put incorrect name). What is important is driver author > decision. Usually when the driver is huge and has a lot of debug > messages it is not practical to enable all of them to chasing a > particular bug or issue. You know that debugging (printk) add delays > which could hide or rise additional issue(s) which would complicate > debug and waste time. That is why it is possible to turn on and off debugging messages on a function/line basis already. Why not just use that instead? > For the Venus driver I have defined three groups of KERN_DEBUG - low, > medium and high (again the driver author(s) will decide what the > importance is depending on his past experience). > > There is another point where the debugging is made by person who is not > familiar with the driver code. In that case he/she cannot enable lines > or range of lines because he don't know the details. Here the grouping > by importance could help. And they will really know what "low/medium/high" are? Anyway, that makes a bit more sense, but the documentation could use a lot more in order to describe this type of behavior, and what is expected by both driver authors, and users of the interface. thanks, greg k-h
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(+)