Re: [PATCH] perf lock: clean the options for perf record

[Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>]

On 2011年02月23日 00:28, Hitoshi Mitake wrote:
> From: Hitoshi Mitake<h.mitake@gmail.com>
>
> Hi Frederic, really sorry for my slow work...
>
> This patch cleans the options passed for perf record(or cmd_record()).
> 1. remove ":r" at the tail of the name of events, because it is not supported now
> 2. remove "-f" deprecated option, because it is already default
>
> Signed-off-by: Hitoshi Mitake<h.mitake@gmail.com>
> Cc: Peter Zijlstra<a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras<paulus@samba.org>
> Cc: Ingo Molnar<mingo@elte.hu>
> Cc: Arnaldo Carvalho de Melo<acme@ghostprotocols.net>
> Cc: Steven Rostedt<rostedt@goodmis.org>
> ---
>   tools/perf/builtin-lock.c |    9 ++++-----
>   1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index e00d938..2359f52 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -926,13 +926,12 @@ static const struct option lock_options[] = {
>   static const char *record_args[] = {
>   	"record",
>   	"-R",
> -	"-f",
>   	"-m", "1024",
>   	"-c", "1",
> -	"-e", "lock:lock_acquire:r",
> -	"-e", "lock:lock_acquired:r",
> -	"-e", "lock:lock_contended:r",
> -	"-e", "lock:lock_release:r",
> +	"-e", "lock:lock_acquire",
> +	"-e", "lock:lock_acquired",
> +	"-e", "lock:lock_contended",
> +	"-e", "lock:lock_release",
>   };
>
>   static int __cmd_record(int argc, const char **argv)


BTW, I have a proposal of changing the way of tracing lock event.

Currently, lockdep functions generate lock events (e.g. lock_acquire
and trace_lock_acquire). I don't think that lockdep is an optimal
place for lock event tracing, because:

1. Some subsystems (e.g. workqueue) uses lockdep for their own
validation. It is confusing for modeling locking sequence. I don't
think that the events from lockdep is needless, this might be useful
for trouble shooting or other purposes. But it is not suitable for 
performance analyzing oriented tracing.

2. Currently, every type of lock (spinlock, rwlock, mutex, rwsem)
employs common event tracing. This is not suitable from the perspective 
of the overhead of tracing. Even if a user only looks at the one type
of locks, tracing produces the overhead of the entire. This is harmful 
because lock event is high frequency. <type>_acquire (e.g.
spin_acquire) might be suitable place for putting tracing.

So making new classes of lock event per types is suitable. Tracing
the one of them or combination of them will be better from the 
perspectives of both of modeling and performance.

How do you think about it?

# Shamefully, I still cannot show the concrete example of performance
# improvement by perf lock. It is another big problem... :(

Thanks,
	Hitoshi