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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D63D685.2010401@dcl.info.waseda.ac.jp>
Date:	Wed, 23 Feb 2011 00:30:13 +0900
From:	Hitoshi Mitake <mitake@....info.waseda.ac.jp>
To:	Hitoshi Mitake <mitake@....info.waseda.ac.jp>
CC:	Frederic Weisbecker <fweisbec@...il.com>,
	linux-kernel@...r.kernel.org, h.mitake@...il.com,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>, Ingo Molnar <mingo@...e.hu>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH] perf lock: clean the options for perf record

On 2011年02月23日 00:28, Hitoshi Mitake wrote:
> From: Hitoshi Mitake<h.mitake@...il.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@...il.com>
> Cc: Peter Zijlstra<a.p.zijlstra@...llo.nl>
> Cc: Paul Mackerras<paulus@...ba.org>
> Cc: Ingo Molnar<mingo@...e.hu>
> Cc: Arnaldo Carvalho de Melo<acme@...stprotocols.net>
> Cc: Steven Rostedt<rostedt@...dmis.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/