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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG2=9p9fUKxoPb2t3nVwZYBnU3iGBOEXJ60grQ7bNFmXvArg1g@mail.gmail.com>
Date:   Tue, 23 Aug 2016 16:53:27 +0800
From:   Chunyan Zhang <zhang.chunyan@...aro.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        mingo@...hat.com, Arnd Bergmann <arnd@...db.de>,
        Mike Leach <mike.leach@....com>, Tor Jeremiassen <tor@...com>,
        philippe.langlais@...com, Nicolas GUION <nicolas.guion@...com>,
        felipe.balbi@...ux.intel.com, Lyra Zhang <zhang.lyra@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH V4 1/3] tracing: add a possibility of exporting function
 trace to other places instead of ring buffer only

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