Re: [PATCH v1] dynamic_debug: add support for logs destination

[jim . cromie]

On Mon, Oct 9, 2023 at 4:47 PM Łukasz Bartosik <lb@xxxxxxxxxxxx> wrote:
>
> pt., 6 paź 2023 o 22:49 <jim.cromie@xxxxxxxxx> napisał(a):
> >
> > On Wed, Oct 4, 2023 at 4:55 AM Łukasz Bartosik <lb@xxxxxxxxxxxx> wrote:
> > >
> > > wt., 3 paź 2023 o 22:54 <jim.cromie@xxxxxxxxx> napisał(a):
> > > >
> > > > On Tue, Oct 3, 2023 at 1:57 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, 2 Oct 2023 14:49:20 -0600
> > > > > jim.cromie@xxxxxxxxx wrote:
> > > > >
> > > > > > hi Lukasz,
> > > > > >
> > > > > > sorry my kernel-time has been in my own trees.
> > > > > >
> > > > > > What I dont understand is why +T is insufficient.
> > > > > >
> > >
> > > We would like to be able to separate debug logs from different
> > > subsystem (e.g. thunderbolt and usbcore).
> > > With +T it is not possible because all debug logs will land in the same bucket.
> > >
> > > > > > IIUC, tracefs is intended for production use.
> > > > > > thats why each event can be enabled / disabled
> > > > > > - to select and minimize whats traced, and not impact the system
> > > > > >
> > > > > > and +T  can forward all pr_debugs to trace,
> > > > > > (by 1-few trace events defined similarly to others)
> > > > > > or very few, giving yet another selection mechanism
> > > > > > to choose or eliminate specific pr-debugs and reduce traffic to
> > > > > > interesting stuff.
> > > > > >
> > > > > > Once your debug is in the trace-buf,
> > > > > > shouldnt user-space be deciding what to do with it ?
> > > > > > a smart daemon could leverage tracefs to good effect.
> > > > > >
> > >
> > > Yes, a daemon could separate the debug logs but IMHO it is much
> > > easier to separate logs by sending them directly from a given subsystem
> > > to a separate trace instance. My proposal allows to configure different
> > > trace instance as destination for each callsite.
> > >
> > > > > > IMO the main value of +T is that it allows feeding existing pr_debugs
> > > > > > into the place where other trace-data is already integrated and managed.
> > > > > >
> > > > > > At this point, I dont see any extra destination handling as prudent.
> > > > > >
> > > > >
> > > > >
> > > > > I'm fine with either approach. I kind of like the creation of the instance,
> > > > > as that allows the user to keep this debug separate from other tracing
> > > > > going on. We are starting to have multiple applications using the tracing
> > > > > buffer (although most are using instances, which is why I'm trying to make
> > > > > them lighter weight with the eventfs code).
> > > > >
> > > > > -- Steve
> > > > >
> > >
> > > Steve, thanks for commenting from the trace perspective.
> > >
> > > >
> > > >
> > > > Ok Im starting to grasp that multiple instances are good
> > > > (and wondering how I didnt notice)
> > > >
> > > > What doesnt thrill me is the new _ddebug field, it enlarges the footprint.
> > > >
> > >
> > > Yes it increases _ddebug structure by a pointer size.
> > >
> > > > can you make it go away ?
> > >
> > > I implemented my proposal with flexibility in mind so that if someone
> > > would like to add
> > > another destination in the future it should be easy to do. I
> > > understand that adding a pointer
> > > to the _ddebug structure increases footprint size that's why I also
> > > added CONFIG_DYNAMIC_DEBUG_DST
> > > kernel configuration option in order to enable/disable this functionality.
> > >
> > > > I have some thoughts ..
> > >
> > > Please share your thoughts. I'm sure we can come to an agreement how
> > > to incorporate both +T and my proposal.
> >
> >
> > So heres what Im thinking:
> >
> > shrink lineno, get 2-3 bits back.
> > last I checked largest C file is <32kloc
> > largest header is ~120kloc, but its a data only,
> > no pr_debugs will suddenly appear there.
> >
> > define a dst_id field with 3 bits
>
> The dst_id field would be taken into account only when a callsite has
> T flag set, is that your assumption ?

Im ambivalent about the +T bit itself,
it could as easily be another "special" value in the 0-2^N range of dst_id

its a use-case tradeoff:
+T  goes to main tracebuf.
dst_id>0  goes to separate, pre-registered "flight-recorder"  tracebufs

I'm not sure whether doing both independently is better than having
2^(n-1)-1 extra bufs.

Actually, demoting +T to just another dest makes sense -
theres a large population of pr-debugs, and events like vblank-*
Nobody will send vblank to syslog on purpose either.


either way, dyndbg will need both:
new trace-events, so that prdbgs can get enabled as a single/few event-types
trace_array_buf writes to flight recorders







> Can one bit be taken from class_id to increase dst_id to 4 bits ?
> Decreasing class_id length to 5 bits would still leave its range at
> [0..31]

31 classes / categories should be enough.
64 was convenient, BIT* supported.

> > 0 is for main tracebuf
> > 1-7 is for other instances
> >
>
> Do you want to leave trace events as originally implemented with +T flag ?
> If yes then I would like to propose:
> dst_id = 0 - for trace events for pr_debug and dbg_dev logs (default)
> dst_id = 1 - for default trace instance
> dst_id > 1 - other trace instances
>
> > then the alt-dest lookup is avoided except when the dst_id field is >0
> >
> > It might work to put the alt-dst-pointer into the classmaps,
> > so the destination is used for the entire group of debugs
> > forex DRM_UT_CORE etc.
> >
>
> If we store dst pointers for dst_id > 1 in classmaps then is there a fast way
> to get from callsite (_ddebug) to its corresponding classmap
> (ddebug_class_map) in order to
> lookup trace instance destination ?  I ask because I don't see it being possible
> without adding a new field to the _ddebug structure.

I agree, theres no good enough way, compared to a small dst-id.
The "muse" was on the grouping aspect it might bring.

> > But its no better than the dst_id field, which is per-callsite,
> > and entirely independent of classes.
> >
this was me agreeing with your point above :-)


>
> I don't have a real life use case to configure different trace
> instance for each callsite.
> I just tried to be as much flexible as possible.
>

Ive come around to agree - I looked back at some old threads
(that I was a part of, and barely remembered :-}

At least Sean Paul, Lyude, Simon Ser, Pekka Paalanen
have expressed a desire for a "flight-recorder"
it'd be hard to say now that 2-3 such buffers would always be enough,
esp as theres a performance reason for having your own.


> Thanks,
> Lukasz
>
> >
> > > Thanks,
> > > Lukasz