diff mbox series

perf util: Fix evlist->threads when working with 'perf stat --per-thread'

Message ID 1515775493-14254-1-git-send-email-mathieu.poirier@linaro.org
State New
Headers show
Series perf util: Fix evlist->threads when working with 'perf stat --per-thread' | expand

Commit Message

Mathieu Poirier Jan. 12, 2018, 4:44 p.m. UTC 
After commit ("73c0ca1eee3d perf thread_map: Enumerate all threads
from /proc") any condition where target->per_thread is set will see all the
threads in a system added to evlist->threads.

Since the 'record' utility also uses function thread_map__new_str(), any
attempt to use the --perf-thread option will also end up accounting for all
threads in a system, resulting in new kernel events being created for all
threads rather than just the thread of interest.

This patch keeps the newly introduced functionality but make sure it
doesn't affect other utilities outside of 'stat'.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

---
 tools/perf/builtin-stat.c |  2 +-
 tools/perf/util/evlist.c  | 29 ++++++++++++++++++++++++-----
 tools/perf/util/evlist.h  |  2 ++
 3 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.7.4

Comments

Arnaldo Carvalho de Melo Jan. 12, 2018, 7:53 p.m. UTC | #1 
Em Fri, Jan 12, 2018 at 09:44:53AM -0700, Mathieu Poirier escreveu:
> After commit ("73c0ca1eee3d perf thread_map: Enumerate all threads

> from /proc") any condition where target->per_thread is set will see all the

> threads in a system added to evlist->threads.

> 

> Since the 'record' utility also uses function thread_map__new_str(), any

> attempt to use the --perf-thread option will also end up accounting for all


--perf-thread? Do you mean --per-thread, ok, so 'record' has this, uses
the opts->per_thread that then Jin reused for his recent patches... Jin,
can you try to untangle this, probably you can't reuse that
opts->per-thread thing...

Without looking further, I think that could be a better avenue to fix
this issue, without having to add a 'stat' specific method to the core
classes.

- Arnaldo

> threads in a system, resulting in new kernel events being created for all

> threads rather than just the thread of interest.

> 

> This patch keeps the newly introduced functionality but make sure it

> doesn't affect other utilities outside of 'stat'.

> 

> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> ---

>  tools/perf/builtin-stat.c |  2 +-

>  tools/perf/util/evlist.c  | 29 ++++++++++++++++++++++++-----

>  tools/perf/util/evlist.h  |  2 ++

>  3 files changed, 27 insertions(+), 6 deletions(-)

> 

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c

> index 98bf9d32f222..82d16426b984 100644

> --- a/tools/perf/builtin-stat.c

> +++ b/tools/perf/builtin-stat.c

> @@ -2833,7 +2833,7 @@ int cmd_stat(int argc, const char **argv)

>  	if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))

>  		target.per_thread = true;

>  

> -	if (perf_evlist__create_maps(evsel_list, &target) < 0) {

> +	if (perf_evlist__create_stat_maps(evsel_list, &target) < 0) {

>  		if (target__has_task(&target)) {

>  			pr_err("Problems finding threads of monitor\n");

>  			parse_options_usage(stat_usage, stat_options, "p", 1);

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c

> index f0a5e09c4071..a4ae684f28de 100644

> --- a/tools/perf/util/evlist.c

> +++ b/tools/perf/util/evlist.c

> @@ -1100,13 +1100,11 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages)

>  	return perf_evlist__mmap_ex(evlist, pages, 0, false);

>  }

>  

> -int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

> +static int _perf_evlist__create_maps(struct perf_evlist *evlist,

> +				     struct target *target,

> +				     struct thread_map *threads)

>  {

>  	struct cpu_map *cpus;

> -	struct thread_map *threads;

> -

> -	threads = thread_map__new_str(target->pid, target->tid, target->uid,

> -				      target->per_thread);

>  

>  	if (!threads)

>  		return -1;

> @@ -1130,6 +1128,27 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>  	return -1;

>  }

>  

> +int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

> +{

> +	struct thread_map *threads;

> +

> +	threads = thread_map__new_str(target->pid, target->tid, target->uid,

> +				      false);

> +

> +	return _perf_evlist__create_maps(evlist, target, threads);

> +}

> +

> +int perf_evlist__create_stat_maps(struct perf_evlist *evlist,

> +				  struct target *target)

> +{

> +	struct thread_map *threads;

> +

> +	threads = thread_map__new_str(target->pid, target->tid, target->uid,

> +				      target->per_thread);

> +

> +	return _perf_evlist__create_maps(evlist, target, threads);

> +}

> +

>  void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,

>  			   struct thread_map *threads)

