[PATCH 1/4] perf top: Do not convert address for perf_top__record_precise_ip()

[PATCH 1/4] perf top: Do not convert address for perf_top__record_precise_ip()

[Namhyung Kim]

We call map->unmap_ip() before the function and call map->map_ip()
inside the function.  This is meaningless and look strange since only
one of the two checks 'map'.  Let's use al->addr directly.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 785aa2dd8f0b..3b0978e5578a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -189,8 +189,6 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 	if (pthread_mutex_trylock(&notes->lock))
 		return;
 
-	ip = he->ms.map->map_ip(he->ms.map, ip);
-
 	if (ui__has_annotation())
 		err = hist_entry__inc_addr_samples(he, counter, ip);
 
@@ -687,14 +685,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
 	struct hist_entry *he = iter->he;
 	struct perf_evsel *evsel = iter->evsel;
 
-	if (sort__has_sym && single) {
-		u64 ip = al->addr;
-
-		if (al->map)
-			ip = al->map->unmap_ip(al->map, ip);
-
-		perf_top__record_precise_ip(top, he, evsel->idx, ip);
-	}
+	if (sort__has_sym && single)
+		perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
 
 	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
 		     !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));
-- 
2.6.4

[PATCH 2/4] perf top: Access hists->lock only if needed

[Namhyung Kim]

The perf_top__record_precise_ip() releases and regrabs the
he->hists->lock because it can sleep if there's an error.  But it should
be done conditionally as it slows down the fast path.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 3b0978e5578a..586798acf7db 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -194,21 +194,23 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 
 	pthread_mutex_unlock(&notes->lock);
 
-	/*
-	 * This function is now called with he->hists->lock held.
-	 * Release it before going to sleep.
-	 */
-	pthread_mutex_unlock(&he->hists->lock);
+	if (unlikely(err)) {
+		/*
+		 * This function is now called with he->hists->lock held.
+		 * Release it before going to sleep.
+		 */
+		pthread_mutex_unlock(&he->hists->lock);
+
+		if (err == -ERANGE && !he->ms.map->erange_warned)
+			ui__warn_map_erange(he->ms.map, sym, ip);
+		else if (err == -ENOMEM) {
+			pr_err("Not enough memory for annotating '%s' symbol!\n",
+			       sym->name);
+			sleep(1);
+		}
 
-	if (err == -ERANGE && !he->ms.map->erange_warned)
-		ui__warn_map_erange(he->ms.map, sym, ip);
-	else if (err == -ENOMEM) {
-		pr_err("Not enough memory for annotating '%s' symbol!\n",
-		       sym->name);
-		sleep(1);
+		pthread_mutex_lock(&he->hists->lock);
 	}
-
-	pthread_mutex_lock(&he->hists->lock);
 }
 
 static void perf_top__show_details(struct perf_top *top)
-- 
2.6.4

[tip:perf/core] perf top: Access hists->lock only if needed

[tip-bot for Namhyung Kim]

Commit-ID:  151ee834cc946fa159ee406c62b4d5ce1ebd7115
Gitweb:     http://git.kernel.org/tip/151ee834cc946fa159ee406c62b4d5ce1ebd7115
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 11 Dec 2015 11:56:54 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 11 Dec 2015 08:34:49 -0300

perf top: Access hists->lock only if needed

The perf_top__record_precise_ip() releases and regrabs the
he->hists->lock because it can sleep if there's an error.  But it should
be done conditionally as it slows down the fast path.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449802616-16170-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 3b0978e..586798a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -194,21 +194,23 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 
 	pthread_mutex_unlock(&notes->lock);
 
-	/*
-	 * This function is now called with he->hists->lock held.
-	 * Release it before going to sleep.
-	 */
-	pthread_mutex_unlock(&he->hists->lock);
+	if (unlikely(err)) {
+		/*
+		 * This function is now called with he->hists->lock held.
+		 * Release it before going to sleep.
+		 */
+		pthread_mutex_unlock(&he->hists->lock);
+
+		if (err == -ERANGE && !he->ms.map->erange_warned)
+			ui__warn_map_erange(he->ms.map, sym, ip);
+		else if (err == -ENOMEM) {
+			pr_err("Not enough memory for annotating '%s' symbol!\n",
+			       sym->name);
+			sleep(1);
+		}
 
-	if (err == -ERANGE && !he->ms.map->erange_warned)
-		ui__warn_map_erange(he->ms.map, sym, ip);
-	else if (err == -ENOMEM) {
-		pr_err("Not enough memory for annotating '%s' symbol!\n",
-		       sym->name);
-		sleep(1);
+		pthread_mutex_lock(&he->hists->lock);
 	}
-
-	pthread_mutex_lock(&he->hists->lock);
 }
 
 static void perf_top__show_details(struct perf_top *top)

