Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

On 19 August 2016 at 00:12, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Thu, 18 Aug 2016 17:22:11 +0800
> Chunyan Zhang <zhang.chunyan@xxxxxxxxxx> 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