[PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it

[PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it

[Leo Yan]

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

[PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible

[Leo Yan]

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 <leo.yan@linaro.org>
---
 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

Re: [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible

[Adrian Hunter]

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 <leo.yan@linaro.org>

Perhaps could make use of a helper like the one described in my comments for patch 2.
Nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  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);
>  }
> 

[PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV

[Leo Yan]

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 <leo.yan@linaro.org>
---
 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

Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV

[Adrian Hunter]

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 <leo.yan@linaro.org>
> ---
>  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,
>  };
>  
> 

Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV

[Leo Yan]

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,
> >  };
> >  
> > 
> 

Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV

[Adrian Hunter]

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];
};

Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV

[Leo Yan]

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.

[PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event

[Leo Yan]

Now perf tool uses the common stub function process_event_op2_stub() for
dumping TIME_CONV event, thus it doesn't output the clock parameters
contained in the event.

This patch adds the callback function for dumping the hardware clock
parameters in TIME_CONV event.

Before:

  # perf report -D

  0x978 [0x38]: event: 79
  .
  . ... raw event: size 56 bytes
  .  0000:  4f 00 00 00 00 00 38 00 15 00 00 00 00 00 00 00  O.....8.........
  .  0010:  00 00 40 01 00 00 00 00 86 89 0b bf df ff ff ff  ..@........<BF><DF><FF><FF><FF>
  .  0020:  d1 c1 b2 39 03 00 00 00 ff ff ff ff ff ff ff 00  <D1><C1><B2>9....<FF><FF><FF><FF><FF><FF><FF>.
  .  0030:  01 01 00 00 00 00 00 00                          ........

  0 0 0x978 [0x38]: PERF_RECORD_TIME_CONV
  : unhandled!

  [...]

After:

  # perf report -D

  0x978 [0x38]: event: 79
  .
  . ... raw event: size 56 bytes
  .  0000:  4f 00 00 00 00 00 38 00 15 00 00 00 00 00 00 00  O.....8.........
  .  0010:  00 00 40 01 00 00 00 00 86 89 0b bf df ff ff ff  ..@........<BF><DF><FF><FF><FF>
  .  0020:  d1 c1 b2 39 03 00 00 00 ff ff ff ff ff ff ff 00  <D1><C1><B2>9....<FF><FF><FF><FF><FF><FF><FF>.
  .  0030:  01 01 00 00 00 00 00 00                          ........

  0 0 0x978 [0x38]: PERF_RECORD_TIME_CONV
  ... Time Shift      21
  ... Time Muliplier  20971520
  ... Time Zero       18446743935180835206
  ... Time Cycles     13852918225
  ... Time Mask       0xffffffffffffff
  ... Cap Time Zero   1
  ... Cap Time Short  1
  : unhandled!

  [...]

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/session.c | 13 ++++++++++++-
 tools/perf/util/tsc.c     | 31 +++++++++++++++++++++++++++++++
 tools/perf/util/tsc.h     |  4 ++++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index afca3d5fc851..19a0b2bc5f33 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -29,6 +29,7 @@
 #include "thread-stack.h"
 #include "sample-raw.h"
 #include "stat.h"
+#include "tsc.h"
 #include "ui/progress.h"
 #include "../perf.h"
 #include "arch/common.h"
@@ -451,6 +452,16 @@ static int process_stat_round_stub(struct perf_session *perf_session __maybe_unu
 	return 0;
 }
 
+static int process_event_time_conv_stub(struct perf_session *perf_session __maybe_unused,
+					union perf_event *event)
+{
+	if (dump_trace)
+		perf_event__fprintf_time_conv(event, stdout);
+
+	dump_printf(": unhandled!\n");
+	return 0;
+}
+
 static int perf_session__process_compressed_event_stub(struct perf_session *session __maybe_unused,
 						       union perf_event *event __maybe_unused,
 						       u64 file_offset __maybe_unused)
@@ -532,7 +543,7 @@ void perf_tool__fill_defaults(struct perf_tool *tool)
 	if (tool->stat_round == NULL)
 		tool->stat_round = process_stat_round_stub;
 	if (tool->time_conv == NULL)
-		tool->time_conv = process_event_op2_stub;
+		tool->time_conv = process_event_time_conv_stub;
 	if (tool->feature == NULL)
 		tool->feature = process_event_op2_stub;
 	if (tool->compressed == NULL)
diff --git a/tools/perf/util/tsc.c b/tools/perf/util/tsc.c
index 62b4c75c966c..e2a2c63e5189 100644
--- a/tools/perf/util/tsc.c
+++ b/tools/perf/util/tsc.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <errno.h>
+#include <inttypes.h>
+#include <string.h>
 
 #include <linux/compiler.h>
 #include <linux/perf_event.h>
@@ -110,3 +112,32 @@ u64 __weak rdtsc(void)
 {
 	return 0;
 }
+
+size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp)
+{
+	struct perf_record_time_conv *tc = (struct perf_record_time_conv *)event;
+	size_t ret;
+
+	ret  = fprintf(fp, "\n... Time Shift      %" PRI_lu64 "\n", tc->time_shift);
+	ret += fprintf(fp, "... Time Muliplier  %" PRI_lu64 "\n", tc->time_mult);
+	ret += fprintf(fp, "... Time Zero       %" PRI_lu64 "\n", tc->time_zero);
+
+	/*
+	 * 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 prints these extended fields if these
+	 * fields are contained in the perf data file.
+	 */
+	if (tc->header.size > ((void *)&tc->time_cycles - (void *)tc)) {
+		ret += fprintf(fp, "... Time Cycles     %" PRI_lu64 "\n",
+			       tc->time_cycles);
+		ret += fprintf(fp, "... Time Mask       %#" PRI_lx64 "\n",
+			       tc->time_mask);
+		ret += fprintf(fp, "... Cap Time Zero   %" PRId32 "\n",
+			       tc->cap_user_time_zero);
+		ret += fprintf(fp, "... Cap Time Short  %" PRId32 "\n",
+			       tc->cap_user_time_short);
+	}
+
+	return ret;
+}
diff --git a/tools/perf/util/tsc.h b/tools/perf/util/tsc.h
index 72a15419f3b3..7d83a31732a7 100644
--- a/tools/perf/util/tsc.h
+++ b/tools/perf/util/tsc.h
@@ -4,6 +4,8 @@
 
 #include <linux/types.h>
 