>  {

> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h

> index 75160666d305..80c654990e19 100644

> --- a/tools/perf/util/evlist.h

> +++ b/tools/perf/util/evlist.h

> @@ -188,6 +188,8 @@ void perf_evlist__set_selected(struct perf_evlist *evlist,

>  void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,

>  			   struct thread_map *threads);

>  int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target);

> +int perf_evlist__create_stat_maps(struct perf_evlist *evlist,

> +				  struct target *target);

>  int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel);

>  

>  void __perf_evlist__set_leader(struct list_head *list);

> -- 

> 2.7.4
Mathieu Poirier Jan. 12, 2018, 8:45 p.m. UTC | #2 
On 12 January 2018 at 12:53, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> Em Fri, Jan 12, 2018 at 09:44:53AM -0700, Mathieu Poirier escreveu:

>> After commit ("73c0ca1eee3d perf thread_map: Enumerate all threads

>> from /proc") any condition where target->per_thread is set will see all the

>> threads in a system added to evlist->threads.

>>

>> Since the 'record' utility also uses function thread_map__new_str(), any

>> attempt to use the --perf-thread option will also end up accounting for all

>

> --perf-thread? Do you mean --per-thread, ok, so 'record' has this, uses

> the opts->per_thread that then Jin reused for his recent patches... Jin,

> can you try to untangle this, probably you can't reuse that

> opts->per-thread thing...


Right, I meant --per-thread.

>

> Without looking further, I think that could be a better avenue to fix

> this issue, without having to add a 'stat' specific method to the core

> classes.


Ok, let me have another look at builtin-stat.c...

>

> - Arnaldo

>

>> threads in a system, resulting in new kernel events being created for all

>> threads rather than just the thread of interest.

>>

>> This patch keeps the newly introduced functionality but make sure it

>> doesn't affect other utilities outside of 'stat'.

>>

>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>> ---

>>  tools/perf/builtin-stat.c |  2 +-

>>  tools/perf/util/evlist.c  | 29 ++++++++++++++++++++++++-----

>>  tools/perf/util/evlist.h  |  2 ++

>>  3 files changed, 27 insertions(+), 6 deletions(-)

>>

>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c

>> index 98bf9d32f222..82d16426b984 100644

>> --- a/tools/perf/builtin-stat.c

>> +++ b/tools/perf/builtin-stat.c

>> @@ -2833,7 +2833,7 @@ int cmd_stat(int argc, const char **argv)

>>       if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))

>>               target.per_thread = true;

>>

>> -     if (perf_evlist__create_maps(evsel_list, &target) < 0) {

>> +     if (perf_evlist__create_stat_maps(evsel_list, &target) < 0) {

>>               if (target__has_task(&target)) {

>>                       pr_err("Problems finding threads of monitor\n");

>>                       parse_options_usage(stat_usage, stat_options, "p", 1);

>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c

>> index f0a5e09c4071..a4ae684f28de 100644

>> --- a/tools/perf/util/evlist.c

>> +++ b/tools/perf/util/evlist.c

>> @@ -1100,13 +1100,11 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages)

>>       return perf_evlist__mmap_ex(evlist, pages, 0, false);

>>  }

>>

>> -int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>> +static int _perf_evlist__create_maps(struct perf_evlist *evlist,

>> +                                  struct target *target,

>> +                                  struct thread_map *threads)

>>  {

>>       struct cpu_map *cpus;

>> -     struct thread_map *threads;

>> -

>> -     threads = thread_map__new_str(target->pid, target->tid, target->uid,

>> -                                   target->per_thread);

>>

>>       if (!threads)

>>               return -1;

>> @@ -1130,6 +1128,27 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>>       return -1;

>>  }

>>

>> +int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>> +{

>> +     struct thread_map *threads;

>> +

>> +     threads = thread_map__new_str(target->pid, target->tid, target->uid,

>> +                                   false);

>> +

>> +     return _perf_evlist__create_maps(evlist, target, threads);

>> +}

>> +

>> +int perf_evlist__create_stat_maps(struct perf_evlist *evlist,

>> +                               struct target *target)

>> +{

>> +     struct thread_map *threads;

>> +

>> +     threads = thread_map__new_str(target->pid, target->tid, target->uid,

>> +                                   target->per_thread);

>> +

>> +     return _perf_evlist__create_maps(evlist, target, threads);

>> +}

>> +

>>  void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,

>>                          struct thread_map *threads)

