Re: [PATCH v8-RESEND 00/33] Fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y regression

[Łukasz Bartosik]

On Tue, May 21, 2024 at 9:11 PM <jim.cromie@xxxxxxxxx> wrote:
>
> On Tue, May 21, 2024 at 5:41 AM Łukasz Bartosik <ukaszb@xxxxxxxxxxxx> wrote:
> >
> > On Thu, May 16, 2024 at 7:44 PM Jim Cromie <jim.cromie@xxxxxxxxx> wrote:
> > >
> > > hi Greg, Jason,
> > >
> > > This patchset fixes the CONFIG_DRM_USE_DYNAMIC_DEBUG=y regression,
> > > Fixes: bb2ff6c27bc9 ("drm: Disable dynamic debug as broken")
> > >
> > > Im calling it v8, to keep consistent with previous labels.
> > > v6 was what got committed, back in 9/2022.
> > > v7 had at least 2 problems that blocked its submission:
> > >
> > >  https://lore.kernel.org/lkml/20231101002609.3533731-1-jim.cromie@xxxxxxxxx/
> > >  https://patchwork.freedesktop.org/series/125066/
> > >
> > > 1. missing __align(8) in METATDATA macro, giving too much placement
> > > freedom to linker, caused weird segvs following non-ptr vals, but for
> > > builtin modules only. found by lkp-test.
> > >
> > > 2. the main patch touched 2 subsystems at once, which would require
> > > special handling.
> > >
> > > What was broken about DYNAMIC_DEBUG ?
> > >
> > > Booting a modular kernel with drm.debug=0x1ff enabled pr_debugs only
> > > in drm itself, not in the yet-to-be loaded driver + helpers.  Once
> > > loaded, the driver's pr_debugs are properly enabled by:
> > >
> > >    echo 0x1ff > /sys/module/drm/parameters/debug  # still worked
> > >
> > > I had tested with scripts doing lots of modprobes with various
> > > permutations of dyndbg=<> option, and I missed that I didn't test
> > > without them.
> > >
> > > The deeper cause was my design error, a violation of the K&R rule:
> > > "define once, refer many times".
> > >
> > > DECLARE_DYNDBG_CLASSMAP defined the classmap, and was used everywhere,
> > > re-declaring the same static classmap repeatedly. Jani Nikula actually
> > > picked up on this (iirc shortly after committed), but the problem
> > > hadn't been seen yet in CI.  One patchset across 2 subsystems didn't
> > > help either.
> > >
> >
> > Could  you summarize classmaps from the beginning ?
> >
>
> IMO, the best thing about dynamic-debug is that it uses
> JUMP_LABELs / static-keys , so it is NOPs except when you want them.
>
> Classmaps 1st purpose is to bring that to DRM,
> which has 10 mutually exclusive categories of debug output,
> all controlled from bits in /sys/modules/drm/parameters/debug.
> w/o dyndbg, __drm_debug is constantly checked during normal ops.
>
> verbose/relative is added for the other kind of debug levels
>
> classes === categories, with name change to avoid muddling
> the 2 sides of the drm <=> dyndbg relation
>
> classmaps to contain the classes, and to not just take the "class" name,
> which is way too generic a term.
>
> Since dyndbg's sole input is the >control file, the "class" keyword was added.
> the PARAM support constructs queries using class foo to effect its changes.
>
>    # change classes by name
>    echo class DRM_UT_CORE +p > /proc/dynamic_debug/control
>    # this cant touch them
>    echo  +p > /proc/dynamic_debug/control
>
> this syntax extension allows dyndbg to honor DRM's privacy expectations
> DRM owns its categories via sysfs knob (only)
> and should not tolerate things changing out from under it.
> (your bank account isnt useful if everyone can withdraw from it)
>
> Also, the class'd pr-dbgs are only manipulable by their classname,
> not by their corresponding class_id : a 64 value space, which is
> shared across all classmaps in a module.
> IOW a module could have 8 separate classmaps, each with 8 classes.
>
> >
> > > So the revised classmap API "splits" it to def & ref:
> > >
> > > DYNDBG_CLASSMAP_DEFINE fixes & updates the busted macro, EXPORTing the
> > > classmap instead.  It gets invoked once per subsystem, by the
> > > parent/builtin, drm.ko for DRM.
> > >
> > > DYNDBG_CLASSMAP_USE in drivers and helpers refer to the classmap by
> > > name, which links the 2 modules, (like a driver's dependency on extern
> > > __drm_debug).
>
> Summarizing,
> author(s) _DEFINE their categories / classes,
> other author(s) _USE that definition.
>
> Both these tell dyndbg that it is allowed to manipulate those classnames
> in the invoking module.
>
> Does that fill the gaps in the explanation / motivation ?
>

Yes it does. Thank you for elaborating on the topic.

> thanks
> ~jimc