Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[jim . cromie]

On Thu, Jun 18, 2020 at 1:40 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>
>
>
> 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:
> >>

> >
> >> 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);

Exactly

and using format, which always have user relevant info,
and often some severity indication (forex warn info err)
are a workable classification scheme already in use at least informally

So Id expect that this classification can often be done in 1 query.
define the set of callsites in 1 query-string, add +p or -p to it, and
manipulate away.

Amplifying,
this is the only user interface of consequence in dyndbg.
/sys/.../verbose doesnt count

Letting module authors use it is the full-featured way,
everything else is crap (movie reference)
and would require far more maintenance

>
> 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.
>

Indeed.
Id also like to hear from drm folks

./drm/amd/display/include/logger_types.h:#define DC_LOG_SURFACE(...)
pr_debug("[SURFACE]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_HW_LINK_TRAINING(...)
pr_debug("[HW_LINK_TRAINING]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_HW_AUDIO(...)
pr_debug("[HW_AUDIO]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_SCALER(...)
pr_debug("[SCALER]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_BIOS(...)
pr_debug("[BIOS]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_BANDWIDTH_CALCS(...) pr_debug("[BANDWIDTH_CALCS]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_DML(...)
pr_debug("[DML]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_IF_TRACE(...)
pr_debug("[IF_TRACE]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_GAMMA(...)
pr_debug("[GAMMA]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_ALL_GAMMA(...)
pr_debug("[GAMMA]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_ALL_TF_CHANNELS(...) pr_debug("[GAMMA]:"__VA_ARGS__)

those defines suggest that they are already doing this with existing formats
with the export,
they can implement this group control using dyndbg with little effort.
including the tie-in to the __debug var if thats useful

and of course, user can add or subtract from that set ad-hoc.

> Thanks,
>
> -Jason

thanks
jimc