[PATCH] perf tools: Fix the logic of thread__fork

[PATCH] perf tools: Fix the logic of thread__fork

[Tony Lu]

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 <zlu@tilera.com>
---
 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

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

[Arnaldo Carvalho de Melo]

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 <dsahern@gmail.com>
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 <dsahern@gmail.com>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Link: http://lkml.kernel.org/r/1386736538-23525-1-git-send-email-dsahern@gmail.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
 
> Signed-off-by: Zhigang Lu <zlu@tilera.com>
> ---
>  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

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

[Tony Lu]

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:arnaldo.melo@gmail.com] 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; linux-kernel@vger.kernel.org;
>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 <dsahern@gmail.com>
>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 <dsahern@gmail.com>
>    Cc: Frederic Weisbecker <fweisbec@gmail.com>
>    Link:
>http://lkml.kernel.org/r/1386736538-23525-1-git-send-email-dsahern@gmail.co
>m
>    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
>> Signed-off-by: Zhigang Lu <zlu@tilera.com>
>> ---
>>  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