The event PERF_RECORD_TIME_CONV was extended for clock parameters, but
the tool fails to be backwards-compatible for the old event format.
Based on checking the event size, this patch series can decide if the
extended clock parameters are contained in the perf event or not. This
allows the event PERF_RECORD_TIME_CONV to be backwards-compatible.
The last patch also is introduced for dumping the event, for both the
old and latest event formats.
The patch set has been tested on Arm64 HiSilicon D06 platform.
Leo Yan (3):
perf jit: Let convert_timestamp() to be backwards-compatible
perf session: Add swap operation for event TIME_CONV
perf session: Dump PERF_RECORD_TIME_CONV event
tools/perf/util/jitdump.c | 31 +++++++++++++++++++++----------
tools/perf/util/session.c | 35 +++++++++++++++++++++++++++++++++--
tools/perf/util/tsc.c | 31 +++++++++++++++++++++++++++++++
tools/perf/util/tsc.h | 4 ++++
4 files changed, 89 insertions(+), 12 deletions(-)
--
2.25.1
Since commit d110162cafc8 ("perf tsc: Support cap_user_time_short for
event TIME_CONV"), the event PERF_RECORD_TIME_CONV has extended the data
structure for clock parameters.
To be backwards-compatible, this patch adds a dedicated swap operation
for the event PERF_RECORD_TIME_CONV, based on checking the event size,
it can support both for the old and new event formats.
Fixes: d110162cafc8 ("perf tsc: Support cap_user_time_short for event TIME_CONV")
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/util/session.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 9a8808507bd9..afca3d5fc851 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -949,6 +949,26 @@ static void perf_event__stat_round_swap(union perf_event *event,
event->stat_round.time = bswap_64(event->stat_round.time);
}
+static void perf_event__time_conv_swap(union perf_event *event,
+ bool sample_id_all __maybe_unused)
+{
+ size_t time_zero_size;
+
+ event->time_conv.time_shift = bswap_64(event->time_conv.time_shift);
+ event->time_conv.time_mult = bswap_64(event->time_conv.time_mult);
+ event->time_conv.time_zero = bswap_64(event->time_conv.time_zero);
+
+ time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event;
+ if (event->header.size > time_zero_size) {
+ event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles);
+ event->time_conv.time_mask = bswap_64(event->time_conv.time_mask);
+ event->time_conv.cap_user_time_zero =
+ bswap_32(event->time_conv.cap_user_time_zero);
+ event->time_conv.cap_user_time_short =
+ bswap_32(event->time_conv.cap_user_time_short);
+ }
+}
+
typedef void (*perf_event__swap_op)(union perf_event *event,
bool sample_id_all);
@@ -985,7 +1005,7 @@ static perf_event__swap_op perf_event__swap_ops[] = {
[PERF_RECORD_STAT] = perf_event__stat_swap,
[PERF_RECORD_STAT_ROUND] = perf_event__stat_round_swap,
[PERF_RECORD_EVENT_UPDATE] = perf_event__event_update_swap,
- [PERF_RECORD_TIME_CONV] = perf_event__all64_swap,
+ [PERF_RECORD_TIME_CONV] = perf_event__time_conv_swap,
[PERF_RECORD_HEADER_MAX] = NULL,
};
--
2.25.1
Commit d110162cafc8 ("perf tsc: Support cap_user_time_short for event
TIME_CONV") supports the extended parameters for event TIME_CONV, but it
broke the backwards compatibility, so any perf data file with old event
format fails to convert timestamp.
For the backwards-compatibility, this patch checks the event size, if
the event size confirms the extended parameters are supported in the
event TIME_CONV, then copies these parameters.
Fixes: d110162cafc8 ("perf tsc: Support cap_user_time_short for event TIME_CONV")
Signed-off-by: Leo Yan <[email protected]>
---
tools/perf/util/jitdump.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 9760d8e7b386..67b514c38a43 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -396,21 +396,32 @@ static pid_t jr_entry_tid(struct jit_buf_desc *jd, union jr_entry *jr)
static uint64_t convert_timestamp(struct jit_buf_desc *jd, uint64_t timestamp)
{
- struct perf_tsc_conversion tc;
+ struct perf_tsc_conversion tc = { 0 };
+ struct perf_record_time_conv *time_conv = &jd->session->time_conv;
if (!jd->use_arch_timestamp)
return timestamp;
- tc.time_shift = jd->session->time_conv.time_shift;
- tc.time_mult = jd->session->time_conv.time_mult;
- tc.time_zero = jd->session->time_conv.time_zero;
- tc.time_cycles = jd->session->time_conv.time_cycles;
- tc.time_mask = jd->session->time_conv.time_mask;
- tc.cap_user_time_zero = jd->session->time_conv.cap_user_time_zero;
- tc.cap_user_time_short = jd->session->time_conv.cap_user_time_short;
+ tc.time_shift = time_conv->time_shift;
+ tc.time_mult = time_conv->time_mult;
+ tc.time_zero = time_conv->time_zero;
- if (!tc.cap_user_time_zero)
- return 0;
+ /*
+ * The event TIME_CONV was extended for the fields from "time_cycles"
+ * when supported cap_user_time_short, for backward compatibility,
+ * checks the event size and assigns these extended fields if these
+ * fields are contained in the event.
+ */
+ if (time_conv->header.size >
+ ((void *)&time_conv->time_cycles - (void *)time_conv)) {
+ tc.time_cycles = time_conv->time_cycles;
+ tc.time_mask = time_conv->time_mask;
+ tc.cap_user_time_zero = time_conv->cap_user_time_zero;
+ tc.cap_user_time_short = time_conv->cap_user_time_short;
+
+ if (!tc.cap_user_time_zero)
+ return 0;
+ }
return tsc_to_perf_time(timestamp, &tc);
}
--
2.25.1
On Mon, Apr 12, 2021 at 04:34:56PM +0800, Leo Yan wrote:
> The event PERF_RECORD_TIME_CONV was extended for clock parameters, but
> the tool fails to be backwards-compatible for the old event format.
Gentle ping ...
Adrian, could you take a look for this patch series?
Thanks,
Leo
On 12/04/21 11:34 am, Leo Yan wrote:
> Since commit d110162cafc8 ("perf tsc: Support cap_user_time_short for
> event TIME_CONV"), the event PERF_RECORD_TIME_CONV has extended the data
> structure for clock parameters.
>
> To be backwards-compatible, this patch adds a dedicated swap operation
> for the event PERF_RECORD_TIME_CONV, based on checking the event size,
> it can support both for the old and new event formats.
>
> Fixes: d110162cafc8 ("perf tsc: Support cap_user_time_short for event TIME_CONV")
> Signed-off-by: Leo Yan <[email protected]>
> ---
> tools/perf/util/session.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 9a8808507bd9..afca3d5fc851 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -949,6 +949,26 @@ static void perf_event__stat_round_swap(union perf_event *event,
> event->stat_round.time = bswap_64(event->stat_round.time);
> }
>
> +static void perf_event__time_conv_swap(union perf_event *event,
> + bool sample_id_all __maybe_unused)
> +{
> + size_t time_zero_size;
> +
> + event->time_conv.time_shift = bswap_64(event->time_conv.time_shift);
> + event->time_conv.time_mult = bswap_64(event->time_conv.time_mult);
> + event->time_conv.time_zero = bswap_64(event->time_conv.time_zero);
> +
> + time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event;
> + if (event->header.size > time_zero_size) {
I wonder if we could have a helper for this e.g. (untested)
#define event_contains(obj, mem) (obj.header.size > offsetof(typeof(obj), mem))
if (event_contains(event->time_conv, time_cycles)) {
> + event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles);
> + event->time_conv.time_mask = bswap_64(event->time_conv.time_mask);
> + event->time_conv.cap_user_time_zero =
> + bswap_32(event->time_conv.cap_user_time_zero);
> + event->time_conv.cap_user_time_short =
> + bswap_32(event->time_conv.cap_user_time_short);
'struct perf_record_time_conv' contains bool, the sizeof which, AFAIK, is not defined.
Is it really 4 bytes on your implementation? It is only 1 byte with gcc on x86.
Either way, you should change 'struct perf_record_time_conv' so it uses a type of known size.
Since you are the only one using it, it should match your implementation.
> + }
> +}
> +
> typedef void (*perf_event__swap_op)(union perf_event *event,
> bool sample_id_all);
>
> @@ -985,7 +1005,7 @@ static perf_event__swap_op perf_event__swap_ops[] = {
> [PERF_RECORD_STAT] = perf_event__stat_swap,
> [PERF_RECORD_STAT_ROUND] = perf_event__stat_round_swap,
> [PERF_RECORD_EVENT_UPDATE] = perf_event__event_update_swap,
> - [PERF_RECORD_TIME_CONV] = perf_event__all64_swap,
> + [PERF_RECORD_TIME_CONV] = perf_event__time_conv_swap,
> [PERF_RECORD_HEADER_MAX] = NULL,
> };
>
>
On 12/04/21 11:34 am, Leo Yan wrote:
> Commit d110162cafc8 ("perf tsc: Support cap_user_time_short for event
> TIME_CONV") supports the extended parameters for event TIME_CONV, but it
> broke the backwards compatibility, so any perf data file with old event
> format fails to convert timestamp.
>
> For the backwards-compatibility, this patch checks the event size, if
> the event size confirms the extended parameters are supported in the
> event TIME_CONV, then copies these parameters.
>
> Fixes: d110162cafc8 ("perf tsc: Support cap_user_time_short for event TIME_CONV")
> Signed-off-by: Leo Yan <[email protected]>
Perhaps could make use of a helper like the one described in my comments for patch 2.
Nevertheless:
Acked-by: Adrian Hunter <[email protected]>
> ---
> tools/perf/util/jitdump.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 9760d8e7b386..67b514c38a43 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -396,21 +396,32 @@ static pid_t jr_entry_tid(struct jit_buf_desc *jd, union jr_entry *jr)
>
> static uint64_t convert_timestamp(struct jit_buf_desc *jd, uint64_t timestamp)
> {
> - struct perf_tsc_conversion tc;
> + struct perf_tsc_conversion tc = { 0 };
> + struct perf_record_time_conv *time_conv = &jd->session->time_conv;
>
> if (!jd->use_arch_timestamp)
> return timestamp;
>
> - tc.time_shift = jd->session->time_conv.time_shift;
> - tc.time_mult = jd->session->time_conv.time_mult;
> - tc.time_zero = jd->session->time_conv.time_zero;
> - tc.time_cycles = jd->session->time_conv.time_cycles;
> - tc.time_mask = jd->session->time_conv.time_mask;
> - tc.cap_user_time_zero = jd->session->time_conv.cap_user_time_zero;
> - tc.cap_user_time_short = jd->session->time_conv.cap_user_time_short;
> + tc.time_shift = time_conv->time_shift;
> + tc.time_mult = time_conv->time_mult;
> + tc.time_zero = time_conv->time_zero;
>
> - if (!tc.cap_user_time_zero)
> - return 0;
> + /*
> + * The event TIME_CONV was extended for the fields from "time_cycles"
> + * when supported cap_user_time_short, for backward compatibility,
> + * checks the event size and assigns these extended fields if these
> + * fields are contained in the event.
> + */
> + if (time_conv->header.size >
> + ((void *)&time_conv->time_cycles - (void *)time_conv)) {
> + tc.time_cycles = time_conv->time_cycles;
> + tc.time_mask = time_conv->time_mask;
> + tc.cap_user_time_zero = time_conv->cap_user_time_zero;
> + tc.cap_user_time_short = time_conv->cap_user_time_short;
> +
> + if (!tc.cap_user_time_zero)
> + return 0;
> + }
>
> return tsc_to_perf_time(timestamp, &tc);
> }
>
Hi Adrian,
On Mon, Apr 26, 2021 at 08:40:00AM +0300, Adrian Hunter wrote:
[...]
> > +static void perf_event__time_conv_swap(union perf_event *event,
> > + bool sample_id_all __maybe_unused)
> > +{
> > + size_t time_zero_size;
> > +
> > + event->time_conv.time_shift = bswap_64(event->time_conv.time_shift);
> > + event->time_conv.time_mult = bswap_64(event->time_conv.time_mult);
> > + event->time_conv.time_zero = bswap_64(event->time_conv.time_zero);
> > +
> > + time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event;
> > + if (event->header.size > time_zero_size) {
>
> I wonder if we could have a helper for this e.g. (untested)
>
> #define event_contains(obj, mem) (obj.header.size > offsetof(typeof(obj), mem))
>
> if (event_contains(event->time_conv, time_cycles)) {
Yeah, this is a better implementation. Will refine patch for this.
> > + event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles);
> > + event->time_conv.time_mask = bswap_64(event->time_conv.time_mask);
> > + event->time_conv.cap_user_time_zero =
> > + bswap_32(event->time_conv.cap_user_time_zero);
> > + event->time_conv.cap_user_time_short =
> > + bswap_32(event->time_conv.cap_user_time_short);
>
> 'struct perf_record_time_conv' contains bool, the sizeof which, AFAIK, is not defined.
> Is it really 4 bytes on your implementation? It is only 1 byte with gcc on x86.
On Arm64, bool is also 1 byte with GCC.
When working on this patch, I checked the size for structure
perf_record_time_conv, it gave out the structure size is 56; I wrongly
thought the bool size is 4 bytes and there have no padding. In fact,
bool size is 1 byte and GCC pads 6 bytes at the end of structure.
> Either way, you should change 'struct perf_record_time_conv' so it uses a type of known size.
> Since you are the only one using it, it should match your implementation.
Will change to "unsigned int" value. Thank you for reviewing and
suggestions.
Leo
> > + }
> > +}
> > +
> > typedef void (*perf_event__swap_op)(union perf_event *event,
> > bool sample_id_all);
> >
> > @@ -985,7 +1005,7 @@ static perf_event__swap_op perf_event__swap_ops[] = {
> > [PERF_RECORD_STAT] = perf_event__stat_swap,
> > [PERF_RECORD_STAT_ROUND] = perf_event__stat_round_swap,
> > [PERF_RECORD_EVENT_UPDATE] = perf_event__event_update_swap,
> > - [PERF_RECORD_TIME_CONV] = perf_event__all64_swap,
> > + [PERF_RECORD_TIME_CONV] = perf_event__time_conv_swap,
> > [PERF_RECORD_HEADER_MAX] = NULL,
> > };
> >
> >
>
On 26/04/21 9:44 am, Leo Yan wrote:
> Hi Adrian,
>
> On Mon, Apr 26, 2021 at 08:40:00AM +0300, Adrian Hunter wrote:
>
> [...]
>
>>> +static void perf_event__time_conv_swap(union perf_event *event,
>>> + bool sample_id_all __maybe_unused)
>>> +{
>>> + size_t time_zero_size;
>>> +
>>> + event->time_conv.time_shift = bswap_64(event->time_conv.time_shift);
>>> + event->time_conv.time_mult = bswap_64(event->time_conv.time_mult);
>>> + event->time_conv.time_zero = bswap_64(event->time_conv.time_zero);
>>> +
>>> + time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event;
>>> + if (event->header.size > time_zero_size) {
>>
>> I wonder if we could have a helper for this e.g. (untested)
>>
>> #define event_contains(obj, mem) (obj.header.size > offsetof(typeof(obj), mem))
>>
>> if (event_contains(event->time_conv, time_cycles)) {
>
> Yeah, this is a better implementation. Will refine patch for this.
>
>>> + event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles);
>>> + event->time_conv.time_mask = bswap_64(event->time_conv.time_mask);
>>> + event->time_conv.cap_user_time_zero =
>>> + bswap_32(event->time_conv.cap_user_time_zero);
>>> + event->time_conv.cap_user_time_short =
>>> + bswap_32(event->time_conv.cap_user_time_short);
>>
>> 'struct perf_record_time_conv' contains bool, the sizeof which, AFAIK, is not defined.
>> Is it really 4 bytes on your implementation? It is only 1 byte with gcc on x86.
>
> On Arm64, bool is also 1 byte with GCC.
>
> When working on this patch, I checked the size for structure
> perf_record_time_conv, it gave out the structure size is 56; I wrongly
> thought the bool size is 4 bytes and there have no padding. In fact,
> bool size is 1 byte and GCC pads 6 bytes at the end of structure.
>
>> Either way, you should change 'struct perf_record_time_conv' so it uses a type of known size.
>> Since you are the only one using it, it should match your implementation.
>
> Will change to "unsigned int" value.
If it is 1 byte, I'd have thought __u8 i.e.
struct perf_record_time_conv {
struct perf_event_header header;
__u64 time_shift;
__u64 time_mult;
__u64 time_zero;
__u64 time_cycles;
__u64 time_mask;
__u8 cap_user_time_zero;
__u8 cap_user_time_short;
__u8 unused[6];
};
On Mon, Apr 26, 2021 at 10:26:12AM +0300, Adrian Hunter wrote:
> On 26/04/21 9:44 am, Leo Yan wrote:
> > Hi Adrian,
> >
> > On Mon, Apr 26, 2021 at 08:40:00AM +0300, Adrian Hunter wrote:
> >
> > [...]
> >
> >>> +static void perf_event__time_conv_swap(union perf_event *event,
> >>> + bool sample_id_all __maybe_unused)
> >>> +{
> >>> + size_t time_zero_size;
> >>> +
> >>> + event->time_conv.time_shift = bswap_64(event->time_conv.time_shift);
> >>> + event->time_conv.time_mult = bswap_64(event->time_conv.time_mult);
> >>> + event->time_conv.time_zero = bswap_64(event->time_conv.time_zero);
> >>> +
> >>> + time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event;
> >>> + if (event->header.size > time_zero_size) {
> >>
> >> I wonder if we could have a helper for this e.g. (untested)
> >>
> >> #define event_contains(obj, mem) (obj.header.size > offsetof(typeof(obj), mem))
> >>
> >> if (event_contains(event->time_conv, time_cycles)) {
> >
> > Yeah, this is a better implementation. Will refine patch for this.
> >
> >>> + event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles);
> >>> + event->time_conv.time_mask = bswap_64(event->time_conv.time_mask);
> >>> + event->time_conv.cap_user_time_zero =
> >>> + bswap_32(event->time_conv.cap_user_time_zero);
> >>> + event->time_conv.cap_user_time_short =
> >>> + bswap_32(event->time_conv.cap_user_time_short);
> >>
> >> 'struct perf_record_time_conv' contains bool, the sizeof which, AFAIK, is not defined.
> >> Is it really 4 bytes on your implementation? It is only 1 byte with gcc on x86.
> >
> > On Arm64, bool is also 1 byte with GCC.
> >
> > When working on this patch, I checked the size for structure
> > perf_record_time_conv, it gave out the structure size is 56; I wrongly
> > thought the bool size is 4 bytes and there have no padding. In fact,
> > bool size is 1 byte and GCC pads 6 bytes at the end of structure.
> >
> >> Either way, you should change 'struct perf_record_time_conv' so it uses a type of known size.
> >> Since you are the only one using it, it should match your implementation.
> >
> > Will change to "unsigned int" value.
>
> If it is 1 byte, I'd have thought __u8 i.e.
>
> struct perf_record_time_conv {
> struct perf_event_header header;
> __u64 time_shift;
> __u64 time_mult;
> __u64 time_zero;
> __u64 time_cycles;
> __u64 time_mask;
> __u8 cap_user_time_zero;
> __u8 cap_user_time_short;
> __u8 unused[6];
> };
>
Ah, yes, thanks a lot for reminding.