>>  {

>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h

>> index 75160666d305..80c654990e19 100644

>> --- a/tools/perf/util/evlist.h

>> +++ b/tools/perf/util/evlist.h

>> @@ -188,6 +188,8 @@ void perf_evlist__set_selected(struct perf_evlist *evlist,

>>  void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,

>>                          struct thread_map *threads);

>>  int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target);

>> +int perf_evlist__create_stat_maps(struct perf_evlist *evlist,

>> +                               struct target *target);

>>  int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel);

>>

>>  void __perf_evlist__set_leader(struct list_head *list);

>> --

>> 2.7.4
Jin, Yao Jan. 22, 2018, 11:43 a.m. UTC | #3 
On 1/13/2018 3:53 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 12, 2018 at 09:44:53AM -0700, Mathieu Poirier escreveu:

>> After commit ("73c0ca1eee3d perf thread_map: Enumerate all threads

>> from /proc") any condition where target->per_thread is set will see all the

>> threads in a system added to evlist->threads.

>>

>> Since the 'record' utility also uses function thread_map__new_str(), any

>> attempt to use the --perf-thread option will also end up accounting for all

> 

> --perf-thread? Do you mean --per-thread, ok, so 'record' has this, uses

> the opts->per_thread that then Jin reused for his recent patches... Jin,

> can you try to untangle this, probably you can't reuse that

> opts->per-thread thing...

> 

> Without looking further, I think that could be a better avenue to fix

> this issue, without having to add a 'stat' specific method to the core

> classes.

> 

> - Arnaldo

> 


Hi,

Very sorry, I just see this mail today.

Please let me look at this issue first.

Thanks
Jin Yao

>> threads in a system, resulting in new kernel events being created for all

>> threads rather than just the thread of interest.

>>

>> This patch keeps the newly introduced functionality but make sure it

>> doesn't affect other utilities outside of 'stat'.

>>

>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>> ---

>>   tools/perf/builtin-stat.c |  2 +-

>>   tools/perf/util/evlist.c  | 29 ++++++++++++++++++++++++-----

>>   tools/perf/util/evlist.h  |  2 ++

>>   3 files changed, 27 insertions(+), 6 deletions(-)

>>

>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c

>> index 98bf9d32f222..82d16426b984 100644

>> --- a/tools/perf/builtin-stat.c

>> +++ b/tools/perf/builtin-stat.c

>> @@ -2833,7 +2833,7 @@ int cmd_stat(int argc, const char **argv)

>>   	if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))

>>   		target.per_thread = true;

>>   

>> -	if (perf_evlist__create_maps(evsel_list, &target) < 0) {

>> +	if (perf_evlist__create_stat_maps(evsel_list, &target) < 0) {

>>   		if (target__has_task(&target)) {

>>   			pr_err("Problems finding threads of monitor\n");

>>   			parse_options_usage(stat_usage, stat_options, "p", 1);

>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c

>> index f0a5e09c4071..a4ae684f28de 100644

>> --- a/tools/perf/util/evlist.c

>> +++ b/tools/perf/util/evlist.c

>> @@ -1100,13 +1100,11 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages)

>>   	return perf_evlist__mmap_ex(evlist, pages, 0, false);

>>   }

>>   

>> -int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>> +static int _perf_evlist__create_maps(struct perf_evlist *evlist,

>> +				     struct target *target,

>> +				     struct thread_map *threads)

>>   {

>>   	struct cpu_map *cpus;

>> -	struct thread_map *threads;

>> -

>> -	threads = thread_map__new_str(target->pid, target->tid, target->uid,

>> -				      target->per_thread);

>>   

>>   	if (!threads)

>>   		return -1;

>> @@ -1130,6 +1128,27 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>>   	return -1;

>>   }

>>   

>> +int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)

