[PATCH v2 30/59] dyndbg: drop "protection" of class'd pr_debugs from legacy queries
jim.cromie at gmail.com
jim.cromie at gmail.com
Tue Mar 25 18:29:16 UTC 2025
On Mon, Mar 24, 2025 at 9:20 AM Louis Chauvet <louis.chauvet at bootlin.com> wrote:
>
>
>
> Le 20/03/2025 à 19:52, Jim Cromie a écrit :
> > Current classmap code protects class'd pr_debugs from unintended
> > changes by "legacy" unclassed queries:
> >
> > # this doesn't disable all of DRM_UT_* categories
> > echo "-p" > /proc/dynamic_debug/control
> >
> > # name the class to change it - protective but tedious
> > echo "class DRM_UT_CORE +p" > /proc/dynamic_debug/control
> >
> > # or do it the subsystem way
> > echo 1 > /sys/module/drm/parameters/debug
> >
> > This "name the class to change it" behavior gave a modicum of
> > protection to classmap users (ie DRM) so their debug settings aren't
> > trivially and unintentionally altered underneath them.
> >
> > But this made the class keyword special in some sense; the other
> > keywords skip only on explicit mismatch, otherwize the code falls thru
>
> s/otherwize/otherwise/
ack
>
> > to adjust the pr-debug site.
> >
> > So Jason Baron didn't like this special case when I 1st proposed it;
> > I argued 2 points:
> > - "protection gives stable-debug, improving utility"
> > - __drm_debug is authoritative w/o dyndbg under it.
> >
> > I thought I'd convinced him back then, (and the patchset got merged),
> > but he noted it again when he reviewed this series. So this commit
> > names the "special case": ddebug_client_module_protects_classes(), and
> > reverts it to Jason's preference.
> >
> > If a class mismatch is seen, code distinguishes whether the class was
> > explicitly given (and always skips/continue), or the DFLT was assumed
> > because no class was given. Here we test
> > ddebug_client_module_protects_classes(), skip if so.
> >
> > Later, if any user/module wants to protect its classes, we could add a
> > flag to ddebug_table, a means to set it from CLASSMAP_DEFINE, and
> > check it when applying a classless query/cmd.
>
> I don't really understand the goal of the protection, do you have the
> discussion between you and Jason so I can have some context and some
> answer to my questions?
>
The on-list discussion is here.
https://lore.kernel.org/lkml/2d3846cb-ff9a-3484-61a8-973799727d8f@akamai.com/
https://lore.kernel.org/lkml/0d9f644f-3d60-02c3-7ce0-01296757e181@akamai.com/#t
At the time I thought it was unfinished business, and expected some
more discussion,
but that didnt happen, and later GregKH committed the set.
Last summer I emailed him privately, and he made a 5-6 points Ive
addressed in this rev,
(reduction of repetitive code, enforcing classmap constraints,
protecting against misuse)
but it also became clear he still didnt like the "specialness" of the keyword,
given by the _DFLT constraint applied to legacy callsites and queries.
Since thats a bit of a philosophical debate, I looked for a technical solution,
to have it either way with fairly trivial additions, and to yield
until user experience
dictates otherwise
To be clear, I still think protecting the "classed" is proper.
Without dyndbg, /sys/module/drm/parameters/debug is authoritative, full stop.
I'm surprised any customer would give away that certainty,
it looks like a (small caliber) footgun to me.
But its not worth disagreeing on.
Hence this patch reverts that "protection"
> With the example you gave above, I think this could lead to a very odd
> behavior: if I enable dyndbg, I expect any pr_dbg to be managed by
> dyndbg settings.
if by "any" you mean ALL the ones that currently exist,
before we add a whole new "CLASS" of user,
(with ~5k uses, all comfortable with their exclusive control)
I can agree.
echo class FOO +p > control
gives full control. You just have to say so for the new classes of users.
>
> If a user writes stuff on dyndbg control, he clearly knows what he is
> doing, and he wants to control what logs he wants.
>
> And if you allow multiple "protected" users, the normal way to disable
> all dyndbg logs will be:
>
> ddcmd -p
> ddcmd class DRM_UT_CORE -p
> ddcmd class DRM_... -p # all drm classes
> ddcmd class SPI_... -p # all spi classes
> ddcmd class WHATEVER_... -p # all other subsystem
>
> # And only now you can enable only what you want
> ddcmd module my_mod +p
>
> This is clearly annoying to write.
It is clearly annoying.
It doesnt need to be handy.
thats what /sys/module/drm/parameters/debug is for.
with modest "protection" of explicit naming,
the sysfs knob can reasonably be expected
to reflect whats going on underneath.
Without it, bets are misplaced.
>
> If DRM (or whatever subsystem) wants to add a debug parameter and use it
> to control their logs without being impacted by dyndbg, I believe it
> should not use dyndbg classes to do it.
hmm - dyndbg's 1st value is its NOOP cost when off.
If thats not worth something, you wouldnt bother using it.
In any case, its pretty clear that my viewpoint isnt prevailing here,
and as I said, I dont care enough to disagree.
the reversion here can stand.
>
> > CC: jbaron at akamai.com
> > Signed-off-by: Jim Cromie <jim.cromie at gmail.com>
> > ---
> > lib/dynamic_debug.c | 34 +++++++++++++++++++++++++---------
> > 1 file changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index c44502787c2b..13de0dd3a4ad 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -193,6 +193,17 @@ static int ddebug_find_valid_class(struct ddebug_table const *dt, const char *cl
> > return -ENOENT;
> > }
> >
> > +/*
> > + * classmaps-v1 protected classes from changes by legacy commands
> > + * (those selecting _DPRINTK_CLASS_DFLT by omission), v2 undoes that
> > + * special treatment. State so explicitly. Later we could give
> > + * modules the choice to protect their classes or to keep v2 behavior.
> > + */
> > +static inline bool ddebug_client_module_protects_classes(const struct ddebug_table *dt)
> > +{
> > + return false;
> > +}
> > +
> > /*
> > * Search the tables for _ddebug's which match the given `query' and
> > * apply the `flags' and `mask' to them. Returns number of matching
> > @@ -206,7 +217,7 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
> > unsigned int newflags;
> > unsigned int nfound = 0;
> > struct flagsbuf fbuf, nbuf;
> > - int valid_class;
> > + int slctd_class;
>
> Nitpick: can you use full words? slctd is difficult to read.
>
> >
> > /* search for matching ddebugs */
> > mutex_lock(&ddebug_lock);
> > @@ -218,21 +229,26 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
> > continue;
> >
> > if (query->class_string) {
> > - valid_class = ddebug_find_valid_class(dt, query->class_string);
> > - if (valid_class < 0)
> > + slctd_class = ddebug_find_valid_class(dt, query->class_string);
> > + if (slctd_class < 0)
> > + /* skip/reject classes unknown by module */
> > continue;
> > } else {
> > - /* constrain query, do not touch class'd callsites */
> > - valid_class = _DPRINTK_CLASS_DFLT;
> > + slctd_class = _DPRINTK_CLASS_DFLT;
> > }
> >
> > for (i = 0; i < dt->info.descs.len; i++) {
> > struct _ddebug *dp = &dt->info.descs.start[i];
> >
> > - /* match site against query-class */
> > - if (dp->class_id != valid_class)
> > - continue;
> > -
> > + if (dp->class_id != slctd_class) {
> > + if (query->class_string)
> > + /* site.class != given class */
> > + continue;
> > + /* legacy query, class'd site */
> > + else if (ddebug_client_module_protects_classes(dt))
> > + continue;
> > + /* allow change on class'd pr_debug */
> > + }
> > /* match against the source filename */
> > if (query->filename &&
> > !match_wildcard(query->filename, dp->filename) &&
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>
More information about the Intel-gfx-trybot
mailing list