Re: [PATCH v1 11/12] dyndbg: write debug logs to trace instance

[Łukasz Bartosik]

wt., 14 lis 2023 o 16:41 <jim.cromie@xxxxxxxxx> napisał(a):
>
> On Tue, Nov 14, 2023 at 12:45 AM Łukasz Bartosik <lb@xxxxxxxxxxxx> wrote:
> >
> > wt., 14 lis 2023 o 02:08 <jim.cromie@xxxxxxxxx> napisał(a):
> > >
>
> > > > > > Also I think we need a reserved keyword for writing debug logs to
> > > > > > trace events - maybe "event" keyword ?
> > > > >
> > > > > do you mean "event" as a selector, like module, function, class, etc ?
> > > > > if so, what are the values ?
> > > > > any event under  /sys/kernel/debug/tracing/events/ ?
> > > > >
> > > > > how does this get used ?
> > > > >
> > > >
> > > > I meant that we need to reserve name/keyword to enable writing debug
> > > > logs to trace events (prdbg and devdbg), for example
> > > > echo module usbcore +T:event > /proc/dynamic_debug/control
> > > >
> > > > Or do you anticipate other way to do it ?
> > >
> > > way back, when I had even fewer clues,
> > > I sent patches to call trace-printk when +T was set.
> > > Steve didnt like it, I think cuz it could flood the tracebuf.
> > >
> > > Thats why I added the prdbg and devdbg event-types,
> > > so that they could be disabled easily using /sys/kernel/debug/tracing/
> > > putting them squarely under trace-control.
> > >
> > > Note that this puts 2 off-switches in series,
> > > both tracefs and >control can disable all the pr_debug traffic,
> > > tracefs by event-type, and >control at individual callsite level.
> > >
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/enable
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/prdbg/enable
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/devdbg/enable
> > >
> > > I briefly thought about linking the 2 off-switches,
> > > but punted cuz I thought it complicated things,
> > > (how exactly would they get coupled?)
> > > and I didnt want to distract from larger goals
> > >
> > > Does that address your question ?
> > >
> >
> > Jim,
> >
> > Thanks but it doesn't answer my question.
> >
> > How do you plan to enable output to tracefs event at a callsite level ?
> >
> > In my original proposal it was enabled by setting trace destination to
> > 0. Since we are moving to names instead of numbers now I guess we need
> > to reserve a name for it not to clash with trace instance names
> > provided by users. That's why I proposed to reserve name "event" for
> > that purpose and be used as +T:event.
> >
>
> Ok, I got your point now.
>
> how about we call it "0" ?
> it should be an obvious "magical" value,
> cuz it doesnt need to be open'd, and cant be close'd
>
> then we can revert to global tracebuf by its "name"
> echo module foo +T:0 > /proc/dynamic_debug/control
>

I like it. It resembles surprise emoji :0

> we probably should also limit the trace-instance-names to ^\w+
>

Ack

I think this closes the last open topic to discuss for now.
I will get back with next patchset version to you soon.

> > Or did I miss answer to that in our long discussion :> ?
>
> nope :-)
>
> thanks,
> Jim
>
> >
> > Thanks,
> > Lukasz
> >
> > > On a related point, I also added drm_dbg and drm_devdbg.
> > > Those are issued from __drm_dbg & __drm_dev_dbg
> > >  respectively when CONFIG_DRM_USE_DYNAMIC_DEBUG=n.
> > >
> > > Im not sure theyre more useful than confusing yet.
> > >
> > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > > > > > the existing setting)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Sounds good.
> > > > > > > >
> > > > > > >
> > > > > > > :-)