>> +{

>> +	struct thread_map *threads;

>> +

>> +	threads = thread_map__new_str(target->pid, target->tid, target->uid,

>> +				      false);

>> +

>> +	return _perf_evlist__create_maps(evlist, target, threads);

>> +}

>> +

>> +int perf_evlist__create_stat_maps(struct perf_evlist *evlist,

>> +				  struct target *target)

>> +{

>> +	struct thread_map *threads;

>> +

>> +	threads = thread_map__new_str(target->pid, target->tid, target->uid,

>> +				      target->per_thread);

>> +

>> +	return _perf_evlist__create_maps(evlist, target, threads);

>> +}

>> +

>>   void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,

>>   			   struct thread_map *threads)

>>   {

>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h

>> index 75160666d305..80c654990e19 100644

>> --- a/tools/perf/util/evlist.h

>> +++ b/tools/perf/util/evlist.h

>> @@ -188,6 +188,8 @@ void perf_evlist__set_selected(struct perf_evlist *evlist,

>>   void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,

>>   			   struct thread_map *threads);

>>   int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target);

>> +int perf_evlist__create_stat_maps(struct perf_evlist *evlist,

>> +				  struct target *target);

>>   int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel);

>>   

>>   void __perf_evlist__set_leader(struct list_head *list);

>> -- 

>> 2.7.4
diff mbox series

Patch

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 98bf9d32f222..82d16426b984 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2833,7 +2833,7 @@  int cmd_stat(int argc, const char **argv)
 	if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
 		target.per_thread = true;
 
-	if (perf_evlist__create_maps(evsel_list, &target) < 0) {
+	if (perf_evlist__create_stat_maps(evsel_list, &target) < 0) {
 		if (target__has_task(&target)) {
 			pr_err("Problems finding threads of monitor\n");
 			parse_options_usage(stat_usage, stat_options, "p", 1);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index f0a5e09c4071..a4ae684f28de 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1100,13 +1100,11 @@  int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages)
 	return perf_evlist__mmap_ex(evlist, pages, 0, false);
 }
 
-int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
+static int _perf_evlist__create_maps(struct perf_evlist *evlist,
+				     struct target *target,
+				     struct thread_map *threads)
 {
 	struct cpu_map *cpus;
-	struct thread_map *threads;
-
-	threads = thread_map__new_str(target->pid, target->tid, target->uid,
-				      target->per_thread);
 
 	if (!threads)
 		return -1;
@@ -1130,6 +1128,27 @@  int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
 	return -1;
 }
 
+int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
+{
+	struct thread_map *threads;
+
+	threads = thread_map__new_str(target->pid, target->tid, target->uid,
+				      false);
+
+	return _perf_evlist__create_maps(evlist, target, threads);
+}
+
+int perf_evlist__create_stat_maps(struct perf_evlist *evlist,
+				  struct target *target)
+{
+	struct thread_map *threads;
+
+	threads = thread_map__new_str(target->pid, target->tid, target->uid,
+				      target->per_thread);
+
+	return _perf_evlist__create_maps(evlist, target, threads);
+}
+
 void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,
 			   struct thread_map *threads)
 {
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 75160666d305..80c654990e19 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -188,6 +188,8 @@  void perf_evlist__set_selected(struct perf_evlist *evlist,
 void perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus,
 			   struct thread_map *threads);
 int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target);
+int perf_evlist__create_stat_maps(struct perf_evlist *evlist,
+				  struct target *target);
 int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel);
 
 void __perf_evlist__set_leader(struct list_head *list);