| Message ID | 20200609104604.1594-1-stanimir.varbanov@linaro.org |
|---|---|
| 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 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: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.