Re: [PATCH 2/5] dyndbg: add class_id field and query support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 11, 2022 at 12:06 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>
>
>
> On 3/10/22 23:47, Jim Cromie wrote:
> > DRM defines/uses 10 enum drm_debug_category's to create exclusive
> > classes of debug messages.  To support this directly in dynamic-debug,
> > add the following:
> >
> > - struct _ddebug.class_id:4 - 4 bits is enough
> > - define _DPRINTK_SITE_UNCLASSED 15 - see below
> >
> > and the query support:
> > - struct _ddebug_query.class_id
> > - ddebug_parse_query  - looks for "class" keyword, then calls..
> > - parse_class         - accepts uint: 0-15, sets query.class_id and marker
> > - vpr_info_dq         - displays new field
> > - ddebug_proc_show    - append column with "cls:%d" if !UNCLASSED
> >
> > With the patch, one can enable current/unclassed callsites by:
> >
> >   #> echo drm class 15 +p > /proc/dynamic_debug/control
> >
>
> To me, this is hard to read, what the heck is '15'? I have to go look it
> up in the control file and it's not descriptive. I think that using
> classes/categories makes sense but I'm wondering if it can be a bit more
> user friendly? Perhaps, we can pass an array of strings that is indexed
> by the class id to each pr_debug() site that wants to use class. So
> something like:
>

Im not at all averse to nice names, but as something added on.

1st, the interface to make friendlier is DRM's

echo 0x04 > /sys/module/drm/parameters/debug   # which category ?

parm:           debug:Enable debug output, where each bit enables a
debug category.
Bit 0 (0x01)  will enable CORE messages (drm core code)
Bit 1 (0x02)  will enable DRIVER messages (drm controller code)
Bit 2 (0x04)  will enable KMS messages (modesetting code)

echo DRM_UT_DRIVER,DRM_UT_KMS > /sys/module/drm/parameters/debug   #
now its pretty clear

If that works, its cuz DEFINE_DYNAMIC_DEBUG_CLASSBITS()
plucks class symbols out of its __VA_ARGS__, and #stringifes them.
So that macro could then build the 1-per-module static constant string array
and (only) the callbacks would be able to use it.

from there, it shouldnt be hard to ref that from the module's ddebug_table,
so parse_query could validate class args against the module given (or
fail if none given)

Speaking strictly, theres a gap:
   echo module * class DRM_UT_KMS +p > control
is nonsense for * other than drm + drivers,
but its fair/safe to just disallow wildcards on modname for this purpose.

it does however imply that module foo must exist for FOO_CAT_1 to be usable.
thats not currently the case:
bash-5.1# echo module foo +p > /proc/dynamic_debug/control
[   15.403749] dyndbg: read 14 bytes from userspace
[   15.405413] dyndbg: query 0: "module foo +p" mod:*
[   15.406486] dyndbg: split into words: "module" "foo" "+p"
[   15.407070] dyndbg: op='+'
[   15.407388] dyndbg: flags=0x1
[   15.407809] dyndbg: *flagsp=0x1 *maskp=0xffffffff
[   15.408300] dyndbg: parsed: func="" file="" module="foo" format=""
lineno=0-0 class=15
[   15.409151] dyndbg: no matches for query
[   15.409591] dyndbg: no-match: func="" file="" module="foo"
format="" lineno=0-0 class=15
[   15.410524] dyndbg: processed 1 queries, with 0 matches, 0 errs
bash-5.1#

ISTM we can keep that "0 errs" response for that case, but still reject this:

   echo module foo class FOO_NOT_HERE +p > /proc/dynamic_debug/control


> enum levels {
>         LOW,
>         MEDIUM,
>         HIGH
> };

I want to steer clear of "level" anything,
since 2>1 implies non independence of the categories



>
> static const char * const level_to_strings[] = {
>         [LOW] = "low",
>         [MEDIUM] = "medium",
>         [HIGH] = "high",
> };
>
> And then you'd have a wrapper macros in your driver:
>
> #define module_foo_pr_debug_class(level, fmt, args...)
>         pr_debug_class(level, level_to_strings, fmt, args);
>
> Such that call sites look like:
>
> module_foo_pr_debug_class(LOW, fmt, args...);
>

That macro, minus the "module_foo_" prefix,
could go into dynamic_debug.h

I didnt do that, for 2 reasons:

DRM didnt need it - it had an enum var,
and a set of macros to encapsulate the categories.

-  the "prototype" looks like this might be ok:
    define LOW "low"
    pr_debug_class(LOW, "mumble about something %p %p\n", foo, bar)
ok thats a stretch, but...

Basically, I didnt want to deal with creating a new interface. KIS

> Such that you're not always passing the strings array around. Now, this
> does mean another pointer for struct _ddebug and most wouldn't have it.
> Maybe we could just add another linker section for these so as to save
> space.


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux