All of lore.kernel.org
 help / color / mirror / Atom feed
* The issue with `perf report -s comm`
@ 2017-09-21 14:51 Alexander Pozdneev
  2017-10-04  6:13 ` [RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION Ravi Bangoria
  0 siblings, 1 reply; 8+ messages in thread
From: Alexander Pozdneev @ 2017-09-21 14:51 UTC (permalink / raw)
  To: linux-perf-users

Hello,

I have the following issue with perf `report -s comm` that seems to be
a bug. This is how the code looks like:

int main() {
    double a, b, c;
    a = do_things_main(WAIT_TIME * 2); // 40% of time, a.out
    b = do_things1(WAIT_TIME);         // 20% of time, libdo1.so
    c = do_things2(WAIT_TIME * 2);     // 20% of time, libdo2.so
    return (int)(a + b + c);
}

This is how I run it:

$ LD_LIBRARY_PATH="." perf record -g ./a.out
[ perf record: Woken up 3 times to write data ]
[ perf record: Captured and wrote 0.743 MB perf.data (11911 samples) ]

This is the regular output of `perf report`:

$ perf report | grep -v '#' | head -n 12
    39.99%    39.99%  a.out    a.out              [.] main
            |
            ---main

    39.94%    39.94%  a.out    libdo2.so          [.] do_things2
            |
            ---do_things2

    20.05%    20.05%  a.out    libdo1.so          [.] do_things1
            |
            ---do_things1

It looks exactly as expected. If I sort by `dso`, the output is also OK:

$ perf report -s dso | grep -v '#'
    39.99%    39.99%  a.out
            |
            ---main

    39.94%    39.94%  libdo2.so
            |
            ---do_things2

    20.05%    20.05%  libdo1.so
            |
            ---do_things1

However, If I try to sort by `comm`, the output looks strange:

$ perf report -s comm | grep -v '#'
   100.00%   100.00%  a.out
            |
            |--59.99%--do_things1
            |
             --39.99%--main

Specifically, `do_things2()` is missing in the report.

Here is the full source code of this example:
https://github.com/pozdneev/perf-report-s-comm-bug
Originally, Louis Stuber (IBM Client Center Montpellier) discovered
this behaviour when we have been playing with Flame Graphs
(http://www.brendangregg.com/blog/2016-04-30/linux-perf-folded.html).

This is the list of Linux/perf versions that I've tried:

* Ubuntu 14.04.5 (3.19.0-80-generic), perf version 3.19.8-ckt22
* RHEL 7.3 ppc64le (3.10.0-514.el7.ppc64le), perf version
3.10.0-514.el7.ppc64le.debug
* My colleague confirms that he observes the same behavior with kernel
version 4.13.

Could you please check if the issue remains in the current development
version of Linux perf?

Thanks!

Alexander

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-09-21 14:51 The issue with `perf report -s comm` Alexander Pozdneev
@ 2017-10-04  6:13 ` Ravi Bangoria
  2017-10-04 13:08   ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Ravi Bangoria @ 2017-10-04  6:13 UTC (permalink / raw)
  To: pozdneyev, acme, linux-kernel
  Cc: linux-perf-users, peterz, mingo, alexander.shishkin, yao.jin, ak,
	jolsa, kjlx, milian.wolff, zhangmengting, Ravi Bangoria