+#include "event.h"
+
 struct perf_tsc_conversion {
 	u16 time_shift;
 	u32 time_mult;
@@ -24,4 +26,6 @@ u64 perf_time_to_tsc(u64 ns, struct perf_tsc_conversion *tc);
 u64 tsc_to_perf_time(u64 cyc, struct perf_tsc_conversion *tc);
 u64 rdtsc(void);
 
+size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp);
+
 #endif // __PERF_TSC_H
-- 
2.25.1

Re: [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event

[Adrian Hunter]

On 12/04/21 11:34 am, Leo Yan wrote:
> Now perf tool uses the common stub function process_event_op2_stub() for
> dumping TIME_CONV event, thus it doesn't output the clock parameters
> contained in the event.
> 
> This patch adds the callback function for dumping the hardware clock
> parameters in TIME_CONV event.
> 
> Before:
> 
>   # perf report -D
> 
>   0x978 [0x38]: event: 79
>   .
>   . ... raw event: size 56 bytes
>   .  0000:  4f 00 00 00 00 00 38 00 15 00 00 00 00 00 00 00  O.....8.........
>   .  0010:  00 00 40 01 00 00 00 00 86 89 0b bf df ff ff ff  ..@........<BF><DF><FF><FF><FF>
>   .  0020:  d1 c1 b2 39 03 00 00 00 ff ff ff ff ff ff ff 00  <D1><C1><B2>9....<FF><FF><FF><FF><FF><FF><FF>.
>   .  0030:  01 01 00 00 00 00 00 00                          ........
> 
>   0 0 0x978 [0x38]: PERF_RECORD_TIME_CONV
>   : unhandled!
> 
>   [...]
> 
> After:
> 
>   # perf report -D
> 
>   0x978 [0x38]: event: 79
>   .
>   . ... raw event: size 56 bytes
>   .  0000:  4f 00 00 00 00 00 38 00 15 00 00 00 00 00 00 00  O.....8.........
>   .  0010:  00 00 40 01 00 00 00 00 86 89 0b bf df ff ff ff  ..@........<BF><DF><FF><FF><FF>
>   .  0020:  d1 c1 b2 39 03 00 00 00 ff ff ff ff ff ff ff 00  <D1><C1><B2>9....<FF><FF><FF><FF><FF><FF><FF>.
>   .  0030:  01 01 00 00 00 00 00 00                          ........
> 
>   0 0 0x978 [0x38]: PERF_RECORD_TIME_CONV
>   ... Time Shift      21
>   ... Time Muliplier  20971520
>   ... Time Zero       18446743935180835206
>   ... Time Cycles     13852918225
>   ... Time Mask       0xffffffffffffff
>   ... Cap Time Zero   1
>   ... Cap Time Short  1
>   : unhandled!
> 
>   [...]
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Perhaps could make use of a helper like the one described in my comments for patch 2.
Nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  tools/perf/util/session.c | 13 ++++++++++++-
>  tools/perf/util/tsc.c     | 31 +++++++++++++++++++++++++++++++
>  tools/perf/util/tsc.h     |  4 ++++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index afca3d5fc851..19a0b2bc5f33 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -29,6 +29,7 @@
>  #include "thread-stack.h"
>  #include "sample-raw.h"
>  #include "stat.h"
> +#include "tsc.h"
>  #include "ui/progress.h"
>  #include "../perf.h"
>  #include "arch/common.h"
> @@ -451,6 +452,16 @@ static int process_stat_round_stub(struct perf_session *perf_session __maybe_unu
>  	return 0;
>  }
>  
> +static int process_event_time_conv_stub(struct perf_session *perf_session __maybe_unused,
> +					union perf_event *event)
> +{
> +	if (dump_trace)
> +		perf_event__fprintf_time_conv(event, stdout);
> +
> +	dump_printf(": unhandled!\n");
> +	return 0;
> +}
> +
>  static int perf_session__process_compressed_event_stub(struct perf_session *session __maybe_unused,
>  						       union perf_event *event __maybe_unused,
>  						       u64 file_offset __maybe_unused)
> @@ -532,7 +543,7 @@ void perf_tool__fill_defaults(struct perf_tool *tool)
>  	if (tool->stat_round == NULL)
>  		tool->stat_round = process_stat_round_stub;
>  	if (tool->time_conv == NULL)
> -		tool->time_conv = process_event_op2_stub;
> +		tool->time_conv = process_event_time_conv_stub;
>  	if (tool->feature == NULL)
>  		tool->feature = process_event_op2_stub;
>  	if (tool->compressed == NULL)
> diff --git a/tools/perf/util/tsc.c b/tools/perf/util/tsc.c
> index 62b4c75c966c..e2a2c63e5189 100644
> --- a/tools/perf/util/tsc.c
> +++ b/tools/perf/util/tsc.c
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <errno.h>
> +#include <inttypes.h>
> +#include <string.h>
>  
>  #include <linux/compiler.h>
>  #include <linux/perf_event.h>
> @@ -110,3 +112,32 @@ u64 __weak rdtsc(void)
>  {
>  	return 0;
>  }
> +
> +size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp)
> +{
> +	struct perf_record_time_conv *tc = (struct perf_record_time_conv *)event;
> +	size_t ret;
> +
> +	ret  = fprintf(fp, "\n... Time Shift      %" PRI_lu64 "\n", tc->time_shift);
> +	ret += fprintf(fp, "... Time Muliplier  %" PRI_lu64 "\n", tc->time_mult);
> +	ret += fprintf(fp, "... Time Zero       %" PRI_lu64 "\n", tc->time_zero);
> +
> +	/*
> +	 * 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 prints these extended fields if these
> +	 * fields are contained in the perf data file.
> +	 */
> +	if (tc->header.size > ((void *)&tc->time_cycles - (void *)tc)) {
> +		ret += fprintf(fp, "... Time Cycles     %" PRI_lu64 "\n",
> +			       tc->time_cycles);
> +		ret += fprintf(fp, "... Time Mask       %#" PRI_lx64 "\n",
> +			       tc->time_mask);
> +		ret += fprintf(fp, "... Cap Time Zero   %" PRId32 "\n",
> +			       tc->cap_user_time_zero);
> +		ret += fprintf(fp, "... Cap Time Short  %" PRId32 "\n",
> +			       tc->cap_user_time_short);
> +	}
> +
> +	return ret;
> +}
> diff --git a/tools/perf/util/tsc.h b/tools/perf/util/tsc.h
> index 72a15419f3b3..7d83a31732a7 100644
> --- a/tools/perf/util/tsc.h
> +++ b/tools/perf/util/tsc.h
> @@ -4,6 +4,8 @@
>  
>  #include <linux/types.h>
>  
> +#include "event.h"
> +
>  struct perf_tsc_conversion {
>  	u16 time_shift;
>  	u32 time_mult;
> @@ -24,4 +26,6 @@ u64 perf_time_to_tsc(u64 ns, struct perf_tsc_conversion *tc);
>  u64 tsc_to_perf_time(u64 cyc, struct perf_tsc_conversion *tc);
>  u64 rdtsc(void);
>  
> +size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp);
> +
>  #endif // __PERF_TSC_H
> 

Re: [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it

[Leo Yan]

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