| Message ID | 20200609104604.1594-1-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Venus dynamic debug | expand |
On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote: > Here is the third version of dynamic debug improvements in Venus > driver. As has been suggested on previous version by Joe [1] I've > made the relevant changes in dynamic debug core to handle leveling > as more generic way and not open-code/workaround it in the driver. > > About changes: > - added change in the dynamic_debug and in documentation > - added respective pr_debug_level and dev_dbg_level Honestly, this seems like you want to use tracepoints, not dynamic debug.
On 6/9/20 4:13 AM, Matthew Wilcox wrote: > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote: >> Here is the third version of dynamic debug improvements in Venus >> driver. As has been suggested on previous version by Joe [1] I've >> made the relevant changes in dynamic debug core to handle leveling >> as more generic way and not open-code/workaround it in the driver. >> >> About changes: >> - added change in the dynamic_debug and in documentation >> - added respective pr_debug_level and dev_dbg_level > > Honestly, this seems like you want to use tracepoints, not dynamic debug. > Also see this patch series: https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/ [PATCH 00/16] dynamic_debug: cleanups, 2 features It adds/expands dynamic debug flags quite a bit.
On Tue, 2020-06-09 at 13:45 +0300, Stanimir Varbanov wrote: > Hello, > > Here is the third version of dynamic debug improvements in Venus > driver. As has been suggested on previous version by Joe [1] I've > made the relevant changes in dynamic debug core to handle leveling > as more generic way and not open-code/workaround it in the driver. > > About changes: > - added change in the dynamic_debug and in documentation > - added respective pr_debug_level and dev_dbg_level > > regards, > Stan > > [1] https://lkml.org/lkml/2020/5/21/668 This capability is already clumsily used by many drivers that use a module level "debug" flag. $ git grep -P "MODULE_PARM.*\bdebug\b"|wc -l 501 That's a _lot_ of homebrewed mechanisms. Making dynamic debug have this as a feature would help consolidate and standardize the capability. ftrace is definitely useful, but not quite as lightweight and doesn't have the typical uses.
(adding Jim Cromie and comments) On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote: > On 6/9/20 4:13 AM, Matthew Wilcox wrote: > > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote: > > > Here is the third version of dynamic debug improvements in Venus > > > driver. As has been suggested on previous version by Joe [1] I've > > > made the relevant changes in dynamic debug core to handle leveling > > > as more generic way and not open-code/workaround it in the driver. > > > > > > About changes: > > > - added change in the dynamic_debug and in documentation > > > - added respective pr_debug_level and dev_dbg_level > > > > Honestly, this seems like you want to use tracepoints, not dynamic debug. Tracepoints are a bit heavy and do not have any class or grouping mechanism. debug_class is likely a better name than debug_level > Also see this patch series: > https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/ > [PATCH 00/16] dynamic_debug: cleanups, 2 features > > It adds/expands dynamic debug flags quite a bit. Yes, and thanks Randy and Jim and Stanimir I haven't gone through Jim's proposal enough yet. It's unfortunate these patches series conflict. And for Jim, a link to Stanimir's patch series: https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/
On Tue, Jun 9, 2020 at 10:49 AM Joe Perches <joe@perches.com> wrote: > > (adding Jim Cromie and comments) > thanks for bringing me in... > On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote: > > On 6/9/20 4:13 AM, Matthew Wilcox wrote: > > > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote: > > > > Here is the third version of dynamic debug improvements in Venus > > > > driver. As has been suggested on previous version by Joe [1] I've > > > > made the relevant changes in dynamic debug core to handle leveling > > > > as more generic way and not open-code/workaround it in the driver. > > > > > > > > About changes: > > > > - added change in the dynamic_debug and in documentation > > > > - added respective pr_debug_level and dev_dbg_level > > > > > > Honestly, this seems like you want to use tracepoints, not dynamic debug. > > Tracepoints are a bit heavy and do not have any class > or grouping mechanism. > > debug_class is likely a better name than debug_level > > > Also see this patch series: > > https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/ > > [PATCH 00/16] dynamic_debug: cleanups, 2 features > > > > It adds/expands dynamic debug flags quite a bit. > > Yes, and thanks Randy and Jim and Stanimir > > I haven't gone through Jim's proposal enough yet. > It's unfortunate these patches series conflict. > > And for Jim, a link to Stanimir's patch series: > https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/ > > As Joe noted, there is a lot of ad-hockery to possibly clean up, but I dont grok how these levels should be distinguished from KERN_(WARN|INFO|DEBUG) constants. Those constants are used by coders, partly to convey how bad things are As a user, Id be reluctant to disable an EMERG callsite. are you trying to add a User Bit ? or maybe 7-9 of them ? I have a patchset which adds a 'u' flag, for user. An earlier version had x,y,z flags for 3 different user purposes. I simplified, since only 1 was needed to mark up arbitrary sets of callsites. Another patchset feature lets u select on that flag. #> echo u+p > control Joe suggested class, I certainly find level confusing. Is what you want user-flags u[1-7], or driver-flags d]1-7] ? and let me distinguish, your flags are set in code, not modifiable by user, only for filtering on flag/bit state ? so theyd be different than [pfmltu_] flags, which are user changed. my patchset also adds filtering on flag-state, so that "echo u+p > control " could work. if you had echo 'module venus 1+p; 2+p; 9+p' > control how far would you get ? if it covers your needs, then we could add numerical flags (aka U1, U9) can be distinguished from [pfmltu_PFMLTU] and excluded from the mod-flags changes from there, it shouldnt be hard to add some macro help DECLARE_DYNDBG_FLAG ( 1, 'x' ) DECLARE_DYNDBG_FLAG ( 2, 'y' ) DECLARE_DYNDBG_FLAG ( 3, 'z' ) DECLARE_DYNDBG_FLAG_INFO ( 4, 'q', "unquiet a programmer defined debug callsite set" ) also, since Im here, and this is pretty much on-topic, I perused https://lkml.org/lkml/2020/5/21/399 I see 3 things; - s / dev_dbg / VDBGL / - add a bunch of VDBGM calls - sys_get_prop_image_version signature change. (this doesnt really belong here) ISTM most of the selection of dyndbg callsites in that part of venus could be selected by format. echo "module venus format error +p" > control if so, refining your messages will define the logical sets for you ? thanks JimC (one of them anyway)
On Tue, 2020-06-09 at 15:21 -0600, jim.cromie@gmail.com wrote: > On Tue, Jun 9, 2020 at 10:49 AM Joe Perches <joe@perches.com> wrote: > > (adding Jim Cromie and comments) > > On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote: > > > On 6/9/20 4:13 AM, Matthew Wilcox wrote: > > > > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote: > > > > > Here is the third version of dynamic debug improvements in Venus > > > > > driver. As has been suggested on previous version by Joe [1] I've > > > > > made the relevant changes in dynamic debug core to handle leveling > > > > > as more generic way and not open-code/workaround it in the driver. > > > > > > > > > > About changes: > > > > > - added change in the dynamic_debug and in documentation > > > > > - added respective pr_debug_level and dev_dbg_level > > > > > > > > Honestly, this seems like you want to use tracepoints, not dynamic debug. > > > > Tracepoints are a bit heavy and do not have any class > > or grouping mechanism. > > > > debug_class is likely a better name than debug_level > > > > > Also see this patch series: > > > https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/ > > > [PATCH 00/16] dynamic_debug: cleanups, 2 features > > > > > > It adds/expands dynamic debug flags quite a bit. > > > > Yes, and thanks Randy and Jim and Stanimir > > > > I haven't gone through Jim's proposal enough yet. > > It's unfortunate these patches series conflict. > > > > And for Jim, a link to Stanimir's patch series: > > https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/ > > > > > > As Joe noted, there is a lot of ad-hockery to possibly clean up, > but I dont grok how these levels should be distinguished from > KERN_(WARN|INFO|DEBUG) constants. These are not KERN_<LEVEL> at all, all are emitted at KERN_DEBUG These are just driver developer mechanisms to enable/disable groups of formats via some test for < level or | bitmap > Those constants are used by coders, partly to convey how bad things are > As a user, Id be reluctant to disable an EMERG callsite. Not possible. > are you trying to add a User Bit ? or maybe 7-9 of them ? Or maybe a u32/ulong worth as most as I think the largest current use is 32 bits of bitmask. > I have a patchset which adds a 'u' flag, for user. Adapting that with an external bool for bitmask or class would work fine. if (is_bitmask) enable/disable(value|flag) else enable/disable(value < flag) So the equivalent of logical sets would work just fine.
On Tue, 2020-06-09 at 15:23 -0700, Joe Perches wrote: > These are just driver developer mechanisms to enable/disable > groups of formats via some test for < level or | bitmap duh: & bitmask > if (is_bitmask) > enable/disable(value|flag) obviously enable/disable(value & flag)
On Tue, Jun 9, 2020 at 4:23 PM Joe Perches <joe@perches.com> wrote: > > On Tue, 2020-06-09 at 15:21 -0600, jim.cromie@gmail.com wrote: > > > > As Joe noted, there is a lot of ad-hockery to possibly clean up, > > but I dont grok how these levels should be distinguished from > > KERN_(WARN|INFO|DEBUG) constants. > > These are not KERN_<LEVEL> at all, all are emitted at KERN_DEBUG yes indeed. but they are chosen by programmer, fixed by compiler. not dynamic. <pmladek@suse.com> also noted the conceptual adjacency (ambiguity), and referenced KERN_<lvl> If we need this extra query-term, lets call it mbits / mflags / module_flags / module_bits it needs to be module specific, so also requiring "module foo" search term in the query. ( "modflags" is no good, cuz "mod" also means "modified" - just mflags is better ) Already, we have function, file, module, all of which convey semantic structure of the code, and they also match wildcards, so " function foo_*_* " is an effective grouping. Id think this would cover most cases. Finally, all "module venus +p " callsites could be explicitly specified individually in universe=`grep venus control | wc -l` lines, likely a small set. Using the semantic structure exposed by `grep venus control`, it would likely be far less.