Re: [PATCH 0/2 v3] tracing: Tracing event profiling updates

[Ingo Molnar <mingo@elte.hu>]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> 
> Ingo,
> 
> Hopefully this is my last attempt.
> This new iteration fixes the syscalls events to correctly handle
> the buffer. In the previous version, they did not care about interrupts.
> 
> I only resend the second patch as only this one has changed since the v2.
> 
> The new branch is in:
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	tracing/core-v3
> 
> Thanks,
> 	Frederic.
> 
> Frederic Weisbecker (2):
>       tracing: Factorize the events profile accounting
>       tracing: Allocate the ftrace event profile buffer dynamically
> 
>  include/linux/ftrace_event.h       |   10 +++-
>  include/linux/syscalls.h           |   24 +++-----
>  include/trace/ftrace.h             |  111 ++++++++++++++++++++---------------
>  kernel/trace/trace_event_profile.c |   79 +++++++++++++++++++++++++-
>  kernel/trace/trace_syscalls.c      |   97 +++++++++++++++++++++++++------
>  5 files changed, 234 insertions(+), 87 deletions(-)

Hm, the naming is quite confusing here i think:

  -132,8 +133,12 @@ struct ftrace_event_call {
         atomic_t                profile_count;
         int                     (*profile_enable)(void);
         void                    (*profile_disable)(void);
 +       char                    *profile_buf;
 +       char                    *profile_buf_nmi;

These are generic events, not just 'profiling' histograms.

Generic events can have _many_ output modi:

 - SVGs                   (perf timeline)
 - histograms             (perf report)
 - traces                 (perf trace)
 - summaries / maximums   (perf sched lat)
 - maps                   (perf sched map)
 - graphs                 (perf report --call-graph)

So it's quite a misnomer to talk just about profiling here. This is an 
event record buffer.

Also, what is the currently maximum possible size of ->profile_buf? The 
max size of an event record? The new codepath looks a bit heavy with 
rcu-lock/unlock and other bits put inbetween - and this is now in the 
event sending critical path. Cannot we do a permanent buffer that needs 
no extra locking/reference protection?

Is the whole thing even justified? I mean, we keep the size of records 
low anyway. It's a _lot_ easier to handle on-stack records, they are the 
ideal (and very fast) dynamic allocator which is NMI and IRQ safe, etc.

	Ingo