linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it
@ 2021-04-12  8:34 Leo Yan
  2021-04-12  8:34 ` [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible Leo Yan
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Leo Yan @ 2021-04-12  8:34 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel
  Cc: 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible
  2021-04-12  8:34 [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
@ 2021-04-12  8:34 ` Leo Yan
  2021-04-26  5:55   ` Adrian Hunter
  2021-04-12  8:34 ` [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV Leo Yan
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Leo Yan @ 2021-04-12  8:34 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV
  2021-04-12  8:34 [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
  2021-04-12  8:34 ` [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible Leo Yan
@ 2021-04-12  8:34 ` Leo Yan
  2021-04-26  5:40   ` Adrian Hunter
  2021-04-12  8:34 ` [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event Leo Yan
  2021-04-26  1:41 ` [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
  3 siblings, 1 reply; 11+ messages in thread
From: Leo Yan @ 2021-04-12  8:34 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event
  2021-04-12  8:34 [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
  2021-04-12  8:34 ` [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible Leo Yan
  2021-04-12  8:34 ` [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV Leo Yan
@ 2021-04-12  8:34 ` Leo Yan
  2021-04-26  6:10   ` Adrian Hunter
  2021-04-26  1:41 ` [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
  3 siblings, 1 reply; 11+ messages in thread
From: Leo Yan @ 2021-04-12  8:34 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it
  2021-04-12  8:34 [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
                   ` (2 preceding siblings ...)
  2021-04-12  8:34 ` [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event Leo Yan
@ 2021-04-26  1:41 ` Leo Yan
  3 siblings, 0 replies; 11+ messages in thread
From: Leo Yan @ 2021-04-26  1:41 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Adrian Hunter, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV
  2021-04-12  8:34 ` [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV Leo Yan
@ 2021-04-26  5:40   ` Adrian Hunter
  2021-04-26  6:44     ` Leo Yan
  0 siblings, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2021-04-26  5:40 UTC (permalink / raw)
  To: Leo Yan, Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible
  2021-04-12  8:34 ` [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible Leo Yan
@ 2021-04-26  5:55   ` Adrian Hunter
  0 siblings, 0 replies; 11+ messages in thread
From: Adrian Hunter @ 2021-04-26  5:55 UTC (permalink / raw)
  To: Leo Yan, Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event
  2021-04-12  8:34 ` [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event Leo Yan
@ 2021-04-26  6:10   ` Adrian Hunter
  0 siblings, 0 replies; 11+ messages in thread
From: Adrian Hunter @ 2021-04-26  6:10 UTC (permalink / raw)
  To: Leo Yan, Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV
  2021-04-26  5:40   ` Adrian Hunter
@ 2021-04-26  6:44     ` Leo Yan
  2021-04-26  7:26       ` Adrian Hunter
  0 siblings, 1 reply; 11+ messages in thread
From: Leo Yan @ 2021-04-26  6:44 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV
  2021-04-26  6:44     ` Leo Yan
@ 2021-04-26  7:26       ` Adrian Hunter
  2021-04-26  7:39         ` Leo Yan
  0 siblings, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2021-04-26  7:26 UTC (permalink / raw)
  To: Leo Yan
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV
  2021-04-26  7:26       ` Adrian Hunter
@ 2021-04-26  7:39         ` Leo Yan
  0 siblings, 0 replies; 11+ messages in thread
From: Leo Yan @ 2021-04-26  7:39 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers, Gustavo A. R. Silva, Steve MacLean,
	Yonatan Goldschmidt, Kan Liang, linux-kernel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-04-26  7:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12  8:34 [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan
2021-04-12  8:34 ` [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible Leo Yan
2021-04-26  5:55   ` Adrian Hunter
2021-04-12  8:34 ` [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV Leo Yan
2021-04-26  5:40   ` Adrian Hunter
2021-04-26  6:44     ` Leo Yan
2021-04-26  7:26       ` Adrian Hunter
2021-04-26  7:39         ` Leo Yan
2021-04-12  8:34 ` [PATCH v1 3/3] perf session: Dump PERF_RECORD_TIME_CONV event Leo Yan
2021-04-26  6:10   ` Adrian Hunter
2021-04-26  1:41 ` [PATCH v1 0/3] perf: Allow TIME_CONV to be backwards-compatible and dump it Leo Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).