On 6/18/20 3:11 PM, jim.cromie@xxxxxxxxx wrote: > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@xxxxxxxxxx> wrote: >> >> >> >> On 6/18/20 1:40 PM, Petr Mladek wrote: >>> On Thu 2020-06-18 18:19:12, Petr Mladek wrote: >>>> On Wed 2020-06-17 10:25:35, Jim Cromie wrote: >>>>> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no >>>>> effect on callsite behavior; it allows incremental marking of >>>>> arbitrary sets of callsites. >>>>> >>>>> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc. >>>>> And in ddebug_read_flags(): >>>>> current code does: [pfmltu_] -> flags >>>>> copy it to: [PFMLTU_] -> mask >>>>> >>>>> also disallow both of a pair: ie no 'pP', no true & false. >>>>> >>>>> 3. Add filtering ops into ddebug_change(), right after all the >>>>> callsite-property selections are complete. These filter on the >>>>> callsite's current flagstate before applying modflags. >>>>> >>>>> Why ? >>>>> >>>>> The u-flag & filter flags >>>>> >>>>> The 'u' flag lets the user assemble an arbitary set of callsites. >>>>> Then using filter flags, user can activate the 'u' callsite set. >>>>> >>>>> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat >>>>> #> echo 'u+p' > control >>>>> >>>>> Of course, you can continue to just activate your set without ever >>>>> marking it 1st, but you could trivially add the markup as you go, then >>>>> be able to use it as a constraint later, to undo or modify your set. >>>>> >>>>> #> echo 'file foo.c +up' >control >>>>> .. monitor, debug, finish .. >>>>> #> echo 'u-p' >control >>>>> >>>>> # then later resume >>>>> #> echo 'u+p' >control >>>>> >>>>> # disable some cluttering messages, and remove from u-set >>>>> #> echo 'file noisy.c function:jabber_* u-pu' >control >>>>> >>>>> # for doc, recollection >>>>> grep =pu control > my-favorite-callsites >>>>> >>>>> Note: >>>>> >>>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm >>>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can >>>>> enable them early, and $module.dyndbg=+p bootargs will arm them when >>>>> the module is loaded. But you could manage them with u-flags: >>>>> >>>>> #> echo '-t' >control # clear t-flag to use it as 2ndary markup >>>>> #> echo 'p+ut' >control # mark the boot-enabled set of callsites >>>>> #> echo '-p' >control # clean your dmesg -w stream >>>>> >>>>> ... monitor, debug .. >>>>> #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs >>>>> #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set >>>> >>>> Does anyone requested this feature, please? >>>> >>>> For me, it is really hard to imagine people using these complex and hacky >>>> steps. >>> >>> I think that all this is motivated by adding support for module >>> specific groups. >>> >>> What about storing the group as yet another information for each >>> message? I mean the same way as we store module name, file, line, >>> function name. >>> >>> Then we could add API to define group for a given message: >>> >>> pr_debug_group(group_id, fmt, ...); >>> >>> the interface for the control file might be via new keyword "group". >>> You could then do something like: >>> >>> echo module=drm group=0x3 +p >control >>> >>> But more importantly you should add functions that might be called >>> when the drm.debug parameter is changes. I have already mentioned >>> it is another reply: >>> >>> dd_enable_module_group(module_name, group_id); >>> dd_disable_module_group(module_name, group_id); >>> >>> >>> It will _not_ need any new flag or flag filtering. >>> >>> Best Regards, >>> Petr >>> >> >> Yes, I'm wondering as well if people are really going to use the >> new flags and filter flags - I mentioned that here: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_6_12_732&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=vltk6sSzPDQIqO4gGkJeDY6jcEarG4xTztab2EHtPFY&s=6x1EHNoRxebA99Tu-C2i0s5dmdzyEF9bXVcv_cYoM_I&e= >> > > yes, I saw, and replied there. > but since that was v1, and we're on v3, we should refresh. > > the central use-case is above, 1-liner version summarized here: > > 1- enable sites as you chase a problem with +up > 2- examine them with grep =pu > 3- change the set to suit, either by adding or subtracting callsites. > 4- continue debugging, and changing callsites to suit > 5- grep =pu control > ~/debugging-session-task1-callsites > 6- echo up-p >control # disable for now, leave u-set for later > 7- do other stuff > 8 echo uP+p >control # reactivate useful debug-state and resume > > >> The grouping stuff is already being used by lots of modules so >> that seems useful. > > I now dont see the need. > > given N debug callsites, any group can be defined by <N queries, > probably a lot less > if module authors can use ddebug_exec_queries(), cuz its exported, (15/21) > then they can act (+p or -p) on those sets defined by <N queries. > > and now any callsite can be in any number of groups, not just one. > It would be prudent to evaluate such groupings case by case, > because the intersecting callsites are subject to "last manipulator wins" > but its unnecessary to insist that all sets are disjoint. > Unlike pr_debug_n, however its spelled. > hmm - so I think you are saying there is then no need to change the calling functions themselves - its still 'pr_debug()'. You could even use the 'format' qualifier for example to implement your groups that way. For example: pr_debug("failure type1: blah"); pr_debug("failure type2: blah blah"); and then do: ddebug_exec_queries("format type1 +p", module); I would be curious to see what Stanimir thinks of this proposal and whether it would work for his venus driver, which is what prompted this module group discussion. Thanks, -Jason
