Message-ID: <CAK8ByeL4YSCBPPGSeNAPAKtR2=BEZxBjpNE3KeX8TaMmJrficQ@mail.gmail.com>
Date:   Tue, 14 Nov 2023 08:44:49 +0100
From:   Łukasz Bartosik <lb@...ihalf.com>
To:     jim.cromie@...il.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

wt., 14 lis 2023 o 02:08 <jim.cromie@...il.com> napisał(a):
>
> 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 ?
>

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.

Or did I miss answer to that in our long discussion :> ?

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

Powered by blists - more mailing lists