On 19 August 2016 at 00:12, Steven Rostedt <[email protected]> wrote: > On Thu, 18 Aug 2016 17:22:11 +0800 > Chunyan Zhang <[email protected]> wrote: > > >> > Or is this just trying to hook into the tracing that is happening? That >> > is, this isn't replacing writing into the ftrace ring buffer, but it is >> > just adding a way to write to someplace in addition to the ftrace ring >> > buffer. Where you still write to the ftrace ring buffer, but then you >> > can add a hook to copy someplace else as well. >> >> Yes, this is what this patch is trying to implement. >> >> > >> > I was looking at this as a way that you are adding a replacement, not >> > only an addition to. If that's the case, I think there may be a easier >> > way to do this. >> >> I want to know how it would be in the easier way you mentioned here. >> >> I was trying to add a ftrace_ops before, but with that way, I have to >> deal with a lot of trace or ring buffer stuff including the sort of >> discard things like you mentioned, which the existed ftrace code does. >> And if I choose to implement a new ftrace_ops, I'm only able to get >> the function trace support for STM and have to do many things which >> would be overlap with the current ftrace subsystem. > > Adding your own ftrace_ops is a way for replacing, not just adding a > hook into. > >> >> So in order to reuse the existed code and architecture, I chose to add >> a trace_export interface for Ftrace subsytem, and in this way I'm >> using in this patch, I will get all supports of traces which are dealt >> with trace_function(); > > Actually, a trace_export() should only be called if there's been > something added. And that should be done with a static_key_false() > branch (which is dynamically enabled, and does not use a comparison > branch). > > That is, something like this instead: > > if (!call_filter_check_discard(call, entry, buffer, event)) { > if (static_key_false(&ftrace_trace_exports_enabled)) > ftrace_exports(tr, event); > __buffer_unlock_commit(buffer, event); > } >
Thanks for the sample code, I got it, will do like this. > Don't touch the current logic. Just have your code hook into the > ftrace_exports (note I use "ftrace_exports" and not trace_exports() > because it's the function tracer, which has stricter requirements than > events do. If you add a hook for tracepoints later, use trace_exports() > and have a different list for that). > >> >> Another benefit of adding a trace_export is, if there will be other >> subsystem would like to use the processed traces, it only needs to >> register a trace_export and provides a .write() function call back or >> together with a commit function, although from what I can see now >> .write() is enough since my purpose was the processed traces I don't >> need 'ring_buffer_event' so long as I had trace entries. > > I'm saying if you don't mind the ring buffer being used along with > your own code (which seems to be what's happening), then just add a > call back to your code. Don't monkey with the current logic. > > I think that will simplify things tremendously. Thanks for your comments and detailed explanation, Chunyan > > -- Steve