Two functions from different binaries can have same start
address. Thus, comparing only start address in match_chain()
leads to inconsistent callchains. Fix this by adding a check
for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/callchain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 510b513..6d5a483 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -678,6 +678,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
+
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -689,12 +692,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		if (cnode->ms.map && node->map) {
+			left_dso = cnode->ms.map->dso;
+			right_dso = node->map->dso;
+		}
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-10-04  6:13 ` [RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION Ravi Bangoria
@ 2017-10-04 13:08   ` Jiri Olsa
  2017-10-05  3:50     ` [PATCH] " Ravi Bangoria
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2017-10-04 13:08 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: pozdneyev, acme, linux-kernel, linux-perf-users, peterz, mingo,
	alexander.shishkin, yao.jin, ak, jolsa, kjlx, milian.wolff,
	zhangmengting

On Wed, Oct 04, 2017 at 11:43:08AM +0530, Ravi Bangoria wrote:
> Two functions from different binaries can have same start
> address. Thus, comparing only start address in match_chain()
> leads to inconsistent callchains. Fix this by adding a check
> for dsos as well.
> 
> Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html
> 
> Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/callchain.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 510b513..6d5a483 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -678,6 +678,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  {
>  	struct symbol *sym = node->sym;
>  	u64 left, right;
> +	struct dso *left_dso = NULL;
> +	struct dso *right_dso = NULL;
> +
>  
>  	if (callchain_param.key == CCKEY_SRCLINE) {
>  		enum match_result match = match_chain_srcline(node, cnode);
> @@ -689,12 +692,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>  		left = cnode->ms.sym->start;
>  		right = sym->start;
> +		if (cnode->ms.map && node->map) {
> +			left_dso = cnode->ms.map->dso;
> +			right_dso = node->map->dso;

makes sense.. but why not to get those maps separately?

	if (cnode->ms.map)
		left_dso = cnode->ms.map->dso;
	if (node->map) {
		right_dso = node->map->dso;

I'd think that if one is missing, it's most likely different
map/dso and you want to fail the == check

jirka

> +		}
>  	} else {
>  		left = cnode->ip;
>  		right = node->ip;
>  	}
>  
> -	if (left == right) {
> +	if (left == right && left_dso == right_dso) {
>  		if (node->branch) {
>  			cnode->branch_count++;
>  
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-10-04 13:08   ` Jiri Olsa
@ 2017-10-05  3:50     ` Ravi Bangoria
  2017-10-05  4:13       ` Namhyung Kim
  0 siblings, 1 reply; 8+ messages in thread
From: Ravi Bangoria @ 2017-10-05  3:50 UTC (permalink / raw)
  To: pozdneyev, acme, linux-kernel, jolsa
  Cc: linux-perf-users, peterz, mingo, alexander.shishkin, yao.jin, ak,
	kjlx, milian.wolff, zhangmengting, Ravi Bangoria

Two functions from different binaries can have same start
address. Thus, comparing only start address in match_chain()
leads to inconsistent callchains. Fix this by adding a check
for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/callchain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index be09d77..6d7f645 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -685,6 +685,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
+
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -696,12 +699,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		if (cnode->ms.map)
+			left_dso = cnode->ms.map->dso;
+		if (node->map)
+			right_dso = node->map->dso;
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-10-05  3:50     ` [PATCH] " Ravi Bangoria
@ 2017-10-05  4:13       ` Namhyung Kim
  2017-10-05  9:12         ` [PATCH v2] " Ravi Bangoria
  0 siblings, 1 reply; 8+ messages in thread
From: Namhyung Kim @ 2017-10-05  4:13 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: pozdneyev, acme, linux-kernel, jolsa, linux-perf-users, peterz,
	mingo, alexander.shishkin, yao.jin, ak, kjlx, milian.wolff,
	zhangmengting, kernel-team

Hi,

On Thu, Oct 05, 2017 at 09:20:21AM +0530, Ravi Bangoria wrote:
> Two functions from different binaries can have same start
> address. Thus, comparing only start address in match_chain()
> leads to inconsistent callchains. Fix this by adding a check
> for dsos as well.
> 
> Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html
> 
> Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/callchain.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index be09d77..6d7f645 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -685,6 +685,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  {
>  	struct symbol *sym = node->sym;
>  	u64 left, right;
> +	struct dso *left_dso = NULL;
> +	struct dso *right_dso = NULL;
> +
>  
>  	if (callchain_param.key == CCKEY_SRCLINE) {
>  		enum match_result match = match_chain_srcline(node, cnode);
> @@ -696,12 +699,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>  		left = cnode->ms.sym->start;
>  		right = sym->start;
> +		if (cnode->ms.map)
> +			left_dso = cnode->ms.map->dso;
> +		if (node->map)
> +			right_dso = node->map->dso;

AFAIK having a symbol guarantees having a map too.  So it could simply
be:

		left_dso = cnode->ms.map->dso;
		right_dso = node->map->dso;

Thanks,
Namhyung


>  	} else {
>  		left = cnode->ip;
>  		right = node->ip;
>  	}
>  
> -	if (left == right) {
> +	if (left == right && left_dso == right_dso) {
>  		if (node->branch) {
>  			cnode->branch_count++;
>  
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-10-05  4:13       ` Namhyung Kim
@ 2017-10-05  9:12         ` Ravi Bangoria
  2017-10-05  9:26           ` Jiri Olsa
  2017-10-05 18:12           ` [tip:perf/urgent] " tip-bot for Ravi Bangoria
  0 siblings, 2 replies; 8+ messages in thread
From: Ravi Bangoria @ 2017-10-05  9:12 UTC (permalink / raw)
  To: pozdneyev, acme, linux-kernel, jolsa, namhyung
  Cc: linux-perf-users, peterz, mingo, alexander.shishkin, yao.jin, ak,
	kjlx, milian.wolff, zhangmengting, Ravi Bangoria

Two functions from different binaries can have same start
address. Thus, comparing only start address in match_chain()
leads to inconsistent callchains. Fix this by adding a check
for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v2:
  - Remove unnecessary checks for 'map'

 tools/perf/util/callchain.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index be09d77..a971caf 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -685,6 +685,8 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -696,12 +698,14 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		left_dso = cnode->ms.map->dso;
+		right_dso = node->map->dso;
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-10-05  9:12         ` [PATCH v2] " Ravi Bangoria
@ 2017-10-05  9:26           ` Jiri Olsa
  2017-10-05 18:12           ` [tip:perf/urgent] " tip-bot for Ravi Bangoria
  1 sibling, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2017-10-05  9:26 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: pozdneyev, acme, linux-kernel, jolsa, namhyung, linux-perf-users,
	peterz, mingo, alexander.shishkin, yao.jin, ak, kjlx,
	milian.wolff, zhangmengting

