Messages in this thread | | | From | Łukasz Bartosik <> | Date | Wed, 11 Oct 2023 15:47:50 +0200 | Subject | Re: [PATCH v1] dynamic_debug: add support for logs destination |
| |
wt., 10 paź 2023 o 18:02 <jim.cromie@gmail.com> napisał(a): > > On Mon, Oct 9, 2023 at 4:47 PM Łukasz Bartosik <lb@semihalf.com> wrote: > > > > pt., 6 paź 2023 o 22:49 <jim.cromie@gmail.com> napisał(a): > > > > > > On Wed, Oct 4, 2023 at 4:55 AM Łukasz Bartosik <lb@semihalf.com> wrote: > > > > > > > > wt., 3 paź 2023 o 22:54 <jim.cromie@gmail.com> napisał(a): > > > > > > > > > > On Tue, Oct 3, 2023 at 1:57 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > > > > > > On Mon, 2 Oct 2023 14:49:20 -0600 > > > > > > jim.cromie@gmail.com 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. >
Even though having +T seems more consistent I would prefer to combine it with dst_id because it will give us wider range.
> 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 >
Then depending on the value of dst_id (assuming dst_id consumed +T and one more bit from class_id) we would have: dst_id = 0 - trace disabled dst_id = 1 - trace events for pr_debug and dev_dbg dst_id = [2..31] - predefined trace instances
The trace instance destinations will be global but this number seems pretty reasonable and is sufficient from our use case perspective.
> > > > > > > > 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 see :)
> > > > > 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. >
What would you recommend to base continuation of development of this feature on, I mean whether to use one of your private branches you mentioned (in one of the previous replies) or latest upstream ?
> > > Thanks, > > Lukasz > > > > > > > > > Thanks, > > > > Lukasz
| |