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 <[email protected]>
---
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;
--
2.25.1
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > ---
> > 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