On Thu, Oct 05, 2017 at 02:42:34PM +0530, Ravi Bangoria wrote:
> Two functions from different binaries can have same start
> address. Thus, comparing only start address in match_chain()
> leads to inconsistent callchains. Fix this by adding a check
> for dsos as well.
> 
> Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html
> 
> Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v2:
>   - Remove unnecessary checks for 'map'

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
>  tools/perf/util/callchain.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index be09d77..a971caf 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -685,6 +685,8 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  {
>  	struct symbol *sym = node->sym;
>  	u64 left, right;
> +	struct dso *left_dso = NULL;
> +	struct dso *right_dso = NULL;
>  
>  	if (callchain_param.key == CCKEY_SRCLINE) {
>  		enum match_result match = match_chain_srcline(node, cnode);
> @@ -696,12 +698,14 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>  		left = cnode->ms.sym->start;
>  		right = sym->start;
> +		left_dso = cnode->ms.map->dso;
> +		right_dso = node->map->dso;
>  	} else {
>  		left = cnode->ip;
>  		right = node->ip;
>  	}
>  
> -	if (left == right) {
> +	if (left == right && left_dso == right_dso) {
>  		if (node->branch) {
>  			cnode->branch_count++;
>  
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [tip:perf/urgent] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION
  2017-10-05  9:12         ` [PATCH v2] " Ravi Bangoria
  2017-10-05  9:26           ` Jiri Olsa
@ 2017-10-05 18:12           ` tip-bot for Ravi Bangoria
  1 sibling, 0 replies; 8+ messages in thread
From: tip-bot for Ravi Bangoria @ 2017-10-05 18:12 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, yao.jin, namhyung, tglx, alexander.shishkin, acme,
	pozdneyev, ak, hpa, ravi.bangoria, peterz, milian.wolff, jolsa,
	kjlx, linux-kernel

Commit-ID:  c1fbc0cf81f1c464f5fda322c1104d4bb1da6711
Gitweb:     https://git.kernel.org/tip/c1fbc0cf81f1c464f5fda322c1104d4bb1da6711
Author:     Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
AuthorDate: Thu, 5 Oct 2017 14:42:34 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 5 Oct 2017 10:52:54 -0300

perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

Two functions from different binaries can have same start address. Thus,
comparing only start address in match_chain() leads to inconsistent
callchains. Fix this by adding a check for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Krister Johansen <kjlx@templeofstupid.com>
Cc: Milian Wolff <milian.wolff@kdab.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Yao Jin <yao.jin@linux.intel.com>
Cc: zhangmengting@huawei.com
Link: http://lkml.kernel.org/r/20171005091234.5874-1-ravi.bangoria@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/callchain.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index be09d77..a971caf 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -685,6 +685,8 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -696,12 +698,14 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		left_dso = cnode->ms.map->dso;
+		right_dso = node->map->dso;
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-10-05 18:17 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-21 14:51 The issue with `perf report -s comm` Alexander Pozdneev
2017-10-04  6:13 ` [RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION Ravi Bangoria
2017-10-04 13:08   ` Jiri Olsa
2017-10-05  3:50     ` [PATCH] " Ravi Bangoria
2017-10-05  4:13       ` Namhyung Kim
2017-10-05  9:12         ` [PATCH v2] " Ravi Bangoria
2017-10-05  9:26           ` Jiri Olsa
2017-10-05 18:12           ` [tip:perf/urgent] " tip-bot for Ravi Bangoria

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.