From 02ad0702e54f9b82b697718e7e8662eb3f2266ee Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Sun, 8 Sep 2013 19:19:13 -0700 Subject: perf lock: Remove dead code No need for break statements after goto jumps. Signed-off-by: Davidlohr Bueso Cc: Aswin Chandramouleeswaran Cc: Frederic Weisbecker Cc: Hitoshi Mitake Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1378693159-8747-2-git-send-email-davidlohr@hp.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 6 ------ 1 file changed, 6 deletions(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index ee33ba2f05dd..148f7e28ade7 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -446,7 +446,6 @@ broken: list_del(&seq->list); free(seq); goto end; - break; default: BUG_ON("Unknown state of lock sequence found!\n"); break; @@ -508,8 +507,6 @@ static int report_lock_acquired_event(struct perf_evsel *evsel, list_del(&seq->list); free(seq); goto end; - break; - default: BUG_ON("Unknown state of lock sequence found!\n"); break; @@ -564,7 +561,6 @@ static int report_lock_contended_event(struct perf_evsel *evsel, list_del(&seq->list); free(seq); goto end; - break; default: BUG_ON("Unknown state of lock sequence found!\n"); break; @@ -606,7 +602,6 @@ static int report_lock_release_event(struct perf_evsel *evsel, switch (seq->state) { case SEQ_STATE_UNINITIALIZED: goto end; - break; case SEQ_STATE_ACQUIRED: break; case SEQ_STATE_READ_ACQUIRED: @@ -624,7 +619,6 @@ static int report_lock_release_event(struct perf_evsel *evsel, ls->discard = 1; bad_hist[BROKEN_RELEASE]++; goto free_seq; - break; default: BUG_ON("Unknown state of lock sequence found!\n"); break; -- cgit From b33492ade49a223a666e582d0c63566609e7014b Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Sun, 8 Sep 2013 19:19:14 -0700 Subject: perf lock: Return proper code in report_lock_*_event The report_lock_*_event() functions return -1 when lock_stat_findnew(), thread_stat_findnew() or get_seq() return NULL. These functions only return this value when failing to allocate memory, this return -ENOMEM instead. Signed-off-by: Davidlohr Bueso Cc: Aswin Chandramouleeswaran Cc: Frederic Weisbecker Cc: Hitoshi Mitake Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1378693159-8747-3-git-send-email-davidlohr@hp.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 148f7e28ade7..d3188629cbaf 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -400,17 +400,17 @@ static int report_lock_acquire_event(struct perf_evsel *evsel, ls = lock_stat_findnew(addr, name); if (!ls) - return -1; + return -ENOMEM; if (ls->discard) return 0; ts = thread_stat_findnew(sample->tid); if (!ts) - return -1; + return -ENOMEM; seq = get_seq(ts, addr); if (!seq) - return -1; + return -ENOMEM; switch (seq->state) { case SEQ_STATE_UNINITIALIZED: @@ -472,17 +472,17 @@ static int report_lock_acquired_event(struct perf_evsel *evsel, ls = lock_stat_findnew(addr, name); if (!ls) - return -1; + return -ENOMEM; if (ls->discard) return 0; ts = thread_stat_findnew(sample->tid); if (!ts) - return -1; + return -ENOMEM; seq = get_seq(ts, addr); if (!seq) - return -1; + return -ENOMEM; switch (seq->state) { case SEQ_STATE_UNINITIALIZED: @@ -533,17 +533,17 @@ static int report_lock_contended_event(struct perf_evsel *evsel, ls = lock_stat_findnew(addr, name); if (!ls) - return -1; + return -ENOMEM; if (ls->discard) return 0; ts = thread_stat_findnew(sample->tid); if (!ts) - return -1; + return -ENOMEM; seq = get_seq(ts, addr); if (!seq) - return -1; + return -ENOMEM; switch (seq->state) { case SEQ_STATE_UNINITIALIZED: @@ -587,17 +587,17 @@ static int report_lock_release_event(struct perf_evsel *evsel, ls = lock_stat_findnew(addr, name); if (!ls) - return -1; + return -ENOMEM; if (ls->discard) return 0; ts = thread_stat_findnew(sample->tid); if (!ts) - return -1; + return -ENOMEM; seq = get_seq(ts, addr); if (!seq) - return -1; + return -ENOMEM; switch (seq->state) { case SEQ_STATE_UNINITIALIZED: -- cgit From 0a98c7febf55325ebac4f28289a9433f4b66ed0e Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Sun, 8 Sep 2013 19:19:15 -0700 Subject: perf lock: Plug some memleaks Address some trivial leaks. Signed-off-by: Davidlohr Bueso Cc: Aswin Chandramouleeswaran Cc: Frederic Weisbecker Cc: Hitoshi Mitake Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1378693159-8747-4-git-send-email-davidlohr@hp.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index d3188629cbaf..77843478a27a 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -321,10 +321,12 @@ static struct lock_stat *lock_stat_findnew(void *addr, const char *name) new->addr = addr; new->name = zalloc(sizeof(char) * strlen(name) + 1); - if (!new->name) + if (!new->name) { + free(new); goto alloc_failed; - strcpy(new->name, name); + } + strcpy(new->name, name); new->wait_time_min = ULLONG_MAX; list_add(&new->hash_entry, entry); @@ -875,7 +877,7 @@ static int __cmd_record(int argc, const char **argv) const char *record_args[] = { "record", "-R", "-m", "1024", "-c", "1", }; - unsigned int rec_argc, i, j; + unsigned int rec_argc, i, j, ret; const char **rec_argv; for (i = 0; i < ARRAY_SIZE(lock_tracepoints); i++) { @@ -892,7 +894,7 @@ static int __cmd_record(int argc, const char **argv) rec_argc += 2 * ARRAY_SIZE(lock_tracepoints); rec_argv = calloc(rec_argc + 1, sizeof(char *)); - if (rec_argv == NULL) + if (!rec_argv) return -ENOMEM; for (i = 0; i < ARRAY_SIZE(record_args); i++) @@ -908,7 +910,9 @@ static int __cmd_record(int argc, const char **argv) BUG_ON(i != rec_argc); - return cmd_record(i, rec_argv, NULL); + ret = cmd_record(i, rec_argv, NULL); + free(rec_argv); + return ret; } int cmd_lock(int argc, const char **argv, const char *prefix __maybe_unused) -- cgit From 375eb2be5584b8182a917124ca217b74e43d2dc4 Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Sun, 8 Sep 2013 19:19:16 -0700 Subject: perf lock: Redo __cmd_report This function should be straightforward, and we can remove some trivial logic by moving the functionality of read_events() into __cmd_report() - thus allowing a new session to be properly deleted. Since the 'info' subcommand also needs to process the recorded events, add a 'display_info' flag to differentiate between report and info commands. Furthermore, this patch also calls perf_session__has_traces(), making sure that we don't compare apples and oranges, fixing a segfault when using an perf.data file generated by a different subcommand. ie: ./perf mem record sleep 1 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.017 MB perf.data (~724 samples) ] ./perf lock report Segmentation fault (core dumped) Signed-off-by: Davidlohr Bueso Cc: Aswin Chandramouleeswaran Cc: Frederic Weisbecker Cc: Hitoshi Mitake Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1378693159-8747-5-git-send-email-davidlohr@hp.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 68 +++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 32 deletions(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 77843478a27a..780484fb59a0 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -818,6 +818,18 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused, return 0; } +static void sort_result(void) +{ + unsigned int i; + struct lock_stat *st; + + for (i = 0; i < LOCKHASH_SIZE; i++) { + list_for_each_entry(st, &lockhash_table[i], hash_entry) { + insert_to_result(st, compare); + } + } +} + static const struct perf_evsel_str_handler lock_tracepoints[] = { { "lock:lock_acquire", perf_evsel__process_lock_acquire, }, /* CONFIG_LOCKDEP */ { "lock:lock_acquired", perf_evsel__process_lock_acquired, }, /* CONFIG_LOCKDEP, CONFIG_LOCK_STAT */ @@ -825,51 +837,47 @@ static const struct perf_evsel_str_handler lock_tracepoints[] = { { "lock:lock_release", perf_evsel__process_lock_release, }, /* CONFIG_LOCKDEP */ }; -static int read_events(void) +static int __cmd_report(bool display_info) { + int err = -EINVAL; struct perf_tool eops = { .sample = process_sample_event, .comm = perf_event__process_comm, .ordered_samples = true, }; + session = perf_session__new(input_name, O_RDONLY, 0, false, &eops); if (!session) { pr_err("Initializing perf session failed\n"); - return -1; + return -ENOMEM; } + if (!perf_session__has_traces(session, "lock record")) + goto out_delete; + if (perf_session__set_tracepoints_handlers(session, lock_tracepoints)) { pr_err("Initializing perf session tracepoint handlers failed\n"); - return -1; + goto out_delete; } - return perf_session__process_events(session, &eops); -} + if (select_key()) + goto out_delete; -static void sort_result(void) -{ - unsigned int i; - struct lock_stat *st; - - for (i = 0; i < LOCKHASH_SIZE; i++) { - list_for_each_entry(st, &lockhash_table[i], hash_entry) { - insert_to_result(st, compare); - } - } -} + err = perf_session__process_events(session, &eops); + if (err) + goto out_delete; -static int __cmd_report(void) -{ setup_pager(); + if (display_info) /* used for info subcommand */ + err = dump_info(); + else { + sort_result(); + print_result(); + } - if ((select_key() != 0) || - (read_events() != 0)) - return -1; - - sort_result(); - print_result(); - - return 0; +out_delete: + perf_session__delete(session); + return err; } static int __cmd_record(int argc, const char **argv) @@ -970,7 +978,7 @@ int cmd_lock(int argc, const char **argv, const char *prefix __maybe_unused) if (argc) usage_with_options(report_usage, report_options); } - __cmd_report(); + rc = __cmd_report(false); } else if (!strcmp(argv[0], "script")) { /* Aliased to 'perf script' */ return cmd_script(argc, argv, prefix); @@ -983,11 +991,7 @@ int cmd_lock(int argc, const char **argv, const char *prefix __maybe_unused) } /* recycling report_lock_ops */ trace_handler = &report_lock_ops; - setup_pager(); - if (read_events() != 0) - rc = -1; - else - rc = dump_info(); + rc = __cmd_report(true); } else { usage_with_options(lock_usage, lock_options); } -- cgit From 60a25cbc4a167fc0129296c3c640d8506a57acc5 Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Sun, 8 Sep 2013 19:19:18 -0700 Subject: perf lock: Limit bad rate precision Two decimal precision should be enough for this. Signed-off-by: Davidlohr Bueso Cc: Aswin Chandramouleeswaran Cc: Frederic Weisbecker Cc: Hitoshi Mitake Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1378693159-8747-7-git-send-email-davidlohr@hp.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 780484fb59a0..972310cbeb63 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -686,7 +686,7 @@ static void print_bad_events(int bad, int total) pr_info("\n=== output for debug===\n\n"); pr_info("bad: %d, total: %d\n", bad, total); - pr_info("bad rate: %f %%\n", (double)bad / (double)total * 100); + pr_info("bad rate: %.2f %%\n", (double)bad / (double)total * 100); pr_info("histogram of events caused bad sequence\n"); for (i = 0; i < BROKEN_MAX; i++) pr_info(" %10s: %d\n", name[i], bad_hist[i]); -- cgit From f37376cd721a539ac398cbb7718b72fce83cd49b Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Sun, 8 Sep 2013 19:19:19 -0700 Subject: perf lock: Account for lock average wait time While perf-lock currently reports both the total wait time and the number of contentions, it doesn't explicitly show the average wait time. Having this value immediately in the report can be quite useful when looking into performance issues. Furthermore, allowing report to sort by averages is another handy feature to have - and thus do not only print the value, but add it to the lock_stat structure. Signed-off-by: Davidlohr Bueso Cc: Aswin Chandramouleeswaran Cc: Frederic Weisbecker Cc: Hitoshi Mitake Cc: Ingo Molnar Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1378693159-8747-8-git-send-email-davidlohr@hp.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 972310cbeb63..6a9076f165f4 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -56,7 +56,9 @@ struct lock_stat { unsigned int nr_readlock; unsigned int nr_trylock; + /* these times are in nano sec. */ + u64 avg_wait_time; u64 wait_time_total; u64 wait_time_min; u64 wait_time_max; @@ -208,6 +210,7 @@ static struct thread_stat *thread_stat_findnew_first(u32 tid) SINGLE_KEY(nr_acquired) SINGLE_KEY(nr_contended) +SINGLE_KEY(avg_wait_time) SINGLE_KEY(wait_time_total) SINGLE_KEY(wait_time_max) @@ -244,6 +247,7 @@ static struct rb_root result; /* place to store sorted data */ struct lock_key keys[] = { DEF_KEY_LOCK(acquired, nr_acquired), DEF_KEY_LOCK(contended, nr_contended), + DEF_KEY_LOCK(avg_wait, avg_wait_time), DEF_KEY_LOCK(wait_total, wait_time_total), DEF_KEY_LOCK(wait_min, wait_time_min), DEF_KEY_LOCK(wait_max, wait_time_max), @@ -516,6 +520,7 @@ static int report_lock_acquired_event(struct perf_evsel *evsel, seq->state = SEQ_STATE_ACQUIRED; ls->nr_acquired++; + ls->avg_wait_time = ls->nr_contended ? ls->wait_time_total/ls->nr_contended : 0; seq->prev_event_time = sample->time; end: return 0; @@ -570,6 +575,7 @@ static int report_lock_contended_event(struct perf_evsel *evsel, seq->state = SEQ_STATE_CONTENDED; ls->nr_contended++; + ls->avg_wait_time = ls->wait_time_total/ls->nr_contended; seq->prev_event_time = sample->time; end: return 0; @@ -703,6 +709,7 @@ static void print_result(void) pr_info("%10s ", "acquired"); pr_info("%10s ", "contended"); + pr_info("%15s ", "avg wait (ns)"); pr_info("%15s ", "total wait (ns)"); pr_info("%15s ", "max wait (ns)"); pr_info("%15s ", "min wait (ns)"); @@ -734,6 +741,7 @@ static void print_result(void) pr_info("%10u ", st->nr_acquired); pr_info("%10u ", st->nr_contended); + pr_info("%15" PRIu64 " ", st->avg_wait_time); pr_info("%15" PRIu64 " ", st->wait_time_total); pr_info("%15" PRIu64 " ", st->wait_time_max); pr_info("%15" PRIu64 " ", st->wait_time_min == ULLONG_MAX ? @@ -940,7 +948,7 @@ int cmd_lock(int argc, const char **argv, const char *prefix __maybe_unused) }; const struct option report_options[] = { OPT_STRING('k', "key", &sort_key, "acquired", - "key for sorting (acquired / contended / wait_total / wait_max / wait_min)"), + "key for sorting (acquired / contended / avg_wait / wait_total / wait_max / wait_min)"), /* TODO: type */ OPT_END() }; -- cgit From f5fc14124c5cefdd052a2b2a6a3f0ed531540113 Mon Sep 17 00:00:00 2001 From: Jiri Olsa Date: Tue, 15 Oct 2013 16:27:32 +0200 Subject: perf tools: Add data object to handle perf data file This patch is adding 'struct perf_data_file' object as a placeholder for all attributes regarding perf.data file handling. Changing perf_session__new to take it as an argument. The rest of the functionality will be added later to keep this change simple enough, because all the places using perf_session are changed now. Signed-off-by: Jiri Olsa Acked-by: Namhyung Kim Cc: Adrian Hunter Cc: Andi Kleen Cc: Corey Ashford Cc: David Ahern Cc: Frederic Weisbecker Cc: Ingo Molnar Cc: Namhyung Kim Cc: Paul Mackerras Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1381847254-28809-2-git-send-email-jolsa@redhat.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 6a9076f165f4..33c7253295b9 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -15,6 +15,7 @@ #include "util/debug.h" #include "util/session.h" #include "util/tool.h" +#include "util/data.h" #include #include @@ -853,8 +854,12 @@ static int __cmd_report(bool display_info) .comm = perf_event__process_comm, .ordered_samples = true, }; + struct perf_data_file file = { + .path = input_name, + .mode = PERF_DATA_MODE_READ, + }; - session = perf_session__new(input_name, O_RDONLY, 0, false, &eops); + session = perf_session__new(&file, false, &eops); if (!session) { pr_err("Initializing perf session failed\n"); return -ENOMEM; -- cgit From b9c5143a012a543c4ee872498d6dbae5c10beb2e Mon Sep 17 00:00:00 2001 From: Frederic Weisbecker Date: Wed, 11 Sep 2013 14:46:56 +0200 Subject: perf tools: Use an accessor to read thread comm As the thread comm is going to be implemented by way of a more complicated data structure than just a pointer to a string from the thread struct, convert the readers of comm to use an accessor instead of accessing it directly. The accessor will be later overriden to support an enhanced comm implementation. Signed-off-by: Frederic Weisbecker Tested-by: Jiri Olsa Cc: Jiri Olsa Cc: David Ahern Cc: Ingo Molnar Cc: Peter Zijlstra Cc: Arnaldo Carvalho de Melo Cc: Stephane Eranian Link: http://lkml.kernel.org/n/tip-wr683zwy94hmj4ibogmnv9ce@git.kernel.org [ Rename thread__comm_curr() to thread__comm_str() ] Signed-off-by: Namhyung Kim [ Fixed up some minor const pointer issues ] Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 33c7253295b9..35f9aaa565cc 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -767,7 +767,7 @@ static void dump_threads(void) while (node) { st = container_of(node, struct thread_stat, rb); t = perf_session__findnew(session, st->tid); - pr_info("%10d: %s\n", st->tid, t->comm); + pr_info("%10d: %s\n", st->tid, thread__comm_str(t)); node = rb_next(node); }; } -- cgit From 744a971940520cf0818e1fe882b64892c528e6de Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo Date: Wed, 6 Nov 2013 10:17:38 -0300 Subject: perf evsel: Ditch evsel->handler.data field Not needed since this cset: fcf65bf149af: perf evsel: Cache associated event_format So lets trim this struct a bit. Cc: Adrian Hunter Cc: David Ahern Cc: Frederic Weisbecker Cc: Jiri Olsa Cc: Mike Galbraith Cc: Paul Mackerras Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lkml.kernel.org/n/tip-j8setslokt0goiwxq9dogzqm@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-lock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tools/perf/builtin-lock.c') diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index 35f9aaa565cc..c852c7a85d32 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -819,8 +819,8 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused, return -1; } - if (evsel->handler.func != NULL) { - tracepoint_handler f = evsel->handler.func; + if (evsel->handler != NULL) { + tracepoint_handler f = evsel->handler; return f(evsel, sample); } -- cgit