Skip to contentSkip to conversation

perf sched map: re-annotate shortname if thread comm changed

PATCH RESEND Superseded [email protected] 4 participants 10 messages Feb 23, 2018 – Mar 6, 2018 perf events

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

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
index 83283fe..53bb8df 100644
@@ -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
index 68b65b1..c660fe6 100644
@@ -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
index 40cfa36..b9a328b 100644
@@ -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
#1 lore raw

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
      ...
---
 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
index 83283fe..53bb8df 100644
@@ -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
index 68b65b1..c660fe6 100644
@@ -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
index 40cfa36..b9a328b 100644
@@ -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
Acked-by
#2 lore raw
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
index 68b65b1..c660fe6 100644
@@ -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 -0
 	return 0;
tools/perf/util/thread.h +1 -0
index 40cfa36..b9a328b 100644
@@ -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

maintainer
#3 lore raw

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
index 68b65b1..c660fe6 100644
@@ -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 -0
 	return 0;
tools/perf/util/thread.h +1 -0
index 40cfa36..b9a328b 100644
@@ -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..

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

#4 lore raw

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
index 68b65b1..c660fe6 100644
@@ -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 -0
 	return 0;
tools/perf/util/thread.h +1 -0
index 40cfa36..b9a328b 100644
@@ -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..

Quoted 9 lines

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?

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
#5 lore raw

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 tricky

What about adding perf_sched__process_comm() to set it in the thread::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.

(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

maintainer
#6 lore raw

Hi,

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 tricky

What about adding perf_sched__process_comm() to set it in the thread::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.

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_thread in 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_runtime then

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 biger

we're trying to keep the core structs generic,
so this one fits better

Right.

Thanks,
Namhyung

#7 lore raw
In reply to Jiri Olsa

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 tricky

What about adding perf_sched__process_comm() to set it in the thread::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.

(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

Signature · 2 lines
Thanks,
Changbin Du
#8 lore raw
In reply to [email protected]

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

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
      ...
---
 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
index 83283fe..53bb8df 100644
@@ -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
index 68b65b1..c660fe6 100644
@@ -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
index 40cfa36..b9a328b 100644
@@ -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
#9 lore raw
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 secs
Quoted 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
      ...
---
 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
index 83283fe..53bb8df 100644
@@ -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
index 68b65b1..c660fe6 100644
@@ -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
index 40cfa36..b9a328b 100644
@@ -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