| Message ID | 20250320185238.447458-31-jim.cromie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y | expand |
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/ > 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? 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 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. 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. > CC: jbaron@akamai.com > Signed-off-by: Jim Cromie <jim.cromie@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) &&
On Mon, Mar 24, 2025 at 9:20 AM Louis Chauvet <louis.chauvet@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@akamai.com > > Signed-off-by: Jim Cromie <jim.cromie@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 > >
> > - int valid_class; > > + int slctd_class; > > Nitpick: can you use full words? slctd is difficult to read. > yes. done. thx.
On Tue, Mar 25, 2025 at 12:29 PM <jim.cromie@gmail.com> wrote: > > On Mon, Mar 24, 2025 at 9:20 AM Louis Chauvet <louis.chauvet@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. > Heres an improvement: a use of CLASSMAP_PARAM means user wants a sysfs knob. We can reasonably conclude they prefer that mode of control (its what DRM users would expect, since a long time ago). In that case, protect the PARAM settings (from unqualified settings, use of class FOO still works) otherwise no protection. simple to explain, no extra knobs. > > > > 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. > > apologies, since Im sounding kinda argumentative. Jim
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; /* 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) &&
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 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. CC: jbaron@akamai.com Signed-off-by: Jim Cromie <jim.cromie@gmail.com> --- lib/dynamic_debug.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)