Message-ID: <CAK8ByeJH8vKUxeM21ME0vYjKmC_Z=P8XsKiB42k95iZ09bJXJg@mail.gmail.com>
Date:   Tue, 14 Nov 2023 00:44:26 +0100
From:   Łukasz Bartosik <lb@...ihalf.com>
To:     jim.cromie@...il.com
Cc:     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>,
        Steven Rostedt <rostedt@...dmis.org>,
        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

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 ?

> >
> >
> > >
> > > > >
> > > > > 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