2014-02-10 04:26:55

by Tony Lu

[permalink] [raw]
Subject: [PATCH] perf tools: Fix the logic of thread__fork

Revert the logic of checking the return value of thread__set_comm().

If thread__set_comm() returns zero without errors, we should not return
immediately, instead we should finish the rest of the clone work.
Otherwise, perf report would fail to resolve symbols sampled in forked
threads.

Signed-off-by: Zhigang Lu <[email protected]>
---
tools/perf/util/thread.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 49eaf1d..e394861 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -126,7 +126,7 @@ int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp)
if (!comm)
return -ENOMEM;
err = thread__set_comm(thread, comm, timestamp);
- if (!err)
+ if (err)
return err;
thread->comm_set = true;
}
--
1.7.1


2014-02-10 12:39:05

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH] perf tools: Fix the logic of thread__fork

Em Mon, Feb 10, 2014 at 04:21:23AM +0000, Tony Lu escreveu:
> Revert the logic of checking the return value of thread__set_comm().
>
> If thread__set_comm() returns zero without errors, we should not return
> immediately, instead we should finish the rest of the clone work.
> Otherwise, perf report would fail to resolve symbols sampled in forked
> threads.

You should mention against which tree your patch is supposed to be
applied, in this case a fix was made in december:

[acme@ssdandy linux]$ git show 8d00be815c05ed0f0202f606bab4e54f98fd3b30
commit 8d00be815c05ed0f0202f606bab4e54f98fd3b30
Author: David Ahern <[email protected]>
Date: Tue Dec 10 21:35:38 2013 -0700

perf tools: Fix inverted error verification bug in thread__fork

Commit 1902efe7f for the new comm infra added the wrong check for return
code on thread__set_comm. err == 0 is normal, so don't return at that
point unless err != 0.

Signed-off-by: David Ahern <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>

> Signed-off-by: Zhigang Lu <[email protected]>
> ---
> tools/perf/util/thread.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 49eaf1d..e394861 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -126,7 +126,7 @@ int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp)
> if (!comm)
> return -ENOMEM;
> err = thread__set_comm(thread, comm, timestamp);
> - if (!err)
> + if (err)
> return err;
> thread->comm_set = true;
> }
> --
> 1.7.1

2014-02-11 07:05:27

by Tony Lu

[permalink] [raw]
Subject: RE: [PATCH] perf tools: Fix the logic of thread__fork

Thanks, I see. I was using the mainline tree, but it was just updated to 3.13-rc8.

>-----Original Message-----
>From: Arnaldo Carvalho de Melo [mailto:[email protected]] On Behalf Of
>Arnaldo Carvalho de Melo
>Sent: Monday, February 10, 2014 8:39 PM
>To: Tony Lu
>Cc: Peter Zijlstra; Paul Mackerras; Ingo Molnar; [email protected];
>Chris Metcalf
>Subject: Re: [PATCH] perf tools: Fix the logic of thread__fork
>
>Em Mon, Feb 10, 2014 at 04:21:23AM +0000, Tony Lu escreveu:
>> Revert the logic of checking the return value of thread__set_comm().
>>
>> If thread__set_comm() returns zero without errors, we should not return
>> immediately, instead we should finish the rest of the clone work.
>> Otherwise, perf report would fail to resolve symbols sampled in forked
>> threads.
>
>You should mention against which tree your patch is supposed to be
>applied, in this case a fix was made in december:
>
>[acme@ssdandy linux]$ git show 8d00be815c05ed0f0202f606bab4e54f98fd3b30
>commit 8d00be815c05ed0f0202f606bab4e54f98fd3b30
>Author: David Ahern <[email protected]>
>Date: Tue Dec 10 21:35:38 2013 -0700
>
> perf tools: Fix inverted error verification bug in thread__fork
>
> Commit 1902efe7f for the new comm infra added the wrong check for return
> code on thread__set_comm. err == 0 is normal, so don't return at that
> point unless err != 0.
>
> Signed-off-by: David Ahern <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Link:
>http://lkml.kernel.org/r/[email protected]
>m
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
>
>> Signed-off-by: Zhigang Lu <[email protected]>
>> ---
>> tools/perf/util/thread.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
>> index 49eaf1d..e394861 100644
>> --- a/tools/perf/util/thread.c
>> +++ b/tools/perf/util/thread.c
>> @@ -126,7 +126,7 @@ int thread__fork(struct thread *thread, struct thread
>*parent, u64 timestamp)
>> if (!comm)
>> return -ENOMEM;
>> err = thread__set_comm(thread, comm, timestamp);
>> - if (!err)
>> + if (err)
>> return err;
>> thread->comm_set = true;
>> }
>> --
>> 1.7.1