Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1536639 > unrolled thread

[PATCH 03/10] perf sched timehist: Handle zero sample->tid properly

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly Namhyung Kim <namhyung@kernel.org> - 2016-12-06 04:50 +0100
    Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid  properly David Ahern <dsahern@gmail.com> - 2016-12-06 05:00 +0100
      Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly Namhyung Kim <namhyung@kernel.org> - 2016-12-06 05:10 +0100
        Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid  properly David Ahern <dsahern@gmail.com> - 2016-12-06 05:10 +0100
      Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid  properly Namhyung Kim <namhyung@kernel.org> - 2016-12-07 03:30 +0100
        Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid  properly Namhyung Kim <namhyung@kernel.org> - 2016-12-08 15:30 +0100
    [tip:perf/core] perf sched timehist: Handle zero sample->tid  properly tip-bot for Namhyung Kim <tipbot@zytor.com> - 2016-12-07 19:30 +0100

#1536639 — [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly

Sometimes samples have tid of 0 but non-0 pid.  It ends up having a
new thread of 0 tid/pid (instead of referring idle task) since tid is
used to search matching task.  But I guess it's wrong to use 0 as a
tid when pid is set.  This patch uses tid only if it has a non-zero
value or same as pid (of 0).

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

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 6991686bcaa5..e34a71166b4a 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -2118,7 +2118,9 @@ static struct thread *timehist_get_thread(struct perf_sched *sched,
 			pr_err("Failed to get idle thread for cpu %d.\n", sample->cpu);
 
 	} else {
-		thread = machine__findnew_thread(machine, sample->pid, sample->tid);
+		/* there were samples with tid 0 but non-zero pid */
+		thread = machine__findnew_thread(machine, sample->pid,
+						 sample->tid ?: sample->pid);
 		if (thread == NULL) {
 			pr_debug("Failed to get thread for tid %d. skipping sample.\n",
 				 sample->tid);
-- 
2.10.1

[toc] | [next] | [standalone]

#1536650 — Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly

On 12/5/16 7:40 PM, Namhyung Kim wrote:
> Sometimes samples have tid of 0 but non-0 pid.  It ends up having a

Any idea how that happens?

> new thread of 0 tid/pid (instead of referring idle task) since tid is
> used to search matching task.  But I guess it's wrong to use 0 as a
> tid when pid is set.  This patch uses tid only if it has a non-zero
> value or same as pid (of 0).

[toc] | [prev] | [next] | [standalone]

#1536662

Hi David,

On Tue, Dec 6, 2016 at 12:52 PM, David Ahern <dsahern@gmail.com> wrote:
> On 12/5/16 7:40 PM, Namhyung Kim wrote:
>> Sometimes samples have tid of 0 but non-0 pid.  It ends up having a
>
> Any idea how that happens?

No, I didn't investigate it yet.  Looking at the original code, you
seemed to have same issue and workaround like checking prev_pid or
callchains, right?

Thanks,
Namhyung

>
>> new thread of 0 tid/pid (instead of referring idle task) since tid is
>> used to search matching task.  But I guess it's wrong to use 0 as a
>> tid when pid is set.  This patch uses tid only if it has a non-zero
>> value or same as pid (of 0).

[toc] | [prev] | [next] | [standalone]

#1536663 — Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly

On 12/5/16 7:59 PM, Namhyung Kim wrote:
> No, I didn't investigate it yet.  Looking at the original code, you
> seemed to have same issue and workaround like checking prev_pid or
> callchains, right?

most likely. As I responded on another patch, the sched timehist command has been used for years on a range of OS'es and kernel versions. Most of the oddities you see are (sometime quick) fixes to strange differences in kernel versions.

[toc] | [prev] | [next] | [standalone]

#1537428 — Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly

On Mon, Dec 05, 2016 at 07:52:57PM -0800, David Ahern wrote:
> On 12/5/16 7:40 PM, Namhyung Kim wrote:
> > Sometimes samples have tid of 0 but non-0 pid.  It ends up having a
> 
> Any idea how that happens?

It seems that an exiting task wakes up its parent and the parent might
call wait(2) concurrently.  So at the time of calling last schedule(),
its pid (and tgid) link might be unhashed by the parent and can have 0
sample->tid and/or sample->pid depending on timing IMHO.  Not sure
anything guarantees that the sample tid/pid is preserved during the
event.  From a quick look I couldn't find..

If that's true we need to somehow make sure that sample->tid of 0 is
actually from idle task or not.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1538569 — Re: [PATCH 03/10] perf sched timehist: Handle zero sample->tid properly

On Wed, Dec 07, 2016 at 11:06:29AM +0900, Namhyung Kim wrote:
> On Mon, Dec 05, 2016 at 07:52:57PM -0800, David Ahern wrote:
> > On 12/5/16 7:40 PM, Namhyung Kim wrote:
> > > Sometimes samples have tid of 0 but non-0 pid.  It ends up having a
> > 
> > Any idea how that happens?
> 
> It seems that an exiting task wakes up its parent and the parent might
> call wait(2) concurrently.  So at the time of calling last schedule(),
> its pid (and tgid) link might be unhashed by the parent and can have 0
> sample->tid and/or sample->pid depending on timing IMHO.  Not sure
> anything guarantees that the sample tid/pid is preserved during the
> event.  From a quick look I couldn't find..

I found following line in my data file.

  $ perf script --time 52460.536907,
  swapper     0 [000] 52460.536907:       sched:sched_switch: pool:3775 [120] x ==> at-spi2-registr:1961 [120]
                    7f3963 __schedule (/lib/modules/4.8.3-1-ARCH/build/vmlinux)
                    7f3d7c schedule (/lib/modules/4.8.3-1-ARCH/build/vmlinux)
                    280b14 do_exit (/lib/modules/4.8.3-1-ARCH/build/vmlinux)
                    280fa7 [unknown] (/lib/modules/4.8.3-1-ARCH/build/vmlinux)
                    7f7cf2 entry_SYSCALL_64_fastpath (/lib/modules/4.8.3-1-ARCH/build/vmlinux)
  ...

As you can see task 3775 called schedule() during exit but sample tid
was 0.  In this case prev_pid is still correct (though it's always pid
in the root namespace) since its number was saved in task_struct at the
time of creation and not changed.

I'll change to check prev_pid for idle task.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1537985 — [tip:perf/core] perf sched timehist: Handle zero sample->tid properly

Commit-ID:  5d92d96a947a5d0d83710d11750bb29a0c1b985d
Gitweb:     http://git.kernel.org/tip/5d92d96a947a5d0d83710d11750bb29a0c1b985d
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 6 Dec 2016 12:40:03 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 7 Dec 2016 12:00:34 -0300

perf sched timehist: Handle zero sample->tid properly

Sometimes samples have tid of 0 but non-0 pid.  It ends up having a new
thread of 0 tid/pid (instead of referring idle task) since tid is used
to search matching task.  But I guess it's wrong to use 0 as a tid when
pid is set.  This patch uses tid only if it has a non-zero value or same
as pid (of 0).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20161206034010.6499-4-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-sched.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 634d8cf..c8b3e6c 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -2118,7 +2118,9 @@ static struct thread *timehist_get_thread(struct perf_sched *sched,
 			pr_err("Failed to get idle thread for cpu %d.\n", sample->cpu);
 
 	} else {
-		thread = machine__findnew_thread(machine, sample->pid, sample->tid);
+		/* there were samples with tid 0 but non-zero pid */
+		thread = machine__findnew_thread(machine, sample->pid,
+						 sample->tid ?: sample->pid);
 		if (thread == NULL) {
 			pr_debug("Failed to get thread for tid %d. skipping sample.\n",
 				 sample->tid);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web