Message-ID: <CAJfuBxxw6DbmGG-UdiZeUr9680tJVEVcw-czPL+jt-88YUMgbw@mail.gmail.com>
Date:   Mon, 13 Nov 2023 18:08:18 -0700
From:   jim.cromie@...il.com
To:     Łukasz Bartosik <lb@...ihalf.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Jason Baron <jbaron@...mai.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Guenter Roeck <groeck@...gle.com>,
        Yaniv Tzoreff <yanivt@...gle.com>,
        Benson Leung <bleung@...gle.com>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>,
        Pekka Paalanen <ppaalanen@...il.com>,
        Sean Paul <seanpaul@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>, linux-kernel@...r.kernel.org,
        upstream@...ihalf.com
Subject: Re: [PATCH v1 11/12] dyndbg: write debug logs to trace instance

On Mon, Nov 13, 2023 at 4:44 PM Łukasz Bartosik <lb@...ihalf.com> wrote:
>
> pon., 13 lis 2023 o 19:59 <jim.cromie@...il.com> napisał(a):
> >
> > On Sun, Nov 12, 2023 at 9:32 AM Łukasz Bartosik <lb@...ihalf.com> wrote:
> > >
> > > pt., 10 lis 2023 o 21:03 <jim.cromie@...il.com> napisał(a):
> > > >
> > > > On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@...ihalf.com> wrote:
> > > > >
> > > > > sob., 4 lis 2023 o 22:49 <jim.cromie@...il.com> napisał(a):
> > > > > >
> > > > > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@...ihalf.com> wrote:
> > > > > > >
> > > > > > > When trace is enabled (T flag is set) and trace_dst field is set
> > > > > > > to value greater than 0 (0 is reserved for trace events) then
> > > > > > > debug logs will be written to trace instance pointed by trace_dst
> > > > > > > value, for example when trace_dst value is 2 then debug logs will
> > > > > > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > > > > > Given trace instance will not be initialized until debug logs are
> > > > > > > requested to be written to it and afer init will persist until
> > > > > > > reboot.
> > > > > > >
> > > > > >
> > > > > > restating 00 comments -
> > > > > >
> > > > > > you can get rid of integer destination ids by adding a new command: open/close
> > > > > >
> > > > > > $> echo  \
> > > > > >  open kms-instance \;\
> > > > > >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > > > > > /proc/dynamic_debug/control
> > > > > >
> > > > >
> > > > > Instead of using above command to preset destination we could preset
> > > > > destination with open command. I mean last successful
> > > > > open would preset destination ? What do you think ?
> > > > >
> > > >
> > > > I dont think it works - if open maps to a dest-number, (or implicit as
> > > > TOP-of-stack)
> > > > then you just have +T<dest-number>  (or +T <implicit tos>)
> > > > rather than +T:dest-name
> > > > and you still have to keep track of what dest-numbers were already used.
> > > > (or every new dest needs an explicit OPEN before it)
> > > >
> > > > and how do you then get back to default instance ?
> > > > open 0 ?
> > > > close <previous-handle> ?
> > > >
> > > >
> > > > by using names, all opens can be at the top,
> > > > (and thus document in 1 block all the named-instances)
> > > > and any named dest that hasnt been opened is an error
> > > > (not just reusing previous OPEN)
> > > >
> > >
> > > Sorry, I should have been more specific with my proposal. Let me use
> > > an example to clarify it:
> > > open usb    # -> create trace instance "usb" and make it default
> > > echo module usbcore +T > /proc/dynamic_debug/control ## --> write usbcore
> > > ## debug logs to trace instance named usb
> > > open tbt --> create trace instance "tbt" and make it default
> > > echo module aaa +T:usb > /proc/dynamic_debug/control --> write aaa
> > > debug logs to trace instance named usb, instance usb has to be used
> > > explicitly
> > >
> > >                          because now tbt is default trace instance
> >
> > that feels too magical/ action at a distance.
> >
> > ISTM it also muddles what the "default" is:
> >
> > my-default:
> > what each callsite's current/preset dest is/was
> > the only way to set it is with explicit [-+]T:outstream
> >
>
> I see your point, I will follow your suggestion.
>
> > your-default:
> > whatever was last opened. whether it was 2 or 50 lines above,
> > or set weeks ago, the last time somebody opened an instance.
> >
> > and as more instances are created
> > (potentially by different users?
> > after all there are separate instances,
> > and presumably separate interests),
> > the default gets less predictable.
> >
> >
> > > echo module bbb +T > /proc/dynamic_debug/control --> write bbb debug
> > > logs to trace instance named tbt
> > > close tbt --> close tbt trace instance, I omit this step but in order
> > > for an instance to be successful closed it must not be used by any
> > > callsite, after
> > >                     closing tbt instance the usb becomes default instance
> >
> > so after 'close tbt',  the previous 'open usb' is now top-of-stack ?
> >
> > how does that affect all existing callsite-users of tbt ?
> > do they continue to use the trace-instance theyve been writing to ?
> > If not, then reverting to the global instance seems much more predictable.
> >
> > Or are you proposing that the close fails because the instance is still in use ?
> > this seems least surprising,
> > and more robust in the face of the next 'open foo'
> > which could otherwize reuse the dst_id mapped to tbt
> >
> >
> > >
> > > I agree that your method of setting default trace instance is more flexible:
> > > class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites
> > >
> > > Maybe we can combine both to set default trace destination ?
> > >
> > > 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 ?

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

Powered by blists - more mailing lists