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

Groups > linux.kernel > #1286986 > unrolled thread

[PATCH perf/core 16/22] perf: Fix __cmd_top and perf_session__process_events to put the idle thread

Started byMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
First post2015-12-09 03:20 +0100
Last post2015-12-10 11:10 +0100
Articles 3 — 3 participants

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 perf/core 16/22] perf: Fix __cmd_top and  perf_session__process_events to put the idle thread Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:20 +0100
    Re: [PATCH perf/core 16/22] perf: Fix __cmd_top and  perf_session__process_events to put the idle thread Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 15:40 +0100
      RE: [PATCH perf/core 16/22] perf: Fix __cmd_top and  perf_session__process_events to put the idle thread 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 11:10 +0100

#1286986 — [PATCH perf/core 16/22] perf: Fix __cmd_top and perf_session__process_events to put the idle thread

FromMasami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date2015-12-09 03:20 +0100
Subject[PATCH perf/core 16/22] perf: Fix __cmd_top and perf_session__process_events to put the idle thread
Message-ID<qDCwi-25G-11@gated-at.bofh.it>
Since perf_session__register_idle_thread() got the idle thread,
caller functions have to put it afterwards.
Note that since the thread was already inserted to the session
list, it will be released when the session is released.
Also, in perf_session__register_idle_thread() failure path,
the thread should be put before returning.

Refcnt debugger shows that the perf_session__register_idle_thread
gets the returned thread, but the caller (__cmd_top) does not
put the returned idle thread.

  ----
  ==== [0] ====
  Unreclaimed thread@0x24e6240
  Refcount +1 => 0 at
    ./perf(thread__new+0xe5) [0x4c8a75]
    ./perf(machine__findnew_thread+0x9a) [0x4bbdba]
    ./perf(perf_session__register_idle_thread+0x28) [0x4c63c8]
    ./perf(cmd_top+0xd7d) [0x43cf6d]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f06027c5af5]
    ./perf() [0x42272d]
  Refcount +1 => 1 at
    ./perf(thread__get+0x2c) [0x4c8bcc]
    ./perf(machine__findnew_thread+0xee) [0x4bbe0e]
    ./perf(perf_session__register_idle_thread+0x28) [0x4c63c8]
    ./perf(cmd_top+0xd7d) [0x43cf6d]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f06027c5af5]
    ./perf() [0x42272d]
  Refcount +1 => 2 at
    ./perf(thread__get+0x2c) [0x4c8bcc]
    ./perf(machine__findnew_thread+0x112) [0x4bbe32]
    ./perf(perf_session__register_idle_thread+0x28) [0x4c63c8]
    ./perf(cmd_top+0xd7d) [0x43cf6d]
    ./perf() [0x47ba35]
    ./perf(main+0x617) [0x4225b7]
    /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f06027c5af5]
    ./perf() [0x42272d]
  ----

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-top.c  |    6 +++++-
 tools/perf/util/session.c |   10 +++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 7e2e72e..430177f 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -945,6 +945,7 @@ static int perf_top__setup_sample_type(struct perf_top *top __maybe_unused)
 static int __cmd_top(struct perf_top *top)
 {
 	struct record_opts *opts = &top->record_opts;
+	struct thread *idle;
 	pthread_t thread;
 	int ret;
 
@@ -964,8 +965,11 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
-	if (perf_session__register_idle_thread(top->session) == NULL)
+	idle = perf_session__register_idle_thread(top->session);
+	if (idle == NULL)
 		goto out_delete;
+	/* perf_session__register_idle_thread() got the returned thread. */
+	thread__put(idle);
 
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
 				    top->evlist->threads, false, opts->proc_map_timeout);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c35ffdd..ee16228 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1318,6 +1318,8 @@ struct thread *perf_session__register_idle_thread(struct perf_session *session)
 	thread = machine__findnew_thread(&session->machines.host, 0, 0);
 	if (thread == NULL || thread__set_comm(thread, "swapper", 0)) {
 		pr_err("problem inserting idle task.\n");
+		/* machine__findnew_thread() got the thread, so put it */
+		thread__put(thread);
 		thread = NULL;
 	}
 
