perf sched map: re-annotate shortname if thread comm changed
PATCH RESEND 1/1 perf sched map: re-annotate shortname if thread comm changed
From: Changbin Du <[email protected]>
This is to show the real name of thread that created via fork-exec. See below example for shortname *A0*.
$ sudo ./perf sched map
*A0 80393.050639 secs A0 => perf:22368
*. A0 80393.050748 secs . => swapper:0
. *. 80393.050887 secs
*B0 . . 80393.052735 secs B0 => rcu_sched:8
*. . . 80393.052743 secs
. *C0 . 80393.056264 secs C0 => kworker/2:1H:287
. *A0 . 80393.056270 secs
. *D0 . 80393.056769 secs D0 => ksoftirqd/2:22
. *A0 . 80393.056804 secs A0 => pi:22368
. *. . 80393.056854 secs
*B0 . . 80393.060727 secs
...--- tools/perf/builtin-sched.c | 4 +++- tools/perf/util/thread.c | 1 + tools/perf/util/thread.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-)
tools/perf/builtin-sched.c +3 -1
@@ -1580,7 +1580,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,timestamp__scnprintf_usec (timestamp, stimestamp,sizeof (stimestamp));color_fprintf (stdout, color," %12s secs " , stimestamp); -if (new_shortname || (verbose >0 && sched_in->tid)) { +if (new_shortname || sched_in->comm_changed || (verbose >0 && sched_in->tid)) {const char *pid_color = color;if (thread__has_color (sched_in))
@@ -1588,6 +1588,8 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,color_fprintf (stdout, pid_color,"%s => %s:%d" , sched_in->shortname,thread__comm_str (sched_in), sched_in->tid); + + sched_in->comm_changed = false; }if (sched->map.comp && new_cpu)
tools/perf/util/thread.c +1 -0
@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;return 0 ;
tools/perf/util/thread.h +1 -0
@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;bool comm_set;int comm_len;bool dead;/* if set thread has exited */
Signature · 1 line
2.7.4
Hello, any comment?
Quoted 75 lines · On Fri, Feb 23, 2018 at 07:40:40PM +0800, [email protected] wrote:
From: Changbin Du <[email protected]>
This is to show the real name of thread that created via fork-exec. See below example for shortname *A0*.
$ sudo ./perf sched map *A0 80393.050639 secs A0 => perf:22368 *. A0 80393.050748 secs . => swapper:0 . *. 80393.050887 secs *B0 . . 80393.052735 secs B0 => rcu_sched:8 *. . . 80393.052743 secs . *C0 . 80393.056264 secs C0 => kworker/2:1H:287 . *A0 . 80393.056270 secs . *D0 . 80393.056769 secs D0 => ksoftirqd/2:22 . *A0 . 80393.056804 secs A0 => pi:22368 . *. . 80393.056854 secs *B0 . . 80393.060727 secs ...Signed-off-by Changbin Du <[email protected]>--- tools/perf/builtin-sched.c | 4 +++- tools/perf/util/thread.c | 1 + tools/perf/util/thread.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-)tools/perf/builtin-sched.c +3
-1@@ -1580,7 +1580,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,timestamp__scnprintf_usec (timestamp, stimestamp,sizeof (stimestamp));color_fprintf (stdout, color," %12s secs " , stimestamp); -if (new_shortname || (verbose >0 && sched_in->tid)) { +if (new_shortname || sched_in->comm_changed || (verbose >0 && sched_in->tid)) {const char *pid_color = color;if (thread__has_color (sched_in))@@ -1588,6 +1588,8 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,color_fprintf (stdout, pid_color,"%s => %s:%d" , sched_in->shortname,thread__comm_str (sched_in), sched_in->tid); + + sched_in->comm_changed = false; }if (sched->map.comp && new_cpu)tools/perf/util/thread.c +1
-0@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;return 0 ;tools/perf/util/thread.h +1
-0@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;bool comm_set;int comm_len;bool dead;/* if set thread has exited */ Signature · 1 line
2.7.4
Signature · 2 lines
Thanks, Changbin Du
Jiri Olsa
Acked-byQuoted 1 line · Changbin
Hello, any comment?
sry, overlooked this one
SNIP
Quoted 11 lines
Quoted 11 lines
tools/perf/util/thread.c +1
-0@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;
Quoted 10 lines
tools/perf/util/thread.h struct thread { · line 30
Quoted 10 lines
tools/perf/util/thread.c +0
-0return 0 ;tools/perf/util/thread.h +1
-0@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;
I don't like that it's in struct thread and set by generic function, and just one command (sched) checks/sets it back.. I'd rather see it in thread::priv area.. on the other hand it's simple enough and looks like generic solution would be more tricky
jirka
Namhyung Kim
maintainerHi,
Quoted 18 lines · Jiri Olsa
Quoted 1 line · Changbin
Hello, any comment?
sry, overlooked this one
SNIP
Quoted 11 lines
Quoted 11 lines
tools/perf/util/thread.c +1
-0@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;
Quoted 14 lines
Quoted 10 lines
tools/perf/util/thread.h struct thread { · line 30
Quoted 10 lines
tools/perf/util/thread.c +0
-0return 0 ;tools/perf/util/thread.h +1
-0@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;I don't like that it's in struct thread and set by generic function, and just one command (sched) checks/sets it back.. I'd rather see it in
thread::privarea..
100% agreed.
Quoted 2 lines
on the other hand it's simple enough and looks
like generic solution would be more tricky
What about adding perf_sched__process_comm() to set it in the thread::priv?
Thanks,
Namhyung
Hi,
Quoted 36 lines · Namhyung Kim
Hi,
Quoted 18 lines · Jiri Olsa
Quoted 1 line · Changbin
Hello, any comment?
sry, overlooked this one
SNIP
Quoted 11 lines
Quoted 11 lines
tools/perf/util/thread.c +1
-0@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;Quoted 14 lines
Quoted 10 lines
tools/perf/util/thread.h struct thread { · line 30
Quoted 10 lines
tools/perf/util/thread.c +0
-0return 0 ;tools/perf/util/thread.h +1
-0@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;I don't like that it's in struct thread and set by generic function, and just one command (sched) checks/sets it back.. I'd rather see it in
thread::privarea..
Quoted 9 lines
100% agreed.
Quoted 2 lines
on the other hand it's simple enough and looks
like generic solution would be more trickyWhat about adding
perf_sched__process_comm()to set it in thethread::priv?
I can be done, then thread->comm_changed moves to thread_runtime->comm_changed. Draft code as below. It is also a little tricky.
+int perf_sched__process_comm(struct perf_tool *tool __maybe_unused,
+ union perf_event *event, + struct perf_sample *sample, + struct machine *machine)
+{
+ struct thread *thread;
+ struct thread_runtime *r;
+
+ perf_event__process_comm(tool, event, sample, machine);
+
+ thread = machine__findnew_thread(machine, pid, tid);
+ if (thread) {
+ r = thread__priv(thread);
+ if (r)
+ r->comm_changed = true;
+ thread__put(thread);
+ }
+}
+
static int perf_sched__read_events(struct perf_sched *sched)
{
const struct perf_evsel_str_handler handlers[] = {
@@ -3291,7 +3311,7 @@ int cmd_sched(int argc, const char **argv)
struct perf_sched sched = {
.tool = {
.sample = perf_sched__process_tracepoint_sample,
- .comm = perf_event__process_comm,
+ .comm = perf_sched__process_comm,But I'd keep 'comm_changed' where 'shortname' is defined. I think they should appears togother. And 'shortname' is only used by sched command, too.
So I still prefer my privous simpler change. Thanks!
Quoted 2 lines
Thanks,
Namhyung
Signature · 2 lines
Thanks, Changbin Du
On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:
SNIP
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
on the other hand it's simple enough and looks
like generic solution would be more trickyWhat about adding
perf_sched__process_comm()to set it in thethread::priv?I can be done, then
thread->comm_changedmoves tothread_runtime->comm_changed. Draft code as below. It is also a little tricky.(quoted diff) +1
-0+int perf_sched__process_comm(struct perf_tool *tool __maybe_unused,
Review
Quoted 10 lines
(quoted diff) +10
-0+ union perf_event *event, + struct perf_sample *sample, + struct machine *machine) +{ + struct thread *thread; + struct thread_runtime *r; + + perf_event__process_comm(tool, event, sample, machine); + + thread = machine__findnew_thread(machine, pid, tid);
should you use machine__find_thread in here?
Quoted 19 lines
(quoted diff) +9
-1+ if (thread) { + r = thread__priv(thread); + if (r) + r->comm_changed = true; + thread__put(thread); + } +} + static int perf_sched__read_events(struct perf_sched *sched) { const struct perf_evsel_str_handler handlers[] = {@@ -3291,7 +3311,7 @@ int cmd_sched(int argc, const char **argv) struct perf_sched sched = { .tool = { .sample = perf_sched__process_tracepoint_sample, - .comm = perf_event__process_comm, + .comm = perf_sched__process_comm,
Quoted 2 lines
But I'd keep '
comm_changed' where 'shortname' is defined. I think they should appears togother. And 'shortname' is only used by sched command, too.
they can both go to struct thread_runtime then
Quoted 2 lines
So I still prefer my privous simpler change. Thanks!
I was wrong thinking that the amount of code
making it sched specific would be biger
we're trying to keep the core structs generic,
so this one fits better
thanks,
jirka
Namhyung Kim
maintainerHi,
Quoted 13 lines · Jiri Olsa
On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:
SNIP
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
on the other hand it's simple enough and looks
like generic solution would be more trickyWhat about adding
perf_sched__process_comm()to set it in thethread::priv?I can be done, then
thread->comm_changedmoves tothread_runtime->comm_changed. Draft code as below. It is also a little tricky.
Quoted 13 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+int perf_sched__process_comm(struct perf_tool *tool __maybe_unused, + union perf_event *event, + struct perf_sample *sample, + struct machine *machine) +{ + struct thread *thread; + struct thread_runtime *r; + + perf_event__process_comm(tool, event, sample, machine); + + thread = machine__findnew_thread(machine, pid, tid);should you use
machine__find_threadin here?
Yep, perf_event__process_comm() already created a new thread if needed. And the return value of it should be checked.
Quoted 7 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ if (thread) { + r = thread__priv(thread); + if (r) + r->comm_changed = true; + thread__put(thread); + }
Missing return.
Quoted 13 lines
Quoted 13 lines
(quoted diff) +3
-1+} + static int perf_sched__read_events(struct perf_sched *sched) { const struct perf_evsel_str_handler handlers[] = {@@ -3291,7 +3311,7 @@ int cmd_sched(int argc, const char **argv) struct perf_sched sched = { .tool = { .sample = perf_sched__process_tracepoint_sample, - .comm = perf_event__process_comm, + .comm = perf_sched__process_comm,
Quoted 4 lines
Quoted 2 lines
But I'd keep '
comm_changed' where 'shortname' is defined. I think they should appears togother. And 'shortname' is only used by sched command, too.they can both go to struct
thread_runtimethen
Agreed.
Quoted 9 lines
Quoted 3 lines
So I still prefer my privous simpler change. Thanks!
I was wrong thinking that the amount of code
making it sched specific would be bigerwe're trying to keep the core structs generic,
so this one fits better
Right.
Thanks,
Namhyung
I just done final version, please check v2. Thanks for your comments!
Quoted 62 lines · Jiri Olsa
On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:
SNIP
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
on the other hand it's simple enough and looks
like generic solution would be more trickyWhat about adding
perf_sched__process_comm()to set it in thethread::priv?I can be done, then
thread->comm_changedmoves tothread_runtime->comm_changed. Draft code as below. It is also a little tricky.(quoted diff) +1
-0+int perf_sched__process_comm(struct perf_tool *tool __maybe_unused,Review
Quoted 10 lines
(quoted diff) +10
-0+ union perf_event *event, + struct perf_sample *sample, + struct machine *machine) +{ + struct thread *thread; + struct thread_runtime *r; + + perf_event__process_comm(tool, event, sample, machine); + + thread = machine__findnew_thread(machine, pid, tid);should you use
machine__find_threadin here?Quoted 19 lines
(quoted diff) +9
-1+ if (thread) { + r = thread__priv(thread); + if (r) + r->comm_changed = true; + thread__put(thread); + } +} + static int perf_sched__read_events(struct perf_sched *sched) { const struct perf_evsel_str_handler handlers[] = {@@ -3291,7 +3311,7 @@ int cmd_sched(int argc, const char **argv) struct perf_sched sched = { .tool = { .sample = perf_sched__process_tracepoint_sample, - .comm = perf_event__process_comm, + .comm = perf_sched__process_comm,Quoted 2 lines
But I'd keep '
comm_changed' where 'shortname' is defined. I think they should appears togother. And 'shortname' is only used by sched command, too.they can both go to struct
thread_runtimethenQuoted 2 lines
So I still prefer my privous simpler change. Thanks!
I was wrong thinking that the amount of code
making it sched specific would be bigerwe're trying to keep the core structs generic,
so this one fits betterthanks,
jirka
Signature · 2 lines
Thanks, Changbin Du
Arnaldo Carvalho de Melo
maintainerEm Fri, Feb 23, 2018 at 07:40:40PM +0800, [email protected] escreveu:
Quoted 4 lines
From: Changbin Du <[email protected]>
This is to show the real name of thread that created via fork-exec. See below example for shortname *A0*.
Can you ellaborate a bit more and perhaps provide before and after outputs?
- Arnaldo
Quoted 69 lines
$ sudo ./perf sched map *A0 80393.050639 secs A0 => perf:22368 *. A0 80393.050748 secs . => swapper:0 . *. 80393.050887 secs *B0 . . 80393.052735 secs B0 => rcu_sched:8 *. . . 80393.052743 secs . *C0 . 80393.056264 secs C0 => kworker/2:1H:287 . *A0 . 80393.056270 secs . *D0 . 80393.056769 secs D0 => ksoftirqd/2:22 . *A0 . 80393.056804 secs A0 => pi:22368 . *. . 80393.056854 secs *B0 . . 80393.060727 secs ...Signed-off-by Changbin Du <[email protected]>--- tools/perf/builtin-sched.c | 4 +++- tools/perf/util/thread.c | 1 + tools/perf/util/thread.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-)tools/perf/builtin-sched.c +3
-1@@ -1580,7 +1580,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,timestamp__scnprintf_usec (timestamp, stimestamp,sizeof (stimestamp));color_fprintf (stdout, color," %12s secs " , stimestamp); -if (new_shortname || (verbose >0 && sched_in->tid)) { +if (new_shortname || sched_in->comm_changed || (verbose >0 && sched_in->tid)) {const char *pid_color = color;if (thread__has_color (sched_in))@@ -1588,6 +1588,8 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,color_fprintf (stdout, pid_color,"%s => %s:%d" , sched_in->shortname,thread__comm_str (sched_in), sched_in->tid); + + sched_in->comm_changed = false; }if (sched->map.comp && new_cpu)tools/perf/util/thread.c +1
-0@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;return 0 ;tools/perf/util/thread.h +1
-0@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;bool comm_set;int comm_len;bool dead;/* if set thread has exited */ Signature · 1 line
2.7.4
Quoted 11 lines · Arnaldo Carvalho de Melo
Em Fri, Feb 23, 2018 at 07:40:40PM +0800, [email protected] escreveu:
Quoted 4 lines
From: Changbin Du <[email protected]>
This is to show the real name of thread that created via fork-exec. See below example for shortname *A0*.
Can you ellaborate a bit more and perhaps provide before and after outputs?
- Arnaldo
Arnaldo, please see below diff stat.
*A0 80393.050639 secs A0 => perf:22368
*. A0 80393.050748 secs . => swapper:0
. *. 80393.050887 secs
*B0 . . 80393.052735 secs B0 => rcu_sched:8
*. . . 80393.052743 secs
. *C0 . 80393.056264 secs C0 => kworker/2:1H:287
. *A0 . 80393.056270 secs
. *D0 . 80393.056769 secs D0 => ksoftirqd/2:22
- . *A0 . 80393.056804 secs
+ . *A0 . 80393.056804 secs A0 => pi:22368
. *. . 80393.056854 secsQuoted 69 lines
Quoted 69 lines
$ sudo ./perf sched map *A0 80393.050639 secs A0 => perf:22368 *. A0 80393.050748 secs . => swapper:0 . *. 80393.050887 secs *B0 . . 80393.052735 secs B0 => rcu_sched:8 *. . . 80393.052743 secs . *C0 . 80393.056264 secs C0 => kworker/2:1H:287 . *A0 . 80393.056270 secs . *D0 . 80393.056769 secs D0 => ksoftirqd/2:22 . *A0 . 80393.056804 secs A0 => pi:22368 . *. . 80393.056854 secs *B0 . . 80393.060727 secs ...Signed-off-by Changbin Du <[email protected]>--- tools/perf/builtin-sched.c | 4 +++- tools/perf/util/thread.c | 1 + tools/perf/util/thread.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-)tools/perf/builtin-sched.c +3
-1@@ -1580,7 +1580,7 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,timestamp__scnprintf_usec (timestamp, stimestamp,sizeof (stimestamp));color_fprintf (stdout, color," %12s secs " , stimestamp); -if (new_shortname || (verbose >0 && sched_in->tid)) { +if (new_shortname || sched_in->comm_changed || (verbose >0 && sched_in->tid)) {const char *pid_color = color;if (thread__has_color (sched_in))@@ -1588,6 +1588,8 @@ static int map_switch_event(struct perf_sched *sched, struct perf_evsel *evsel,color_fprintf (stdout, pid_color,"%s => %s:%d" , sched_in->shortname,thread__comm_str (sched_in), sched_in->tid); + + sched_in->comm_changed = false; }if (sched->map.comp && new_cpu)tools/perf/util/thread.c +1
-0@@ -212,6 +212,7 @@ static int ____thread__set_comm(struct thread *thread, const char *str,unwind__flush_access (thread); } + thread->comm_changed = true; thread->comm_set = true;return 0 ;tools/perf/util/thread.h +1
-0@@ -27,6 +27,7 @@ struct thread {int cpu;refcount_t refcnt;char shortname[3 ]; +bool comm_changed;bool comm_set;int comm_len;bool dead;/* if set thread has exited */ Signature · 1 line
2.7.4
Signature · 2 lines
Thanks, Changbin Du