* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-22 22:15 [PATCH] perf util: Use target->per_thread and target->system_wide flags Jin Yao
@ 2018-01-22 21:10 ` Mathieu Poirier
2018-01-22 23:02 ` Jin, Yao
2018-01-22 23:56 ` Mathieu Poirier
1 sibling, 1 reply; 8+ messages in thread
From: Mathieu Poirier @ 2018-01-22 21:10 UTC (permalink / raw)
To: Jin Yao
Cc: Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, Linux-kernel, Andi Kleen, kan.liang, yao.jin
On 22 January 2018 at 15:15, Jin Yao <yao.jin@linux.intel.com> wrote:
> Mathieu Poirier reports issue in commit ("73c0ca1eee3d perf thread_map:
> Enumerate all threads from /proc") that it has negative impact on
> 'perf record --per-thread'. It has the effect of creating a kernel event
> for each thread in the system for 'perf record --per-thread'.
>
> Mathieu Poirier's patch ("perf util: Do not reuse target->per_thread flag")
> can fix this issue by creating a new target->all_threads flag.
>
> This patch is based on Mathieu Poirier's patch but it doesn't use a new
> target->all_threads flag. This patch just uses 'target->per_thread &&
> target->system_wide' as a condition to check for all threads case.
>
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
> tools/perf/util/evlist.c | 2 +-
> tools/perf/util/thread_map.c | 4 ++--
> tools/perf/util/thread_map.h | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 120efd8..9dff74a 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1106,7 +1106,7 @@ 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,
> - target->per_thread);
> + target->per_thread && target->system_wide);
At first glance I thought your solution would do the trick but perf
record does use target->system_wide when the '-a' switch is used.
Moreover specifying the '-a' switch doesn't prevent the '--per-thread'
option from being used as well, making both target->perf_thread and
target_system_wide equal to true (and that is not good).
Although not a fan of adding more to struct target, the advantage of
having target->all_threads is that we are guaranteed that it isn't
used anywhere else.
Let me know what you think,
Mathieu
>
> if (!threads)
> return -1;
> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
> index 3e1038f..729dad8 100644
> --- a/tools/perf/util/thread_map.c
> +++ b/tools/perf/util/thread_map.c
> @@ -323,7 +323,7 @@ struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
> }
>
> struct thread_map *thread_map__new_str(const char *pid, const char *tid,
> - uid_t uid, bool per_thread)
> + uid_t uid, bool all_threads)
> {
> if (pid)
> return thread_map__new_by_pid_str(pid);
> @@ -331,7 +331,7 @@ struct thread_map *thread_map__new_str(const char *pid, const char *tid,
> if (!tid && uid != UINT_MAX)
> return thread_map__new_by_uid(uid);
>
> - if (per_thread)
> + if (all_threads)
> return thread_map__new_all_cpus();
>
> return thread_map__new_by_tid_str(tid);
> diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
> index 0a806b9..5ec91cf 100644
> --- a/tools/perf/util/thread_map.h
> +++ b/tools/perf/util/thread_map.h
> @@ -31,7 +31,7 @@ struct thread_map *thread_map__get(struct thread_map *map);
> void thread_map__put(struct thread_map *map);
>
> struct thread_map *thread_map__new_str(const char *pid,
> - const char *tid, uid_t uid, bool per_thread);
> + const char *tid, uid_t uid, bool all_threads);
>
> struct thread_map *thread_map__new_by_tid_str(const char *tid_str);
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] perf util: Use target->per_thread and target->system_wide flags
@ 2018-01-22 22:15 Jin Yao
2018-01-22 21:10 ` Mathieu Poirier
2018-01-22 23:56 ` Mathieu Poirier
0 siblings, 2 replies; 8+ messages in thread
From: Jin Yao @ 2018-01-22 22:15 UTC (permalink / raw)
To: mathieu.poirier, acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
Mathieu Poirier reports issue in commit ("73c0ca1eee3d perf thread_map:
Enumerate all threads from /proc") that it has negative impact on
'perf record --per-thread'. It has the effect of creating a kernel event
for each thread in the system for 'perf record --per-thread'.
Mathieu Poirier's patch ("perf util: Do not reuse target->per_thread flag")
can fix this issue by creating a new target->all_threads flag.
This patch is based on Mathieu Poirier's patch but it doesn't use a new
target->all_threads flag. This patch just uses 'target->per_thread &&
target->system_wide' as a condition to check for all threads case.
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/util/evlist.c | 2 +-
tools/perf/util/thread_map.c | 4 ++--
tools/perf/util/thread_map.h | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 120efd8..9dff74a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1106,7 +1106,7 @@ 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,
- target->per_thread);
+ target->per_thread && target->system_wide);
if (!threads)
return -1;
diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index 3e1038f..729dad8 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -323,7 +323,7 @@ struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
}
struct thread_map *thread_map__new_str(const char *pid, const char *tid,
- uid_t uid, bool per_thread)
+ uid_t uid, bool all_threads)
{
if (pid)
return thread_map__new_by_pid_str(pid);
@@ -331,7 +331,7 @@ struct thread_map *thread_map__new_str(const char *pid, const char *tid,
if (!tid && uid != UINT_MAX)
return thread_map__new_by_uid(uid);
- if (per_thread)
+ if (all_threads)
return thread_map__new_all_cpus();
return thread_map__new_by_tid_str(tid);
diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
index 0a806b9..5ec91cf 100644
--- a/tools/perf/util/thread_map.h
+++ b/tools/perf/util/thread_map.h
@@ -31,7 +31,7 @@ struct thread_map *thread_map__get(struct thread_map *map);
void thread_map__put(struct thread_map *map);
struct thread_map *thread_map__new_str(const char *pid,
- const char *tid, uid_t uid, bool per_thread);
+ const char *tid, uid_t uid, bool all_threads);
struct thread_map *thread_map__new_by_tid_str(const char *tid_str);
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-22 21:10 ` Mathieu Poirier
@ 2018-01-22 23:02 ` Jin, Yao
2018-01-22 23:55 ` Mathieu Poirier
2018-01-23 14:40 ` Jiri Olsa
0 siblings, 2 replies; 8+ messages in thread
From: Jin, Yao @ 2018-01-22 23:02 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, Linux-kernel, Andi Kleen, kan.liang, yao.jin
On 1/23/2018 5:10 AM, Mathieu Poirier wrote:
> On 22 January 2018 at 15:15, Jin Yao <yao.jin@linux.intel.com> wrote:
>> Mathieu Poirier reports issue in commit ("73c0ca1eee3d perf thread_map:
>> Enumerate all threads from /proc") that it has negative impact on
>> 'perf record --per-thread'. It has the effect of creating a kernel event
>> for each thread in the system for 'perf record --per-thread'.
>>
>> Mathieu Poirier's patch ("perf util: Do not reuse target->per_thread flag")
>> can fix this issue by creating a new target->all_threads flag.
>>
>> This patch is based on Mathieu Poirier's patch but it doesn't use a new
>> target->all_threads flag. This patch just uses 'target->per_thread &&
>> target->system_wide' as a condition to check for all threads case.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>> tools/perf/util/evlist.c | 2 +-
>> tools/perf/util/thread_map.c | 4 ++--
>> tools/perf/util/thread_map.h | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 120efd8..9dff74a 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1106,7 +1106,7 @@ 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,
>> - target->per_thread);
>> + target->per_thread && target->system_wide);
>
> At first glance I thought your solution would do the trick but perf
> record does use target->system_wide when the '-a' switch is used.
> Moreover specifying the '-a' switch doesn't prevent the '--per-thread'
> option from being used as well, making both target->perf_thread and
> target_system_wide equal to true (and that is not good).
>
> Although not a fan of adding more to struct target, the advantage of
> having target->all_threads is that we are guaranteed that it isn't
> used anywhere else.
>
> Let me know what you think,
> Mathieu
>
If we specify both '-a' and '--per-thread' to perf record, perf record
will override'--per-thread'. So now target->per_thread = false, and
target->system_wide = true.
If we specify '--per-thread' only to perf record, target->per_thread =
true, and target->system_wide = false.
So whatever for any case, target->per_thread && target->system_wide is
false.
Since the parameter is false, in thread_map__new_str(), it will not
execute the thread_map__new_all_cpus(). So that will not change perf
record previous behavior.
In perf stat, it allows the case that target->per_thread and
target->system_wide are all true. That means we want to collect
system-wide per-thread metrics.
That's my current thinking.
Thanks
Jin Yao
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-22 23:02 ` Jin, Yao
@ 2018-01-22 23:55 ` Mathieu Poirier
2018-01-23 14:40 ` Jiri Olsa
1 sibling, 0 replies; 8+ messages in thread
From: Mathieu Poirier @ 2018-01-22 23:55 UTC (permalink / raw)
To: Jin, Yao
Cc: Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, Linux-kernel, Andi Kleen, kan.liang, yao.jin
On 22 January 2018 at 16:02, Jin, Yao <yao.jin@linux.intel.com> wrote:
>
>
> On 1/23/2018 5:10 AM, Mathieu Poirier wrote:
>>
>> On 22 January 2018 at 15:15, Jin Yao <yao.jin@linux.intel.com> wrote:
>>>
>>> Mathieu Poirier reports issue in commit ("73c0ca1eee3d perf thread_map:
>>> Enumerate all threads from /proc") that it has negative impact on
>>> 'perf record --per-thread'. It has the effect of creating a kernel event
>>> for each thread in the system for 'perf record --per-thread'.
>>>
>>> Mathieu Poirier's patch ("perf util: Do not reuse target->per_thread
>>> flag")
>>> can fix this issue by creating a new target->all_threads flag.
>>>
>>> This patch is based on Mathieu Poirier's patch but it doesn't use a new
>>> target->all_threads flag. This patch just uses 'target->per_thread &&
>>> target->system_wide' as a condition to check for all threads case.
>>>
>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>> ---
>>> tools/perf/util/evlist.c | 2 +-
>>> tools/perf/util/thread_map.c | 4 ++--
>>> tools/perf/util/thread_map.h | 2 +-
>>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>> index 120efd8..9dff74a 100644
>>> --- a/tools/perf/util/evlist.c
>>> +++ b/tools/perf/util/evlist.c
>>> @@ -1106,7 +1106,7 @@ 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,
>>> - target->per_thread);
>>> + target->per_thread &&
>>> target->system_wide);
>>
>>
>> At first glance I thought your solution would do the trick but perf
>> record does use target->system_wide when the '-a' switch is used.
>> Moreover specifying the '-a' switch doesn't prevent the '--per-thread'
>> option from being used as well, making both target->perf_thread and
>> target_system_wide equal to true (and that is not good).
>>
>> Although not a fan of adding more to struct target, the advantage of
>> having target->all_threads is that we are guaranteed that it isn't
>> used anywhere else.
>>
>> Let me know what you think,
>> Mathieu
>>
>
> If we specify both '-a' and '--per-thread' to perf record, perf record will
> override'--per-thread'. So now target->per_thread = false, and
> target->system_wide = true.
Just spent time looking at it further and target__validate() will make
sure per thread and cpu wide options are mutually exclusive. I like
your approach better so let's go with that.
Thanks,
Mathieu
>
> If we specify '--per-thread' only to perf record, target->per_thread = true,
> and target->system_wide = false.
>
> So whatever for any case, target->per_thread && target->system_wide is
> false.
>
> Since the parameter is false, in thread_map__new_str(), it will not execute
> the thread_map__new_all_cpus(). So that will not change perf record previous
> behavior.
>
> In perf stat, it allows the case that target->per_thread and
> target->system_wide are all true. That means we want to collect system-wide
> per-thread metrics.
>
> That's my current thinking.
>
> Thanks
> Jin Yao
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-22 22:15 [PATCH] perf util: Use target->per_thread and target->system_wide flags Jin Yao
2018-01-22 21:10 ` Mathieu Poirier
@ 2018-01-22 23:56 ` Mathieu Poirier
2018-01-23 1:08 ` Jin, Yao
1 sibling, 1 reply; 8+ messages in thread
From: Mathieu Poirier @ 2018-01-22 23:56 UTC (permalink / raw)
To: Jin Yao
Cc: Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, Linux-kernel, Andi Kleen, kan.liang, yao.jin
On 22 January 2018 at 15:15, Jin Yao <yao.jin@linux.intel.com> wrote:
> Mathieu Poirier reports issue in commit ("73c0ca1eee3d perf thread_map:
> Enumerate all threads from /proc") that it has negative impact on
> 'perf record --per-thread'. It has the effect of creating a kernel event
> for each thread in the system for 'perf record --per-thread'.
>
> Mathieu Poirier's patch ("perf util: Do not reuse target->per_thread flag")
> can fix this issue by creating a new target->all_threads flag.
>
> This patch is based on Mathieu Poirier's patch but it doesn't use a new
> target->all_threads flag. This patch just uses 'target->per_thread &&
> target->system_wide' as a condition to check for all threads case.
>
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
> tools/perf/util/evlist.c | 2 +-
> tools/perf/util/thread_map.c | 4 ++--
> tools/perf/util/thread_map.h | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 120efd8..9dff74a 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1106,7 +1106,7 @@ 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,
> - target->per_thread);
> + target->per_thread && target->system_wide);
>
> if (!threads)
> return -1;
> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
> index 3e1038f..729dad8 100644
> --- a/tools/perf/util/thread_map.c
> +++ b/tools/perf/util/thread_map.c
> @@ -323,7 +323,7 @@ struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
> }
>
> struct thread_map *thread_map__new_str(const char *pid, const char *tid,
> - uid_t uid, bool per_thread)
> + uid_t uid, bool all_threads)
> {
> if (pid)
> return thread_map__new_by_pid_str(pid);
> @@ -331,7 +331,7 @@ struct thread_map *thread_map__new_str(const char *pid, const char *tid,
> if (!tid && uid != UINT_MAX)
> return thread_map__new_by_uid(uid);
>
> - if (per_thread)
> + if (all_threads)
> return thread_map__new_all_cpus();
>
> return thread_map__new_by_tid_str(tid);
> diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
> index 0a806b9..5ec91cf 100644
> --- a/tools/perf/util/thread_map.h
> +++ b/tools/perf/util/thread_map.h
> @@ -31,7 +31,7 @@ struct thread_map *thread_map__get(struct thread_map *map);
> void thread_map__put(struct thread_map *map);
>
> struct thread_map *thread_map__new_str(const char *pid,
> - const char *tid, uid_t uid, bool per_thread);
> + const char *tid, uid_t uid, bool all_threads);
>
> struct thread_map *thread_map__new_by_tid_str(const char *tid_str);
Reviewed-and-tested-by: Mathieu Poirier "mathieu.poirier@linaro.org"
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-22 23:56 ` Mathieu Poirier
@ 2018-01-23 1:08 ` Jin, Yao
0 siblings, 0 replies; 8+ messages in thread
From: Jin, Yao @ 2018-01-23 1:08 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, Linux-kernel, Andi Kleen, kan.liang, yao.jin
On 1/23/2018 7:56 AM, Mathieu Poirier wrote:
> On 22 January 2018 at 15:15, Jin Yao <yao.jin@linux.intel.com> wrote:
>> Mathieu Poirier reports issue in commit ("73c0ca1eee3d perf thread_map:
>> Enumerate all threads from /proc") that it has negative impact on
>> 'perf record --per-thread'. It has the effect of creating a kernel event
>> for each thread in the system for 'perf record --per-thread'.
>>
>> Mathieu Poirier's patch ("perf util: Do not reuse target->per_thread flag")
>> can fix this issue by creating a new target->all_threads flag.
>>
>> This patch is based on Mathieu Poirier's patch but it doesn't use a new
>> target->all_threads flag. This patch just uses 'target->per_thread &&
>> target->system_wide' as a condition to check for all threads case.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>> tools/perf/util/evlist.c | 2 +-
>> tools/perf/util/thread_map.c | 4 ++--
>> tools/perf/util/thread_map.h | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 120efd8..9dff74a 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1106,7 +1106,7 @@ 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,
>> - target->per_thread);
>> + target->per_thread && target->system_wide);
>>
>> if (!threads)
>> return -1;
>> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
>> index 3e1038f..729dad8 100644
>> --- a/tools/perf/util/thread_map.c
>> +++ b/tools/perf/util/thread_map.c
>> @@ -323,7 +323,7 @@ struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
>> }
>>
>> struct thread_map *thread_map__new_str(const char *pid, const char *tid,
>> - uid_t uid, bool per_thread)
>> + uid_t uid, bool all_threads)
>> {
>> if (pid)
>> return thread_map__new_by_pid_str(pid);
>> @@ -331,7 +331,7 @@ struct thread_map *thread_map__new_str(const char *pid, const char *tid,
>> if (!tid && uid != UINT_MAX)
>> return thread_map__new_by_uid(uid);
>>
>> - if (per_thread)
>> + if (all_threads)
>> return thread_map__new_all_cpus();
>>
>> return thread_map__new_by_tid_str(tid);
>> diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
>> index 0a806b9..5ec91cf 100644
>> --- a/tools/perf/util/thread_map.h
>> +++ b/tools/perf/util/thread_map.h
>> @@ -31,7 +31,7 @@ struct thread_map *thread_map__get(struct thread_map *map);
>> void thread_map__put(struct thread_map *map);
>>
>> struct thread_map *thread_map__new_str(const char *pid,
>> - const char *tid, uid_t uid, bool per_thread);
>> + const char *tid, uid_t uid, bool all_threads);
>>
>> struct thread_map *thread_map__new_by_tid_str(const char *tid_str);
>
> Reviewed-and-tested-by: Mathieu Poirier "mathieu.poirier@linaro.org"
>
Thanks a lot for reviewing the patch.
Thanks
Jin Yao
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-22 23:02 ` Jin, Yao
2018-01-22 23:55 ` Mathieu Poirier
@ 2018-01-23 14:40 ` Jiri Olsa
2018-01-24 0:12 ` Jin, Yao
1 sibling, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2018-01-23 14:40 UTC (permalink / raw)
To: Jin, Yao
Cc: Mathieu Poirier, Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra,
Ingo Molnar, Alexander Shishkin, Linux-kernel, Andi Kleen,
kan.liang, yao.jin
On Tue, Jan 23, 2018 at 07:02:44AM +0800, Jin, Yao wrote:
SNIP
> > > threads = thread_map__new_str(target->pid, target->tid, target->uid,
> > > - target->per_thread);
> > > + target->per_thread && target->system_wide);
> >
> > At first glance I thought your solution would do the trick but perf
> > record does use target->system_wide when the '-a' switch is used.
> > Moreover specifying the '-a' switch doesn't prevent the '--per-thread'
> > option from being used as well, making both target->perf_thread and
> > target_system_wide equal to true (and that is not good).
> >
> > Although not a fan of adding more to struct target, the advantage of
> > having target->all_threads is that we are guaranteed that it isn't
> > used anywhere else.
> >
> > Let me know what you think,
> > Mathieu
> >
>
> If we specify both '-a' and '--per-thread' to perf record, perf record will
> override'--per-thread'. So now target->per_thread = false, and
> target->system_wide = true.
>
> If we specify '--per-thread' only to perf record, target->per_thread = true,
> and target->system_wide = false.
>
> So whatever for any case, target->per_thread && target->system_wide is
> false.
>
> Since the parameter is false, in thread_map__new_str(), it will not execute
> the thread_map__new_all_cpus(). So that will not change perf record previous
> behavior.
>
> In perf stat, it allows the case that target->per_thread and
> target->system_wide are all true. That means we want to collect system-wide
> per-thread metrics.
could you please put this description into comment
before the thread_map__new_str is called?
thanks,
jirka
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf util: Use target->per_thread and target->system_wide flags
2018-01-23 14:40 ` Jiri Olsa
@ 2018-01-24 0:12 ` Jin, Yao
0 siblings, 0 replies; 8+ messages in thread
From: Jin, Yao @ 2018-01-24 0:12 UTC (permalink / raw)
To: Jiri Olsa
Cc: Mathieu Poirier, Arnaldo Carvalho de Melo, jolsa, Peter Zijlstra,
Ingo Molnar, Alexander Shishkin, Linux-kernel, Andi Kleen,
kan.liang, yao.jin
On 1/23/2018 10:40 PM, Jiri Olsa wrote:
> On Tue, Jan 23, 2018 at 07:02:44AM +0800, Jin, Yao wrote:
>
> SNIP
>
>>>> threads = thread_map__new_str(target->pid, target->tid, target->uid,
>>>> - target->per_thread);
>>>> + target->per_thread && target->system_wide);
>>>
>>> At first glance I thought your solution would do the trick but perf
>>> record does use target->system_wide when the '-a' switch is used.
>>> Moreover specifying the '-a' switch doesn't prevent the '--per-thread'
>>> option from being used as well, making both target->perf_thread and
>>> target_system_wide equal to true (and that is not good).
>>>
>>> Although not a fan of adding more to struct target, the advantage of
>>> having target->all_threads is that we are guaranteed that it isn't
>>> used anywhere else.
>>>
>>> Let me know what you think,
>>> Mathieu
>>>
>>
>> If we specify both '-a' and '--per-thread' to perf record, perf record will
>> override'--per-thread'. So now target->per_thread = false, and
>> target->system_wide = true.
>>
>> If we specify '--per-thread' only to perf record, target->per_thread = true,
>> and target->system_wide = false.
>>
>> So whatever for any case, target->per_thread && target->system_wide is
>> false.
>>
>> Since the parameter is false, in thread_map__new_str(), it will not execute
>> the thread_map__new_all_cpus(). So that will not change perf record previous
>> behavior.
>>
>> In perf stat, it allows the case that target->per_thread and
>> target->system_wide are all true. That means we want to collect system-wide
>> per-thread metrics.
>
> could you please put this description into comment
> before the thread_map__new_str is called?
>
> thanks,
> jirka
>
OK, I will put this comment in v2. I will send v2 soon.
Thanks
Jin Yao
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-01-24 0:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-22 22:15 [PATCH] perf util: Use target->per_thread and target->system_wide flags Jin Yao
2018-01-22 21:10 ` Mathieu Poirier
2018-01-22 23:02 ` Jin, Yao
2018-01-22 23:55 ` Mathieu Poirier
2018-01-23 14:40 ` Jiri Olsa
2018-01-24 0:12 ` Jin, Yao
2018-01-22 23:56 ` Mathieu Poirier
2018-01-23 1:08 ` Jin, Yao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).