[RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[changbin.du]

From: Changbin Du <changbin.du@intel.com>

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 <changbin.du@intel.com>
---
 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(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 83283fe..53bb8df 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -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)
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 68b65b1..c660fe6 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -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;
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 40cfa36..b9a328b 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -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 */
-- 
2.7.4

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Du, Changbin]

Hello, any comment?

On Fri, Feb 23, 2018 at 07:40:40PM +0800, changbin.du@intel.com wrote:
> From: Changbin Du <changbin.du@intel.com>
> 
> 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 <changbin.du@intel.com>
> ---
>  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(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 83283fe..53bb8df 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -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)
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 68b65b1..c660fe6 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -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;
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 40cfa36..b9a328b 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -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 */
> -- 
> 2.7.4
> 

-- 
Thanks,
Changbin Du

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Jiri Olsa]

On Fri, Mar 02, 2018 at 06:52:54PM +0800, Du, Changbin wrote:
> Hello, any comment?

sry, overlooked this one

SNIP

> > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> > index 68b65b1..c660fe6 100644
> > --- a/tools/perf/util/thread.c
> > +++ b/tools/perf/util/thread.c
> > @@ -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;
> > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> > index 40cfa36..b9a328b 100644
> > --- a/tools/perf/util/thread.h
> > +++ b/tools/perf/util/thread.h
> > @@ -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

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Arnaldo Carvalho de Melo]

Em Fri, Feb 23, 2018 at 07:40:40PM +0800, changbin.du@intel.com escreveu:
> From: Changbin Du <changbin.du@intel.com>
> 
> 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
 
> $ 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 <changbin.du@intel.com>
> ---
>  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(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 83283fe..53bb8df 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -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)
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 68b65b1..c660fe6 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -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;
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 40cfa36..b9a328b 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -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 */
> -- 
> 2.7.4

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Namhyung Kim]

Hi,

On Fri, Mar 02, 2018 at 12:38:45PM +0100, Jiri Olsa wrote:
> On Fri, Mar 02, 2018 at 06:52:54PM +0800, Du, Changbin wrote:
> > Hello, any comment?
> 
> sry, overlooked this one
> 
> SNIP
> 
> > > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> > > index 68b65b1..c660fe6 100644
> > > --- a/tools/perf/util/thread.c
> > > +++ b/tools/perf/util/thread.c
> > > @@ -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;
> > > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> > > index 40cfa36..b9a328b 100644
> > > --- a/tools/perf/util/thread.h
> > > +++ b/tools/perf/util/thread.h
> > > @@ -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.


> 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

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Du, Changbin]

Hi,
On Fri, Mar 02, 2018 at 11:47:32PM +0900, Namhyung Kim wrote:
> Hi,
> 
> On Fri, Mar 02, 2018 at 12:38:45PM +0100, Jiri Olsa wrote:
> > On Fri, Mar 02, 2018 at 06:52:54PM +0800, Du, Changbin wrote:
> > > Hello, any comment?
> > 
> > sry, overlooked this one
> > 
> > SNIP
> > 
> > > > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> > > > index 68b65b1..c660fe6 100644
> > > > --- a/tools/perf/util/thread.c
> > > > +++ b/tools/perf/util/thread.c
> > > > @@ -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;
> > > > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> > > > index 40cfa36..b9a328b 100644
> > > > --- a/tools/perf/util/thread.h
> > > > +++ b/tools/perf/util/thread.h
> > > > @@ -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.
> 
> 
> > 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!

> Thanks,
> Namhyung

-- 
Thanks,
Changbin Du

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Du, Changbin]

On Fri, Mar 02, 2018 at 11:43:12AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Feb 23, 2018 at 07:40:40PM +0800, changbin.du@intel.com escreveu:
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > 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


> > $ 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 <changbin.du@intel.com>
> > ---
> >  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(-)
> > 
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 83283fe..53bb8df 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -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)
> > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> > index 68b65b1..c660fe6 100644
> > --- a/tools/perf/util/thread.c
> > +++ b/tools/perf/util/thread.c
> > @@ -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;
> > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> > index 40cfa36..b9a328b 100644
> > --- a/tools/perf/util/thread.h
> > +++ b/tools/perf/util/thread.h
> > @@ -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 */
> > -- 
> > 2.7.4

-- 
Thanks,
Changbin Du

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Jiri Olsa]

On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:

SNIP

> > > 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);

should you use machine__find_thread in here?

> +       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.

they can both go to struct thread_runtime then

> 
> 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

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Namhyung Kim]

Hi,

On Mon, Mar 05, 2018 at 11:37:54PM +0100, Jiri Olsa wrote:
> On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:
> 
> SNIP
> 
> > > > 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);
> 
> 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.


> 
> > +       if (thread) {
> > +               r = thread__priv(thread);
> > +               if (r)
> > +                       r->comm_changed = true;
> > +               thread__put(thread);
> > +       }

Missing return.

> > +}
> > +
> >  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.
> 
> they can both go to struct thread_runtime then

Agreed.


> 
> > 
> > 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

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Du, Changbin]

I just done final version, please check v2. Thanks for your comments!

On Mon, Mar 05, 2018 at 11:37:54PM +0100, Jiri Olsa wrote:
> On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:
> 
> SNIP
> 
> > > > 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);
> 
> should you use machine__find_thread in here?
> 
> > +       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.
> 
> they can both go to struct thread_runtime then
> 
> > 
> > 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

-- 
Thanks,
Changbin Du