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

[jim . cromie]

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

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

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