Re: [PATCH] perf util: Fix evlist->threads when working with 'perf stat --per-thread'

[Mathieu Poirier]

On 12 January 2018 at 12:53, Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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