Received-SPF: pass (google.com: domain of linux-kernel+bounces-185369-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240516174357.26755-1-jim.cromie@gmail.com> <CALwA+NZxiapXe-BBY6CvpqT-Z1h_YyMQr=JBwybY55TWtDNiBA@mail.gmail.com>
In-Reply-To: <CALwA+NZxiapXe-BBY6CvpqT-Z1h_YyMQr=JBwybY55TWtDNiBA@mail.gmail.com>
From: jim.cromie@gmail.com
Date: Tue, 21 May 2024 13:10:38 -0600
Message-ID: <CAJfuBxw3oDbdAwporJw0DXGRbpLqVXxGaBQmOra4vMGDzH94jA@mail.gmail.com>
Subject: Re: [PATCH v8-RESEND 00/33] Fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y regression
To: =?UTF-8?Q?=C5=81ukasz_Bartosik?= <ukaszb@chromium.org>
Cc: jbaron@akamai.com, gregkh@linuxfoundation.org, 
	linux-kernel@vger.kernel.org, linux@rasmusvillemoes.dk, joe@perches.com, 
	mcgrof@kernel.org, daniel.vetter@ffwll.ch, tvrtko.ursulin@linux.intel.com, 
	jani.nikula@intel.com, ville.syrjala@linux.intel.com, seanpaul@chromium.org, 
	robdclark@gmail.com, groeck@google.com, yanivt@google.com, bleung@google.com, 
	Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 21, 2024 at 5:41=E2=80=AFAM =C5=81ukasz Bartosik <ukaszb@chromi=
um.org> wrote:
>
> On Thu, May 16, 2024 at 7:44=E2=80=AFPM Jim Cromie <jim.cromie@gmail.com>=
 wrote:
> >
> > hi Greg, Jason,
> >
> > This patchset fixes the CONFIG_DRM_USE_DYNAMIC_DEBUG=3Dy 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@gmail=
com/
> >  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=3D0x1ff 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=3D<> 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 =3D=3D=3D categories, with name change to avoid muddling
the 2 sides of the drm <=3D> 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 add=
ed.
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 ?

thanks
~jimc