@@ -1674,10 +1676,16 @@ out_err:
 int perf_session__process_events(struct perf_session *session)
 {
 	u64 size = perf_data_file__size(session->file);
+	struct thread *idle = perf_session__register_idle_thread(session);
 	int err;
 
-	if (perf_session__register_idle_thread(session) == NULL)
+	if (idle == NULL)
 		return -ENOMEM;
+	/*
+	 * Since perf_session__register_idle_thread() got the idle thread,
+	 * we have to put it here.
+	 */
+	thread__put(idle);
 
 	if (!perf_data_file__is_pipe(session->file))
 		err = __perf_session__process_events(session,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1287526

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-12-09 15:40 +0100
Message-ID<qDO4p-1aj-7@gated-at.bofh.it>
In reply to#1286986
Em Wed, Dec 09, 2015 at 11:11:23AM +0900, Masami Hiramatsu escreveu:
> Since perf_session__register_idle_thread() got the idle thread,
> caller functions have to put it afterwards.
> Note that since the thread was already inserted to the session
> list, it will be released when the session is released.
> Also, in perf_session__register_idle_thread() failure path,
> the thread should be put before returning.

Wouldn't this be better by making perf_session__register_idle_thread()
return -1 if it fails? I.e. that way its callers won't have to
immediately put the idle thread, as they are doing nothing with it.

In the future, if someone needs a handle for that thread, a lookup can
be done:

  idle = machine__find_thread(&session->machines.host, 0, 0);

I.e. this would be the resulting patch, please let me know if you are ok
with this approach:

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 7e2e72e6d9d1..f26b08e72f74 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -964,7 +964,7 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
-	if (perf_session__register_idle_thread(top->session) == NULL)
+	if (perf_session__register_idle_thread(top->session) < 0)
 		goto out_delete;
 
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c35ffdd360fe..9774686525b4 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1311,17 +1311,20 @@ struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
 	return machine__findnew_thread(&session->machines.host, -1, pid);
 }
 
-struct thread *perf_session__register_idle_thread(struct perf_session *session)
+int perf_session__register_idle_thread(struct perf_session *session)
 {
 	struct thread *thread;
+	int err = 0;
 
 	thread = machine__findnew_thread(&session->machines.host, 0, 0);
 	if (thread == NULL || thread__set_comm(thread, "swapper", 0)) {
 		pr_err("problem inserting idle task.\n");
-		thread = NULL;
+		err = -1;
 	}
 
-	return thread;
+	/* machine__findnew_thread() got the thread, so put it */
+	thread__put(thread);
+	return err;
 }
 
 static void perf_session__warn_about_errors(const struct perf_session *session)
@@ -1676,7 +1679,7 @@ int perf_session__process_events(struct perf_session *session)
 	u64 size = perf_data_file__size(session->file);
 	int err;
 
-	if (perf_session__register_idle_thread(session) == NULL)
+	if (perf_session__register_idle_thread(session) < 0)
 		return -ENOMEM;
 
 	if (!perf_data_file__is_pipe(session->file))
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 3e900c0efc73..5f792e35d4c1 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -89,7 +89,7 @@ struct machine *perf_session__findnew_machine(struct perf_session *session, pid_
 }
 
 struct thread *perf_session__findnew(struct perf_session *session, pid_t pid);
-struct thread *perf_session__register_idle_thread(struct perf_session *session);
+int perf_session__register_idle_thread(struct perf_session *session);
 
 size_t perf_session__fprintf(struct perf_session *session, FILE *fp);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1288419

From平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com>
Date2015-12-10 11:10 +0100
Message-ID<qE6kF-4M5-9@gated-at.bofh.it>
In reply to#1287526
>From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>
>Em Wed, Dec 09, 2015 at 11:11:23AM +0900, Masami Hiramatsu escreveu:
>> Since perf_session__register_idle_thread() got the idle thread,
>> caller functions have to put it afterwards.
>> Note that since the thread was already inserted to the session
>> list, it will be released when the session is released.
>> Also, in perf_session__register_idle_thread() failure path,
>> the thread should be put before returning.
>
>Wouldn't this be better by making perf_session__register_idle_thread()
>return -1 if it fails? I.e. that way its callers won't have to
>immediately put the idle thread, as they are doing nothing with it.
>

Ah, right. I thought that someone may use this return value, but no,
there is no code which use the returned thread.

>In the future, if someone needs a handle for that thread, a lookup can
>be done:
>
>  idle = machine__find_thread(&session->machines.host, 0, 0);
>
>I.e. this would be the resulting patch, please let me know if you are ok
>with this approach:

I'm OK for your approach :)

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>


Thanks!


>
>diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>index 7e2e72e6d9d1..f26b08e72f74 100644
>--- a/tools/perf/builtin-top.c
>+++ b/tools/perf/builtin-top.c
>@@ -964,7 +964,7 @@ static int __cmd_top(struct perf_top *top)
> 	if (ret)
> 		goto out_delete;
>
>-	if (perf_session__register_idle_thread(top->session) == NULL)
>+	if (perf_session__register_idle_thread(top->session) < 0)
> 		goto out_delete;
>
> 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
>diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>index c35ffdd360fe..9774686525b4 100644
>--- a/tools/perf/util/session.c
>+++ b/tools/perf/util/session.c
>@@ -1311,17 +1311,20 @@ struct thread *perf_session__findnew(struct perf_session *session, pid_t pid)
> 	return machine__findnew_thread(&session->machines.host, -1, pid);
> }
>
>-struct thread *perf_session__register_idle_thread(struct perf_session *session)
>+int perf_session__register_idle_thread(struct perf_session *session)
> {
> 	struct thread *thread;
>+	int err = 0;
>
> 	thread = machine__findnew_thread(&session->machines.host, 0, 0);
> 	if (thread == NULL || thread__set_comm(thread, "swapper", 0)) {
> 		pr_err("problem inserting idle task.\n");
>-		thread = NULL;
>+		err = -1;
> 	}
>
>-	return thread;
>+	/* machine__findnew_thread() got the thread, so put it */
>+	thread__put(thread);
>+	return err;
> }
>
> static void perf_session__warn_about_errors(const struct perf_session *session)
>@@ -1676,7 +1679,7 @@ int perf_session__process_events(struct perf_session *session)
> 	u64 size = perf_data_file__size(session->file);
> 	int err;
>
>-	if (perf_session__register_idle_thread(session) == NULL)
>+	if (perf_session__register_idle_thread(session) < 0)
> 		return -ENOMEM;
>
> 	if (!perf_data_file__is_pipe(session->file))
>diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
>index 3e900c0efc73..5f792e35d4c1 100644
>--- a/tools/perf/util/session.h
>+++ b/tools/perf/util/session.h
>@@ -89,7 +89,7 @@ struct machine *perf_session__findnew_machine(struct perf_session *session, pid_
> }
>
> struct thread *perf_session__findnew(struct perf_session *session, pid_t pid);
>-struct thread *perf_session__register_idle_thread(struct perf_session *session);
>+int perf_session__register_idle_thread(struct perf_session *session);
>
> size_t perf_session__fprintf(struct perf_session *session, FILE *fp);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web