[PATCH 3/4] perf top: Fix annotation on --stdio

[Namhyung Kim]

The ui__has_annotation() inside perf_top__record_precise_ip() should be
removed since it returns true only for TUI (and when sort key has
symbol).  However the 'perf top --stdio' also supports annotation for a
symbol which was specified by 's' key action.

Actually it already does the necessary checks before calling the
function.  So it's ok to get rid of the check here.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 586798acf7db..f447e5531f8b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -189,8 +189,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 	if (pthread_mutex_trylock(&notes->lock))
 		return;
 
-	if (ui__has_annotation())
-		err = hist_entry__inc_addr_samples(he, counter, ip);
+	err = hist_entry__inc_addr_samples(he, counter, ip);
 
 	pthread_mutex_unlock(&notes->lock);
 
-- 
2.6.4

[tip:perf/core] perf top: Fix annotation on --stdio

[tip-bot for Namhyung Kim]

Commit-ID:  448f13b2d18fdc8dbaada97442e8954dcb4ef8fa
Gitweb:     http://git.kernel.org/tip/448f13b2d18fdc8dbaada97442e8954dcb4ef8fa
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 11 Dec 2015 11:56:55 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 11 Dec 2015 08:36:18 -0300

perf top: Fix annotation on --stdio

The ui__has_annotation() inside perf_top__record_precise_ip() should be
removed since it returns true only for TUI (and when sort key has
symbol).  However the 'perf top --stdio' also supports annotation for a
symbol which was specified by 's' key action.

Actually it already does the necessary checks before calling the
function.  So it's ok to get rid of the check here.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449802616-16170-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 586798a..f447e55 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -189,8 +189,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 	if (pthread_mutex_trylock(&notes->lock))
 		return;
 
-	if (ui__has_annotation())
-		err = hist_entry__inc_addr_samples(he, counter, ip);
+	err = hist_entry__inc_addr_samples(he, counter, ip);
 
 	pthread_mutex_unlock(&notes->lock);
 

[PATCH 4/4] perf top: Cleanup condition in perf_top__record_precise_ip()

[Namhyung Kim]

The 'he' cannot be NULL since it's caller hist_iter__top_callback() is
called only if iter->he is not NULL (see hist_entry_iter__add).  So
setting 'sym' before the condition to simplify the code.

