| Message ID | 20210412091006.468557-5-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf arm-spe: Enable timestamp | expand |
On 12/04/2021 12:10, Leo Yan wrote: > In current code, it assigns the arch timer counter to the synthesized > samples Arm SPE trace, thus the samples don't contain the kernel time > but only contain the raw counter value. > > To fix the issue, this patch converts the timer counter to kernel time > and assigns it to sample timestamp. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/util/arm-spe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c > index 23714cf0380e..c13a89f06ab8 100644 > --- a/tools/perf/util/arm-spe.c > +++ b/tools/perf/util/arm-spe.c > @@ -234,7 +234,7 @@ static void arm_spe_prep_sample(struct arm_spe *spe, > struct arm_spe_record *record = &speq->decoder->record; > > if (!spe->timeless_decoding) > - sample->time = speq->timestamp; > + sample->time = tsc_to_perf_time(record->timestamp, &spe->tc); I noticed that in arm_spe_recording_options() the TIME sample bit is set regardless of any options. I don't know of a way to remove this, and if there isn't, does that mean that all the code in this file that looks at spe->timeless_decoding is untested and has never been hit? Unless there is a way to get a perf file with only the AUXTRACE event and no others? I think that one might have no timestamp set. Otherwise other events will always have timestamps so spe->timeless_decoding is always false. > > sample->ip = record->from_ip; > sample->cpumode = arm_spe_cpumode(spe, sample->ip); >
On Thu, Apr 15, 2021 at 05:46:31PM +0300, James Clark wrote: > > > On 12/04/2021 12:10, Leo Yan wrote: > > In current code, it assigns the arch timer counter to the synthesized > > samples Arm SPE trace, thus the samples don't contain the kernel time > > but only contain the raw counter value. > > > > To fix the issue, this patch converts the timer counter to kernel time > > and assigns it to sample timestamp. > > > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > --- > > tools/perf/util/arm-spe.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c > > index 23714cf0380e..c13a89f06ab8 100644 > > --- a/tools/perf/util/arm-spe.c > > +++ b/tools/perf/util/arm-spe.c > > @@ -234,7 +234,7 @@ static void arm_spe_prep_sample(struct arm_spe *spe, > > struct arm_spe_record *record = &speq->decoder->record; > > > > if (!spe->timeless_decoding) > > - sample->time = speq->timestamp; > > + sample->time = tsc_to_perf_time(record->timestamp, &spe->tc); > > > I noticed that in arm_spe_recording_options() the TIME sample bit is set regardless of any options. > I don't know of a way to remove this, and if there isn't, does that mean that all the code in this > file that looks at spe->timeless_decoding is untested and has never been hit? > > Unless there is a way to get a perf file with only the AUXTRACE event and no others? I think that one > might have no timestamp set. Otherwise other events will always have timestamps so spe->timeless_decoding > is always false. Good point. To be honest, I never noticed this issue until you mentioned this. We should fix for the "timeless" flow; and it's questionable for the function arm_spe_recording_options(), except for setting PERF_SAMPLE_TIME, it also hard codes for setting PERF_SAMPLE_CPU and PERF_SAMPLE_TID. Might need to carefully go through this function. Thanks, Leo
On 15/04/2021 18:23, Leo Yan wrote: > On Thu, Apr 15, 2021 at 05:46:31PM +0300, James Clark wrote: >> >> >> On 12/04/2021 12:10, Leo Yan wrote: >>> In current code, it assigns the arch timer counter to the synthesized >>> samples Arm SPE trace, thus the samples don't contain the kernel time >>> but only contain the raw counter value. >>> >>> To fix the issue, this patch converts the timer counter to kernel time >>> and assigns it to sample timestamp. >>> >>> Signed-off-by: Leo Yan <leo.yan@linaro.org> >>> --- >>> tools/perf/util/arm-spe.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c >>> index 23714cf0380e..c13a89f06ab8 100644 >>> --- a/tools/perf/util/arm-spe.c >>> +++ b/tools/perf/util/arm-spe.c >>> @@ -234,7 +234,7 @@ static void arm_spe_prep_sample(struct arm_spe *spe, >>> struct arm_spe_record *record = &speq->decoder->record; >>> >>> if (!spe->timeless_decoding) >>> - sample->time = speq->timestamp; >>> + sample->time = tsc_to_perf_time(record->timestamp, &spe->tc); >> >> >> I noticed that in arm_spe_recording_options() the TIME sample bit is set regardless of any options. >> I don't know of a way to remove this, and if there isn't, does that mean that all the code in this >> file that looks at spe->timeless_decoding is untested and has never been hit? >> >> Unless there is a way to get a perf file with only the AUXTRACE event and no others? I think that one >> might have no timestamp set. Otherwise other events will always have timestamps so spe->timeless_decoding >> is always false. > > Good point. To be honest, I never noticed this issue until you > mentioned this. > > We should fix for the "timeless" flow; and it's questionable for the > function arm_spe_recording_options(), except for setting > PERF_SAMPLE_TIME, it also hard codes for setting > PERF_SAMPLE_CPU and PERF_SAMPLE_TID. Might need to carefully go > through this function. > Yeah, it's not strictly related to your change, which is definitely an improvement. But maybe we should have a look at the SPE implementation relating to timestamps as a whole. > Thanks, > Leo >
On Fri, Apr 16, 2021 at 03:51:25PM +0300, James Clark wrote: [...] > >> I noticed that in arm_spe_recording_options() the TIME sample bit is set regardless of any options. > >> I don't know of a way to remove this, and if there isn't, does that mean that all the code in this > >> file that looks at spe->timeless_decoding is untested and has never been hit? > >> > >> Unless there is a way to get a perf file with only the AUXTRACE event and no others? I think that one > >> might have no timestamp set. Otherwise other events will always have timestamps so spe->timeless_decoding > >> is always false. > > > > Good point. To be honest, I never noticed this issue until you > > mentioned this. > > > > We should fix for the "timeless" flow; and it's questionable for the > > function arm_spe_recording_options(), except for setting > > PERF_SAMPLE_TIME, it also hard codes for setting > > PERF_SAMPLE_CPU and PERF_SAMPLE_TID. Might need to carefully go > > through this function. > > > > Yeah, it's not strictly related to your change, which is definitely an improvement. > But maybe we should have a look at the SPE implementation relating to timestamps as a whole. Totally agree, at least this patch series should not introduce any barrier for timeless case. I will go back to verify it; if you'd like to fix timeless issue, please feel free to go ahead. Thanks, Leo
Hi James, On Fri, Apr 16, 2021 at 03:51:25PM +0300, James Clark wrote: [...] > >>> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c > >>> index 23714cf0380e..c13a89f06ab8 100644 > >>> --- a/tools/perf/util/arm-spe.c > >>> +++ b/tools/perf/util/arm-spe.c > >>> @@ -234,7 +234,7 @@ static void arm_spe_prep_sample(struct arm_spe *spe, > >>> struct arm_spe_record *record = &speq->decoder->record; > >>> > >>> if (!spe->timeless_decoding) > >>> - sample->time = speq->timestamp; > >>> + sample->time = tsc_to_perf_time(record->timestamp, &spe->tc); > >> > >> > >> I noticed that in arm_spe_recording_options() the TIME sample bit is set regardless of any options. > >> I don't know of a way to remove this, and if there isn't, does that mean that all the code in this > >> file that looks at spe->timeless_decoding is untested and has never been hit? > >> > >> Unless there is a way to get a perf file with only the AUXTRACE event and no others? I think that one > >> might have no timestamp set. Otherwise other events will always have timestamps so spe->timeless_decoding > >> is always false. > > > > Good point. To be honest, I never noticed this issue until you > > mentioned this. > > > > We should fix for the "timeless" flow; and it's questionable for the > > function arm_spe_recording_options(), except for setting > > PERF_SAMPLE_TIME, it also hard codes for setting > > PERF_SAMPLE_CPU and PERF_SAMPLE_TID. Might need to carefully go > > through this function. > > > > Yeah, it's not strictly related to your change, which is definitely an improvement. > But maybe we should have a look at the SPE implementation relating to timestamps as a whole. Just now I sent another patch series "perf arm-spe: Correct recording configurations", which is the following up for the issue you found at here. After correcting sample flags, and combining with current patch series "perf arm-spe: Enable timestamp", I verified the Arm SPE decoding flows for "timeless" decoding, which can work as expected. So I think we can move forward for this patch series, for easier review, I have uploaded my testing branch wiht the complete patches [1]. Could you help confirm if this works for you? Thanks! Leo [1] https://github.com/Leo-Yan/linux/tree/perf_arm_spe_timestamp_v4_with_correcting_sample_flags
diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c index 23714cf0380e..c13a89f06ab8 100644 --- a/tools/perf/util/arm-spe.c +++ b/tools/perf/util/arm-spe.c @@ -234,7 +234,7 @@ static void arm_spe_prep_sample(struct arm_spe *spe, struct arm_spe_record *record = &speq->decoder->record; if (!spe->timeless_decoding) - sample->time = speq->timestamp; + sample->time = tsc_to_perf_time(record->timestamp, &spe->tc); sample->ip = record->from_ip; sample->cpumode = arm_spe_cpumode(spe, sample->ip);
In current code, it assigns the arch timer counter to the synthesized samples Arm SPE trace, thus the samples don't contain the kernel time but only contain the raw counter value. To fix the issue, this patch converts the timer counter to kernel time and assigns it to sample timestamp. Signed-off-by: Leo Yan <leo.yan@linaro.org> --- tools/perf/util/arm-spe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)