Currently, cs-etm passes the tid value for both tid and pid parameters when calling machine__set_current_tid(), this can lead to confusion for thread handling. E.g. we arbitrarily pass the same value for pid and tid, perf tool will be misled to consider it is a main thread (see thread__main_thread()).
On the other hand, Perf tool only can retrieve tid from Arm CoreSight context packet, and we have no chance to know pid (it maps to kernel's task_struct::tgid) from hardware tracing data. For this reason, this patch passes -1 as pid for function machine__set_current_tid().
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f323adb1af85..eed1a5930072 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1118,7 +1118,7 @@ int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq, if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) return err;
- err = machine__set_current_tid(etm->machine, cpu, tid, tid); + err = machine__set_current_tid(etm->machine, cpu, -1, tid); if (err) return err;
Good morning Leo,
On Sat, Nov 13, 2021 at 10:35:40PM +0800, Leo Yan wrote:
Currently, cs-etm passes the tid value for both tid and pid parameters when calling machine__set_current_tid(), this can lead to confusion for thread handling. E.g. we arbitrarily pass the same value for pid and tid, perf tool will be misled to consider it is a main thread (see thread__main_thread()).
On the other hand, Perf tool only can retrieve tid from Arm CoreSight context packet, and we have no chance to know pid (it maps to kernel's task_struct::tgid) from hardware tracing data. For this reason, this patch passes -1 as pid for function machine__set_current_tid().
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f323adb1af85..eed1a5930072 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1118,7 +1118,7 @@ int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq, if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) return err;
- err = machine__set_current_tid(etm->machine, cpu, tid, tid);
- err = machine__set_current_tid(etm->machine, cpu, -1, tid);
I remember wondering about what to do with the pid parameter when I wrote this patch...
Do you have a before-and-after snapshot you can add to the changelog? I also think it will require a "Fixes" tag. In your next revision please CC James since you guys are working in that area nowadays.
Thanks, Mathieu
if (err) return err; -- 2.25.1
Hi Mathieu,
On Thu, Nov 18, 2021 at 10:14:12AM -0700, Mathieu Poirier wrote:
Good morning Leo,
On Sat, Nov 13, 2021 at 10:35:40PM +0800, Leo Yan wrote:
Currently, cs-etm passes the tid value for both tid and pid parameters when calling machine__set_current_tid(), this can lead to confusion for thread handling. E.g. we arbitrarily pass the same value for pid and tid, perf tool will be misled to consider it is a main thread (see thread__main_thread()).
On the other hand, Perf tool only can retrieve tid from Arm CoreSight context packet, and we have no chance to know pid (it maps to kernel's task_struct::tgid) from hardware tracing data. For this reason, this patch passes -1 as pid for function machine__set_current_tid().
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f323adb1af85..eed1a5930072 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1118,7 +1118,7 @@ int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq, if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) return err;
- err = machine__set_current_tid(etm->machine, cpu, tid, tid);
- err = machine__set_current_tid(etm->machine, cpu, -1, tid);
I remember wondering about what to do with the pid parameter when I wrote this patch...
Do you have a before-and-after snapshot you can add to the changelog?
I tried to capture log but I didn't observe the difference introduced by this patch, this might because I didn't per-process mode for multi-threading case. I will try more case for this.
I also think it will require a "Fixes" tag. In your next revision please CC James since you guys are working in that area nowadays.
Will do. And will Cc James and German in next spin.
Thanks for review and suggestion.
Leo
On Thu, Nov 18, 2021 at 10:14:12AM -0700, Mathieu Poirier wrote:
Good morning Leo,
On Sat, Nov 13, 2021 at 10:35:40PM +0800, Leo Yan wrote:
Currently, cs-etm passes the tid value for both tid and pid parameters when calling machine__set_current_tid(), this can lead to confusion for thread handling. E.g. we arbitrarily pass the same value for pid and tid, perf tool will be misled to consider it is a main thread (see thread__main_thread()).
On the other hand, Perf tool only can retrieve tid from Arm CoreSight context packet, and we have no chance to know pid (it maps to kernel's task_struct::tgid) from hardware tracing data. For this reason, this patch passes -1 as pid for function machine__set_current_tid().
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f323adb1af85..eed1a5930072 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -1118,7 +1118,7 @@ int cs_etm__etmq_set_tid(struct cs_etm_queue *etmq, if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) return err;
- err = machine__set_current_tid(etm->machine, cpu, tid, tid);
- err = machine__set_current_tid(etm->machine, cpu, -1, tid);
I remember wondering about what to do with the pid parameter when I wrote this patch...
Some updates after I digged into the pid parameter for machine__set_current_tid().
During the recording phase, the perf tool will capture events PERF_RECORD_COMM and PERF_RECORD_FORK; these events contain pid/tid for profiled program. Below is an example for RECORD_FORK/RECORD_COMM events in perf data file:
0x89f0 [0x40]: event: 7 . . ... raw event: size 64 bytes . 0000: 07 00 00 00 00 20 40 00 59 6d 00 00 59 6d 00 00 ..... @.Ym..Ym.. . 0010: 5a 6d 00 00 59 6d 00 00 00 00 00 00 00 00 00 00 Zm..Ym.......... . 0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ . 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0 0 0x89f0 [0x40]: PERF_RECORD_FORK(27993:27994):(27993:27993)
0x8a30 [0x38]: event: 3 . . ... raw event: size 56 bytes . 0000: 03 00 00 00 00 00 38 00 59 6d 00 00 5a 6d 00 00 ......8.Ym..Zm.. . 0010: 6d 61 69 6e 00 00 00 00 00 00 00 00 00 00 00 00 main............ . 0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ . 0030: 00 00 00 00 00 00 00 00 ........
0 0 0x8a30 [0x38]: PERF_RECORD_COMM: main:27993/27994
In the reporting phase, perf tool will setup threads structure based on the RECORD_FORK and RECORD_COMM events. This means perf tool will set the pid/tid for every thread, e.g. in up case, it allocates thread context for 'main' program, and its one child thread is setup to thread->pid_ as '27993' and thread->tid as '27994'.
Afterwards, when perf tool decodes CoreSight trace data and handles context packet, at the end, machine__update_thread_pid() is invoked for updating thread's pid:
machine__update_thread_pid(struct machine *machine, struct thread *th, pid_t pid) { if (pid == th->pid_ || pid == -1 || th->pid_ != -1) return;
... }
Whatever we pass the pid parameter as tid or '-1' from the caller function machine__set_current_tid(), it doesn't change anything for the thread context. Since th->pid_ has been initialized and its value is not '-1', no matter what's the pid value is passed via argument, machine__update_thread_pid() will directly bail out. This is why before we pass 'tid' value rather than '-1' for pid, it doesn't cause any error.
For this reason, this patch doesn't improve anything. After discussed with Mathieu offline, I decided to drop this change. So update the info in case someone is interested in the relevant info.
Thanks, Leo
-
Leo Yan -
Mathieu Poirier