Also make it clearer that the top->symbol_filter_entry check is only
meaningful on stdio mode (i.e. when use_browser is 0).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index f447e5531f8b..e67991eb9304 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -175,15 +175,14 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 					int counter, u64 ip)
 {
 	struct annotation *notes;
-	struct symbol *sym;
+	struct symbol *sym = he->ms.sym;
 	int err = 0;
 
-	if (he == NULL || he->ms.sym == NULL ||
-	    ((top->sym_filter_entry == NULL ||
-	      top->sym_filter_entry->ms.sym != he->ms.sym) && use_browser != 1))
+	if (sym == NULL || (use_browser == 0 &&
+			    (top->sym_filter_entry == NULL ||
+			     top->sym_filter_entry->ms.sym != he->ms.sym)))
 		return;
 
-	sym = he->ms.sym;
 	notes = symbol__annotation(sym);
 
 	if (pthread_mutex_trylock(&notes->lock))
-- 
2.6.4

Re: [PATCH 4/4] perf top: Cleanup condition in perf_top__record_precise_ip()

[Arnaldo Carvalho de Melo]

Em Fri, Dec 11, 2015 at 11:56:56AM +0900, Namhyung Kim escreveu:
> The 'he' cannot be NULL since it's caller hist_iter__top_callback() is
> called only if iter->he is not NULL (see hist_entry_iter__add).  So
> setting 'sym' before the condition to simplify the code.
> 
> Also make it clearer that the top->symbol_filter_entry check is only
> meaningful on stdio mode (i.e. when use_browser is 0).
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-top.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index f447e5531f8b..e67991eb9304 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -175,15 +175,14 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>  					int counter, u64 ip)
>  {
>  	struct annotation *notes;
> -	struct symbol *sym;
> +	struct symbol *sym = he->ms.sym;
>  	int err = 0;
>  
> -	if (he == NULL || he->ms.sym == NULL ||
> -	    ((top->sym_filter_entry == NULL ||
> -	      top->sym_filter_entry->ms.sym != he->ms.sym) && use_browser != 1))
> +	if (sym == NULL || (use_browser == 0 &&
> +			    (top->sym_filter_entry == NULL ||
> +			     top->sym_filter_entry->ms.sym != he->ms.sym)))

Since you're simplifying this... you missed replacing he->ms.sym with
sym in this last line, doing that.

>  		return;
>  
> -	sym = he->ms.sym;
>  	notes = symbol__annotation(sym);
>  
>  	if (pthread_mutex_trylock(&notes->lock))
> -- 
> 2.6.4

[tip:perf/core] perf top: Cleanup condition in perf_top__record_precise_ip()

[tip-bot for Namhyung Kim]

Commit-ID:  beefb8d0e556aaf3cb69168c5953e023ace6aa78
Gitweb:     http://git.kernel.org/tip/beefb8d0e556aaf3cb69168c5953e023ace6aa78
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 11 Dec 2015 11:56:56 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 11 Dec 2015 08:38:10 -0300

perf top: Cleanup condition in perf_top__record_precise_ip()

The 'he' cannot be NULL since it's caller hist_iter__top_callback() is
called only if iter->he is not NULL (see hist_entry_iter__add).  So
setting 'sym' before the condition to simplify the code.

Also make it clearer that the top->symbol_filter_entry check is only
meaningful on stdio mode (i.e. when use_browser is 0).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449802616-16170-4-git-send-email-namhyung@kernel.org
[ Complete the simplification replacing one more he->ms.sym with sym ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index f447e55..92fe963 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -175,15 +175,14 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 					int counter, u64 ip)
 {
 	struct annotation *notes;
-	struct symbol *sym;
+	struct symbol *sym = he->ms.sym;
 	int err = 0;
 
-	if (he == NULL || he->ms.sym == NULL ||
-	    ((top->sym_filter_entry == NULL ||
-	      top->sym_filter_entry->ms.sym != he->ms.sym) && use_browser != 1))
+	if (sym == NULL || (use_browser == 0 &&
+			    (top->sym_filter_entry == NULL ||
+			     top->sym_filter_entry->ms.sym != sym)))
 		return;
 
-	sym = he->ms.sym;
 	notes = symbol__annotation(sym);
 
 	if (pthread_mutex_trylock(&notes->lock))

Re: [PATCH 1/4] perf top: Do not convert address for perf_top__record_precise_ip()

[Arnaldo Carvalho de Melo]

Em Fri, Dec 11, 2015 at 11:56:53AM +0900, Namhyung Kim escreveu:
> We call map->unmap_ip() before the function and call map->map_ip()
> inside the function.  This is meaningless and look strange since only
> one of the two checks 'map'.  Let's use al->addr directly.

Thanks for breaking that patch down, this helps with reviewing and
bisecting,

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-top.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 785aa2dd8f0b..3b0978e5578a 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -189,8 +189,6 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>  	if (pthread_mutex_trylock(&notes->lock))
>  		return;
>  
> -	ip = he->ms.map->map_ip(he->ms.map, ip);
> -
>  	if (ui__has_annotation())
>  		err = hist_entry__inc_addr_samples(he, counter, ip);
>  
> @@ -687,14 +685,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
>  	struct hist_entry *he = iter->he;
>  	struct perf_evsel *evsel = iter->evsel;
>  
> -	if (sort__has_sym && single) {
> -		u64 ip = al->addr;
> -
> -		if (al->map)
> -			ip = al->map->unmap_ip(al->map, ip);
> -
> -		perf_top__record_precise_ip(top, he, evsel->idx, ip);
> -	}
> +	if (sort__has_sym && single)
> +		perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
>  
>  	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
>  		     !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));
> -- 
> 2.6.4

[tip:perf/core] perf top: Do not convert address for perf_top__record_precise_ip()

[tip-bot for Namhyung Kim]

Commit-ID:  64226bcf64629996948dc03c38594f00511bfc2b
Gitweb:     http://git.kernel.org/tip/64226bcf64629996948dc03c38594f00511bfc2b
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 11 Dec 2015 11:56:53 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 11 Dec 2015 08:33:18 -0300

perf top: Do not convert address for perf_top__record_precise_ip()

We call map->unmap_ip() before the function and call map->map_ip()
inside the function.  This is meaningless and look strange since only
one of the two checks 'map'.  Let's use al->addr directly.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449802616-16170-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 785aa2d..3b0978e 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -189,8 +189,6 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 	if (pthread_mutex_trylock(&notes->lock))
 		return;
 
-	ip = he->ms.map->map_ip(he->ms.map, ip);
-
 	if (ui__has_annotation())
 		err = hist_entry__inc_addr_samples(he, counter, ip);
 
@@ -687,14 +685,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
 	struct hist_entry *he = iter->he;
 	struct perf_evsel *evsel = iter->evsel;
 
-	if (sort__has_sym && single) {
-		u64 ip = al->addr;
-
-		if (al->map)
-			ip = al->map->unmap_ip(al->map, ip);
-
-		perf_top__record_precise_ip(top, he, evsel->idx, ip);
-	}
+	if (sort__has_sym && single)
+		perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
 
 	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
 		     !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));