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

Ingo Molnar
2009-09-19T07:34:13Z

* 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