<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 25, 2022 at 9:02 AM Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, May 16, 2022 at 04:56:13PM -0600, Jim Cromie wrote:<br>
> DRM.debug API is 23 macros, issuing 10 exclusive categories of debug<br>
> messages. By rough count, they are used 5140 times in the kernel.<br>
> These all call drm_dbg or drm_devdbg, which call drm_debug_enabled(),<br>
> which checks bits in global __drm_debug. Some of these are page-flips<br>
> and vblanks, and get called often.<br>
> <br>
> DYNAMIC_DEBUG (with CONFIG_JUMP_LABEL) is built to avoid this kind of<br>
> work, with NOOPd jump/callsites.<br>
> <br>
> This patchset is RFC because:<br>
> - it touches 2.5 subsystems: dyndbg, drm, tracefs (new events)<br>
> - dyndbg class support is built for drm, needs it for validation<br>
> - new api, used by drm<br>
> - big memory impact, with 5100 new pr-debug callsites.<br>
> - drm class bikeshedding opportunities<br>
> - others, names etc.<br>
<br>
Thanks a lot for keeping on pushing this!</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> DYNAMIC_DEBUG:<br>
></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> RFC:<br>
> <br>
> dynamic_debug_register_classes() cannot act early enough to be in<br>
> effect at module-load. So this will not work as you'd reasonably<br>
> expect:<br>
> <br>
> modprobe test_dynamic_debug dyndbg='+pfm; class FOO +pfmlt'<br>
> <br>
> The 1st query:+pfm will be enabled during load, but in the 2nd query,<br>
> "class FOO" will be unknown at load time. Early class enablement<br>
> would be nice. DYNAMIC_DEBUG_CLASSES is a static initializer, which<br>
> is certainly early enough, but Im missing a trick, suggestions?<br>
<br>
So maybe I'm just totally overloading this work here so feel free to<br>
ignore or postpone, but: Could we do the dynamic_debug_register_classes()<br>
automatically at module load as a new special section? And then throw in a<br>
bit of kbuild so that in a given subsystem every driver gets the same<br>
class names by default and everything would just work, without having to<br>
sprinkle calls to dynamic_debug_register_classes() all over the place?<br></blockquote><div><br></div><div>This is now done; Ive added __dyndbg_classes section.</div><div>load_module() now grabs it from the .ko</div><div>and ddebug_add_module() attaches it to the module's ddebug_table record.</div><div>for builtins, dynamic_debug_init feeds the builtin class-maps to ddebug_add_module</div><div><br></div><div>bash-5.1# modprobe test_dynamic_debug dyndbg="class FOO +p"<br>[ 88.374722] dyndbg: class[0]: nm:test_dynamic_debug base:20 len:7 ty:1<br>[ 88.375158] dyndbg: 0: EMERG<br>[ 88.375345] dyndbg: 1: DANGER<br>[ 88.375540] dyndbg: 2: ERROR<br>[ 88.375726] dyndbg: 3: WARNING<br>[ 88.375930] dyndbg: 4: NOTICE<br>[ 88.376130] dyndbg: 5: INFO<br>[ 88.376310] dyndbg: 6: DEBUG<br>[ 88.376499] dyndbg: class[1]: nm:test_dynamic_debug base:12 len:3 ty:1<br>[ 88.376903] dyndbg: 0: ONE<br>[ 88.377079] dyndbg: 1: TWO<br>[ 88.377253] dyndbg: 2: THREE<br>[ 88.377441] dyndbg: class[2]: nm:test_dynamic_debug base:8 len:3 ty:0<br>[ 88.377837] dyndbg: 0: bing<br>[ 88.378022] dyndbg: 1: bong<br>[ 88.378203] dyndbg: 2: boom<br>[ 88.378387] dyndbg: class[3]: nm:test_dynamic_debug base:4 len:3 ty:0<br>[ 88.378800] dyndbg: 0: Foo<br>[ 88.378986] dyndbg: 1: Bar<br>[ 88.379167] dyndbg: 2: Buzz<br>[ 88.379348] dyndbg: class[4]: nm:test_dynamic_debug base:0 len:3 ty:0<br>[ 88.379757] dyndbg: 0: FOO<br>[ 88.379938] dyndbg: 1: BAR<br>[ 88.380136] dyndbg: 2: BUZZ<br>[ 88.380410] dyndbg: module:test_dynamic_debug attached 5 classes<br>[ 88.380881] dyndbg: 24 debug prints in module test_dynamic_debug<br>[ 88.381315] dyndbg: module: test_dynamic_debug dyndbg="class FOO +p"<br>[ 88.381714] dyndbg: query 0: "class FOO +p" mod:test_dynamic_debug<br>[ 88.382109] dyndbg: split into words: "class" "FOO" "+p"<br>[ 88.382445] dyndbg: op='+'<br>[ 88.382616] dyndbg: flags=0x1<br>[ 88.382802] dyndbg: *flagsp=0x1 *maskp=0xffffffff<br>[ 88.383101] dyndbg: parsed: func="" file="" module="test_dynamic_debug" format="" lineno=0-0 class=FOO<br>[ 88.383740] dyndbg: applied: func="" file="" module="test_dynamic_debug" format="" lineno=0-0 class=FOO<br>[ 88.384324] dyndbg: processed 1 queries, with 2 matches, 0 errs<br>bash-5.1# <br></div><div><br></div><div>so its working at module-load time.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
For the entire class approach, did you spot another subsystem that could<br>
benefit from this and maybe make a more solid case that this is something<br>
good?<br></blockquote><div><br></div><div>I had been working on the premise that ~4k drm*dbg callsites was a good case.</div><div><br></div><div>verbosity-levels - with x<y logic instead of x==y is what's currently missing.</div><div><br></div><div>the next revision adds something, which "kinda works".</div><div>But I think I'll rip it out, and do this simpler approach instead:</div><div><br></div><div>implement a verbose/levels param & callback, which takes</div><div><br></div><div> echo 3 > /sys/module/foo/parameters/debug_verbosity</div><div><br></div><div>and effectively does</div><div><br></div><div> echo <<EOQRY > /proc/dynamic_debug/control</div><div>module foo class V1 +p</div><div><div>module foo class V2 +p</div><div><div>module foo class V3 +p</div><div><div>module foo class V4 -p</div><div><div>module foo class V5 -p</div><div><div>module foo class V6 -p</div><div><div>module foo class V7 -p</div><div><div>module foo class V8 -p</div><div>EOQRY</div></div></div></div></div></div></div></div><div><br></div><div>since only real +/-p changes incur kernel-patching costs,</div><div>the remaining overheads are minimal.</div><div><br></div><div> <br>
> RFC for DRM:<br>
> <br>
> - decoration flags "fmlt" do not work on drm_*dbg().<br>
> (drm_*dbg() dont use pr_debug, they *become* one flavor of them)<br>
> this could (should?) be added, and maybe tailored for drm.<br>
> some of the device prefixes are very long, a "d" flag could optionalize them.<br>
<br>
I'm lost what the fmlt decoration flags are?<br>
<br></div><br>The flags are::<br><br> p enables the pr_debug() callsite.<br> f Include the function name in the printed message<br> l Include line number in the printed message<br> m Include module name in the printed message<br> t Include thread ID in messages not generated from interrupt context<br> _ No flags are set. (Or'd with others on input)<br><div><br></div><div><br></div><div>the fmlt flags add a "decoration" prefix to enabled/printed log messages</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - api use needs review wrt drm life-cycle.<br>
> enum drm_debug_category and DYNAMIC_DEBUG_CLASSES could be together?<br>
<br>
Hm if they're tied to module lifetime we should be good? Not sure what<br>
could go wrong here.<br>
<br></blockquote><div><br></div><div>with the new __section, "life-cycle" doesnt really pertain.</div><div>the new issue is how the class-maps are shared across the subsystem;</div><div>the current class-maps list for each module will probably break;</div><div>a list item cannot be on N different lists of different modules.</div><div>Altering the list-items to ref the class-map (not contain it) should solve the problem.</div><div><br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - class-names could stand review, perhaps extension<br>
> "drm:core:" etc have appeared (maybe just from me)<br>
> or a "plan" to look at it later<br>
<br>
Yeah it's been a bit sprawling. I'm kinda hoping that by firmly<br>
establishing dyndbg as the drm debug approach we can cut down for the need<br>
for ad-hoc flags a bit.<br>
<br></blockquote><div>yeah thats why I kept the DRM_UT_* names.</div><div>OTOH - the symbolic names patch exposes the choices,</div><div>which locks the names as API ??</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - i915 & amdgpu have pr_debugs (DC_LOG_*, gvt_dbg_*) that have<br>
> class-ish prefixes that are separate from, but similar to DRM_UT_*,<br>
> and could stand review and possible unification with reformed or<br>
> extended drm categories.<br>
<br>
Yeah drm is not entirely consistent with how exactly driver debug printing<br>
should be done. Another reason why I'm hoping that the kitchen sync with<br>
everything approach you're doing here could help unify things.<br></blockquote><div><br></div><div><br></div><div>the decoration flags can help here; they loosely/precisely describe</div><div>the elements of most/all the current debug format-prefix variations.</div><div>So case by case, the ad-hoc variations should map onto these flags,</div><div><br></div><div>The flags allow selectively dropping the prefix info from some of the log entries,</div><div>after you've seen the module name and function a dozen times, </div><div>it's helpful to reduce screen clutter.</div><div><br></div><div>It might make sense to add a new flag for device,</div><div>so that dev_dbg() flavors can shorten-or-skip the longer device strings, </div><div>maybe some drm specific flavors.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> - the change to enum drm_debug_category from bitmask values to 0..31<br>
> means that we foreclose this possiblility:<br>
> <br>
> drm_dbg(DRM_UT_CORE|DRM_UT_KMS, "wierd double-cat experiment");<br>
<br>
Yeah no, that doesn't make much sense to me :-)<br>
<br></blockquote><div>no chuckles for the schrodinger's cat joke ?</div><div>(maybe "yeah no" is the artful superpositional reply, I just caught :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - nouveau has very few drm.debug calls,<br>
> has NV_DEBUG, VMM_DEBUG, nvkm_printk_, I havent looked deeply.<br>
<br></blockquote><div><br></div><div>nouveau has like levels, man ..</div><div>test_dynamic_debug implements its priority-style names as a POC</div><div><br></div><div>patch 18 converts nvkm_debug/trace to use dev_dbg instead of dev_info<br></div><div>it probably could adapt to use drm_devdbg</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Yeah see above. There's a pile more drivers (more on the armsoc side of<br>
things) which are quite big on the raw debug call approach.<br>
<br></blockquote><div><br></div><div><div>LOW, MID, HI has been proposed at least once wrt dyndbg.</div><div>that probably fits well with current disjoint classes.</div><div>level/verbose classes should be practical too, as described above.</div><div><br></div><div>NB: The symbolic names should also work </div><div><br></div><div> echo +MID > /sys/module/foobar/parameters/debug_verbosity</div><div> </div></div><div>though theres some ambiguity with</div><div><br></div><div><div> echo -V3 > /sys/module/foobar/parameters/debug_verbosity</div><br class="gmail-Apple-interchange-newline"></div><div>that should turn off V4,5,6, </div><div>but what about V1,2 ?</div><div>it could leave them alone (whatever their previous settings are)</div><div><br></div><div>anyway, lkp-robot and igt-trybot should be grinding on the latest patchset soon,</div><div>I'll send it after I fix whatever breaks.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Cheers, Daniel<br></blockquote><div><br></div><div>thanks, </div><div>Jim </div></div></div>