Re: [PATCH v2 20/24] dyndbg: WIP towards debug-print-class based callsite controls

[jim . cromie]

On Wed, Jun 17, 2020 at 3:52 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Wed 2020-06-17 10:31:54, Daniel Thompson wrote:
> > On Tue, Jun 16, 2020 at 02:05:27PM -0700, Joe Perches wrote:
> > > On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote:
> > > > On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > > > > There are *lots* of ad-hoc debug printing solutions in kernel,
> > > > > this is a 1st attempt at providing a common mechanism for many of them.
> > > >
> > > > I agree that it might make sense to provide some common mechanism.
> > > []
> > > > My problem with this approach is that it is too generic. Each class
> > > > would have different meaning in each subsystem.
> > > >
> > > > It might help to replace any existing variants. But it would be hard
> > > > for developers debugging the code. They would need to study/remember
> > > > the meaning of these groups for particular subsystems. They would
> > > > need to set different values for different messages.
> > > >
> > > > Could you please provide more details about the potential users?
> > > > Would be possible to find some common patterns and common
> > > > meaning of the groups?
> > >
> > > I doubt the utility of common patterns.
> > > Verbosity is common but groupings are not.
> > >
> > > Look at the DRM patterns vs other groups.
> >
> > I've seen drm.debug mentioned a couple of times but the comments about
> > it seem to only learn part of what is shows us.
> >
> > drm.debug is a form of common grouping but it acts at a sub-system level
> > rather then whole system (and gives a whole sub-system enable/disable).
> > This is where grouping makes most sense.
> >
> > The result is that drm.debug is easy to document, both in official
> > kernel docs and in other resources (like the arch distro documentation).
> > Having controls that are easy to document makes them easy to find and
> > thus sub-system grouping leads directly to higher quality bug reports.
>
> Thanks a lot for explanation.
>
> Now, could anyone please tell me how this new dynamic debug feature
> would allow to replace drm.debug option?
>
> I mean what steps/commands will be needed instead of, for example
> drm.debug=0x3 command line option?
>
> Best Regards,
> Petr


[jimc@frodo linux.git]$ git log -1
commit 12a67ffb3e63c40027e251b44b2abc77463dc2da (HEAD -> dd-v3)
Author: Jim Cromie <jim.cromie@xxxxxxxxx>
Date:   Tue Jun 16 15:36:37 2020 -0600

    dyndbg: export ddebug_exec_queries

    Exporting ddebug_exec_queries will allow module authors using dynamic
    debug to actually control/enable/disable their own pr-debug callsites
    dynamically.

    With it, module authors can tie any update of their internal debug
    variables to a corresponding ddebug_exec_queries invocation, which
    will modify all their selected callsites accordingly.

    Generally, authors would exec +p or -p on some subsets of their set of
    callsites, and leave fmlt flags for user to choose the appropriate
    amount of structural context desired in the logs.

    Depending upon the user needs, the module might be known, and
    therefore a waste of screen width, function would be valuable, file
    would be long and familiar to the author, etc..  That said, author
    could harness the message-prefix facility if they saw fit to do so.
    Any author preferences can be overridden with echo >control

    Is it safe ?

    ddebug_exec_queries() is currently 'exposed' to user space in
    several limited ways;

    1 it is called from module-load callback, where it implements the
      $modname.dyndbg=+p "fake" parameter provided to all modules.

    2 it handles query input from >control directly

    IOW, it is "fully" exposed to local root user; exposing the same
    functionality to other kernel modules is no additional risk.

    The other big issue to check is locking:

    dyndbg has a single mutex, taken by ddebug_change to handle >control,
    and by ddebug_proc_(start|stop) to span `cat control`.  ISTM this
    proposed export presents no locking problems.

    drm use case:

    drm.debug=0x03 appears to be a kernel boot-arg example, setting 2
    internal debug flags.  Each bit is probably controlling a separate
    subset of all debug-prints, they may be overlapping subsets.

    Those subsets are *definitely* expressible as a few dyndbg queries
    each.  Any arbitrary subset is.

       drm.dyndbg='file drm_gem_* +p'       # gem debug
       drm.dyndbg='file *_gem_* +p'         # *gem debug

    With this proposed export, drm authors could exec these examples, most
    likely in the callback that handles updates to the drm.debug variable.
(END)

I should note that none of this needs the WIP patch