Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 22 Jul 2020 07:54:14 +0800
From:   tingwei@codeaurora.org
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Ingo Molnar <mingo@redhat.com>, tsoni@codeaurora.org,
        Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
        Mao Jinlong <jinlmao@codeaurora.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] tracing: add trace_export support for event trace
In-Reply-To: <20200721124342.422ee784@oasis.local.home>
References: <20200720022117.9375-1-tingwei@codeaurora.org>
 <20200720022117.9375-3-tingwei@codeaurora.org>
 <20200721123733.1a87568a@oasis.local.home>
 <20200721124342.422ee784@oasis.local.home>
Message-ID: <78e5b0827f8dfea7d315cea09bd6ab17@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-07-22 00:43, Steven Rostedt wrote:
> On Tue, 21 Jul 2020 12:37:33 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> On Mon, 20 Jul 2020 10:21:15 +0800
>> Tingwei Zhang <tingwei@codeaurora.org> wrote:
>> 
>> > Only function traces can be exported to other destinations currently.
>> > This patch exports event trace as well.
>> >
>> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> > ---
>> >  kernel/trace/trace.c | 24 +++++++++++++-----------
>> >  1 file changed, 13 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> > index bb62269724d5..aef6330836e2 100644
>> > --- a/kernel/trace/trace.c
>> > +++ b/kernel/trace/trace.c
>> > @@ -2697,17 +2697,6 @@ int tracepoint_printk_sysctl(struct ctl_table
> *table, int write,
>> >  	return ret;
>> >  }
>> >
>> > -void trace_event_buffer_commit(struct trace_event_buffer *fbuffer)
>> > -{
>> > -	if (static_key_false(&tracepoint_printk_key.key))
>> > -		output_printk(fbuffer);
>> > -
>> > -	event_trigger_unlock_commit_regs(fbuffer->trace_file,
> fbuffer->buffer,
>> > -				    fbuffer->event, fbuffer->entry,
>> > -				    fbuffer->flags, fbuffer->pc,
> fbuffer->regs);
>> > -}
>> > -EXPORT_SYMBOL_GPL(trace_event_buffer_commit);
>> > -
>> 
>> Please move the ftrace_exports routines up, instead of moving the
>> trace_event_buffer_commit() down. As it fits better where it is (next
>> to the other buffer_commit code).
>> 
>> -- Steve
>> 
>> 
>> >  /*
>> >   * Skip 3:
>> >   *
>> > @@ -2868,6 +2857,19 @@ int unregister_ftrace_export(struct
>> > trace_export *export) }
>> >  EXPORT_SYMBOL_GPL(unregister_ftrace_export);
>> >
>> > +void trace_event_buffer_commit(struct trace_event_buffer *fbuffer)
>> > +{
>> > +	if (static_key_false(&tracepoint_printk_key.key))
>> > +		output_printk(fbuffer);
>> > +
>> > +	if (static_branch_unlikely(&ftrace_exports_enabled))
>> > +		ftrace_exports(fbuffer->event);
> 
> I would also recommend making each static branch a separate variable.
> That way you could pick and choose what you want to enable. If you only
> want events, and functions are being traced, it will add a bit of
> overhead to handle the functions to just ignore them.
> 
> That is:
> 
>  ftrace_exports_enabled, trace_event_exports_enabled,
>  trace_marker_exports_enabled.
> 
> -- Steve
> 
That's a good idea.  Let me add that to next revision.

> 
>> > +	event_trigger_unlock_commit_regs(fbuffer->trace_file,
>> > fbuffer->buffer,
>> > +				    fbuffer->event, fbuffer->entry,
>> > +				    fbuffer->flags, fbuffer->pc,
>> > fbuffer->regs); +}
>> > +EXPORT_SYMBOL_GPL(trace_event_buffer_commit);
>> > +
>> >  void
>> >  trace_function(struct trace_array *tr,
>> >  	       unsigned long ip, unsigned long parent_ip, unsigned
>> > long flags,
>>