[PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Chun-Tse Shao]

The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
asan runtime error:

  # Build with asan
  $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
  # Test success with many asan runtime errors:
  $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
   83: Zstd perf.data compression/decompression:
  ...
  util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
  0x7f69e3f99653: note: pointer points here
   d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
                ^
  util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
  0x7f69e3f99653: note: pointer points here
   d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
                ^
  ...

Since there is no way to align compressed data in zstd compression, this
patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
`data_size` to specify the actual compressed data size. The
`header.size` contains the total record size, including the padding at
the end to make it 8-byte aligned.

Tested with `Zstd perf.data compression/decompression`

Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
v2:
  Add deprecated comment for `PERF_RECORD_COMPRESSED`.

 tools/lib/perf/Documentation/libperf.txt      |  1 +
 tools/lib/perf/include/perf/event.h           | 12 ++++++++++
 .../Documentation/perf.data-file-format.txt   | 24 +++++++++++++++----
 tools/perf/builtin-record.c                   | 23 ++++++++++++++----
 tools/perf/util/event.c                       |  1 +
 tools/perf/util/session.c                     |  5 +++-
 tools/perf/util/tool.c                        | 11 +++++++--
 7 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/tools/lib/perf/Documentation/libperf.txt b/tools/lib/perf/Documentation/libperf.txt
index 59aabdd3cabf..4072bc9b7670 100644
--- a/tools/lib/perf/Documentation/libperf.txt
+++ b/tools/lib/perf/Documentation/libperf.txt
@@ -210,6 +210,7 @@ SYNOPSIS
   struct perf_record_time_conv;
   struct perf_record_header_feature;
   struct perf_record_compressed;
+  struct perf_record_compressed2;
 --

 DESCRIPTION
diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index 37bb7771d914..09b7c643ddac 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -457,6 +457,16 @@ struct perf_record_compressed {
 	char			 data[];
 };

+/*
+ * `header.size` includes the padding we are going to add while writing the record.
+ * `data_size` only includes the size of `data[]` itself.
+ */
+struct perf_record_compressed2 {
+	struct perf_event_header header;
+	__u64			 data_size;
+	char			 data[];
+};
+
 enum perf_user_event_type { /* above any possible kernel type */
 	PERF_RECORD_USER_TYPE_START		= 64,
 	PERF_RECORD_HEADER_ATTR			= 64,
@@ -478,6 +488,7 @@ enum perf_user_event_type { /* above any possible kernel type */
 	PERF_RECORD_HEADER_FEATURE		= 80,
 	PERF_RECORD_COMPRESSED			= 81,
 	PERF_RECORD_FINISHED_INIT		= 82,
+	PERF_RECORD_COMPRESSED2			= 83,
 	PERF_RECORD_HEADER_MAX
 };

@@ -518,6 +529,7 @@ union perf_event {
 	struct perf_record_time_conv		time_conv;
 	struct perf_record_header_feature	feat;
 	struct perf_record_compressed		pack;
+	struct perf_record_compressed2		pack2;
 };

 #endif /* __LIBPERF_EVENT_H */
diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
index 010a4edcd384..cd95ba09f727 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -370,7 +370,7 @@ struct {
 	u32	mmap_len;
 };

-Indicates that trace contains records of PERF_RECORD_COMPRESSED type
+Indicates that trace contains records of PERF_RECORD_COMPRESSED2 type
 that have perf_events records in compressed form.

 	HEADER_CPU_PMU_CAPS = 28,
@@ -602,7 +602,14 @@ struct auxtrace_error_event {
 Describes a header feature. These are records used in pipe-mode that
 contain information that otherwise would be in perf.data file's header.

-	PERF_RECORD_COMPRESSED 			= 81,
+	PERF_RECORD_COMPRESSED 			= 81, /* deprecated */
+
+The header is followed by compressed data frame that can be decompressed
+into array of perf trace records. The size of the entire compressed event
+record including the header is limited by the max value of header.size.
+
+It is deprecated and new files should use PERF_RECORD_COMPRESSED2 to gurantee
+8-byte alignment.

 struct compressed_event {
 	struct perf_event_header	header;
@@ -618,10 +625,17 @@ This is used, for instance, to 'perf inject' events after init and before
 regular events, those emitted by the kernel, to support combining guest and
 host records.

+	PERF_RECORD_COMPRESSED2			= 83,

-The header is followed by compressed data frame that can be decompressed
-into array of perf trace records. The size of the entire compressed event
-record including the header is limited by the max value of header.size.
+8-byte aligned version of `PERF_RECORD_COMPRESSED`. `header.size` indicates the
+total record size, including padding for 8-byte alignment, and `data_size`
+specifies the actual size of the compressed data.
+
+struct perf_record_compressed2 {
+	struct perf_event_header	header;
+	__u64				data_size;
+	char				data[];
+};

 Event types

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 9af3f21fd015..d07ad670daa7 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -648,14 +648,27 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size)
 	struct record *rec = to;

 	if (record__comp_enabled(rec)) {
+		struct perf_record_compressed2 *event = map->data;
+		size_t padding = 0;
+		u8 pad[8] = {0};
 		ssize_t compressed = zstd_compress(rec->session, map, map->data,
 						   mmap__mmap_len(map), bf, size);

 		if (compressed < 0)
 			return (int)compressed;

-		size = compressed;
-		bf   = map->data;
+		bf = event;
+		thread->samples++;
+
+		/*
+		 * The record from `zstd_compress` is not 8 bytes aligned, which would cause asan
+		 * error. We make it aligned here.
+		 */
+		event->data_size = compressed - sizeof(struct perf_record_compressed2);
+		event->header.size = PERF_ALIGN(compressed, sizeof(u64));
+		padding = event->header.size - compressed;
+		return record__write(rec, map, bf, compressed) ||
+		       record__write(rec, map, &pad, padding);
 	}

 	thread->samples++;
@@ -1534,7 +1547,7 @@ static void record__adjust_affinity(struct record *rec, struct mmap *map)

 static size_t process_comp_header(void *record, size_t increment)
 {
-	struct perf_record_compressed *event = record;
+	struct perf_record_compressed2 *event = record;
 	size_t size = sizeof(*event);

 	if (increment) {
@@ -1542,7 +1555,7 @@ static size_t process_comp_header(void *record, size_t increment)
 		return increment;
 	}

-	event->header.type = PERF_RECORD_COMPRESSED;
+	event->header.type = PERF_RECORD_COMPRESSED2;
 	event->header.size = size;

 	return size;
@@ -1552,7 +1565,7 @@ static ssize_t zstd_compress(struct perf_session *session, struct mmap *map,
 			    void *dst, size_t dst_size, void *src, size_t src_size)
 {
 	ssize_t compressed;
-	size_t max_record_size = PERF_SAMPLE_MAX_SIZE - sizeof(struct perf_record_compressed) - 1;
+	size_t max_record_size = PERF_SAMPLE_MAX_SIZE - sizeof(struct perf_record_compressed2) - 1;
 	struct zstd_data *zstd_data = &session->zstd_data;

 	if (map && map->file)
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index c23b77f8f854..80c9ea682413 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -77,6 +77,7 @@ static const char *perf_event__names[] = {
 	[PERF_RECORD_HEADER_FEATURE]		= "FEATURE",
 	[PERF_RECORD_COMPRESSED]		= "COMPRESSED",
 	[PERF_RECORD_FINISHED_INIT]		= "FINISHED_INIT",
+	[PERF_RECORD_COMPRESSED2]		= "COMPRESSED2",
 };

 const char *perf_event__name(unsigned int id)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 60fb9997ea0d..db2653322f9f 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1400,7 +1400,9 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 	int err;

 	perf_sample__init(&sample, /*all=*/true);
-	if (event->header.type != PERF_RECORD_COMPRESSED || perf_tool__compressed_is_stub(tool))
+	if ((event->header.type != PERF_RECORD_COMPRESSED &&
+	     event->header.type != PERF_RECORD_COMPRESSED2) ||
+	    perf_tool__compressed_is_stub(tool))
 		dump_event(session->evlist, event, file_offset, &sample, file_path);

 	/* These events are processed right away */
@@ -1481,6 +1483,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 		err = tool->feature(session, event);
 		break;
 	case PERF_RECORD_COMPRESSED:
+	case PERF_RECORD_COMPRESSED2:
 		err = tool->compressed(session, event, file_offset, file_path);
 		if (err)
 			dump_event(session->evlist, event, file_offset, &sample, file_path);
diff --git a/tools/perf/util/tool.c b/tools/perf/util/tool.c
index 3b7f390f26eb..37bd8ac63b01 100644
--- a/tools/perf/util/tool.c
+++ b/tools/perf/util/tool.c
@@ -43,8 +43,15 @@ static int perf_session__process_compressed_event(struct perf_session *session,
 		decomp->size = decomp_last_rem;
 	}

-	src = (void *)event + sizeof(struct perf_record_compressed);
-	src_size = event->pack.header.size - sizeof(struct perf_record_compressed);
+	if (event->header.type == PERF_RECORD_COMPRESSED) {
+		src = (void *)event + sizeof(struct perf_record_compressed);
+		src_size = event->pack.header.size - sizeof(struct perf_record_compressed);
+	} else if (event->header.type == PERF_RECORD_COMPRESSED2) {
+		src = (void *)event + sizeof(struct perf_record_compressed2);
+		src_size = event->pack2.data_size;
+	} else {
+		return -1;
+	}

 	decomp_size = zstd_decompress_stream(session->active_decomp->zstd_decomp, src, src_size,
 				&(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
--
2.48.1.711.g2feabab25a-goog

[PATCH v2 2/2] perf record: Fix a asan runtime error in util/maps.c

[Chun-Tse Shao]

If I build perf with asan and run Zstd test:

  $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
  $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
   83: Zstd perf.data compression/decompression:
  ...
  util/maps.c:1046:5: runtime error: null pointer passed as argument 2, which is declared to never be null
  ...

The issue was caused by `bsearch`. The patch adds a check to ensure
argument 2 and 3 are not NULL and 0.

Testing with the commands above confirms that the runtime error is
resolved.

Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/util/maps.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 09c9cc326c08..41a99e1f4b50 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -1042,10 +1042,13 @@ struct map *maps__find(struct maps *maps, u64 ip)
 	while (!done) {
 		down_read(maps__lock(maps));
 		if (maps__maps_by_address_sorted(maps)) {
-			struct map **mapp =
-				bsearch(&ip, maps__maps_by_address(maps), maps__nr_maps(maps),
-					sizeof(*mapp), map__addr_cmp);
+			struct map **mapp = NULL;
+			struct map **maps_by_address = maps__maps_by_address(maps);
+			unsigned int nr_maps = maps__nr_maps(maps);
 
+			if (maps_by_address && nr_maps)
+				mapp = bsearch(&ip, maps_by_address, nr_maps, sizeof(*mapp),
+					       map__addr_cmp);
 			if (mapp)
 				result = map__get(*mapp);
 			done = true;
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Namhyung Kim]

Hello,

On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> asan runtime error:
> 
>   # Build with asan
>   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
>   # Test success with many asan runtime errors:
>   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
>    83: Zstd perf.data compression/decompression:
>   ...
>   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
>   0x7f69e3f99653: note: pointer points here
>    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
>                 ^
>   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
>   0x7f69e3f99653: note: pointer points here
>    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
>                 ^
>   ...
> 
> Since there is no way to align compressed data in zstd compression, this
> patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> `data_size` to specify the actual compressed data size. The
> `header.size` contains the total record size, including the padding at
> the end to make it 8-byte aligned.
> 
> Tested with `Zstd perf.data compression/decompression`

Looks good to me.

Arnaldo, are you ok with adding a new record type for this?

Thanks,
Namhyung

> 
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> ---
> v2:
>   Add deprecated comment for `PERF_RECORD_COMPRESSED`.
> 
>  tools/lib/perf/Documentation/libperf.txt      |  1 +
>  tools/lib/perf/include/perf/event.h           | 12 ++++++++++
>  .../Documentation/perf.data-file-format.txt   | 24 +++++++++++++++----
>  tools/perf/builtin-record.c                   | 23 ++++++++++++++----
>  tools/perf/util/event.c                       |  1 +
>  tools/perf/util/session.c                     |  5 +++-
>  tools/perf/util/tool.c                        | 11 +++++++--
>  7 files changed, 64 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/lib/perf/Documentation/libperf.txt b/tools/lib/perf/Documentation/libperf.txt
> index 59aabdd3cabf..4072bc9b7670 100644
> --- a/tools/lib/perf/Documentation/libperf.txt
> +++ b/tools/lib/perf/Documentation/libperf.txt
> @@ -210,6 +210,7 @@ SYNOPSIS
>    struct perf_record_time_conv;
>    struct perf_record_header_feature;
>    struct perf_record_compressed;
> +  struct perf_record_compressed2;
>  --
> 
>  DESCRIPTION
> diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> index 37bb7771d914..09b7c643ddac 100644
> --- a/tools/lib/perf/include/perf/event.h
> +++ b/tools/lib/perf/include/perf/event.h
> @@ -457,6 +457,16 @@ struct perf_record_compressed {
>  	char			 data[];
>  };
> 
> +/*
> + * `header.size` includes the padding we are going to add while writing the record.
> + * `data_size` only includes the size of `data[]` itself.
> + */
> +struct perf_record_compressed2 {
> +	struct perf_event_header header;
> +	__u64			 data_size;
> +	char			 data[];
> +};
> +
>  enum perf_user_event_type { /* above any possible kernel type */
>  	PERF_RECORD_USER_TYPE_START		= 64,
>  	PERF_RECORD_HEADER_ATTR			= 64,
> @@ -478,6 +488,7 @@ enum perf_user_event_type { /* above any possible kernel type */
>  	PERF_RECORD_HEADER_FEATURE		= 80,
>  	PERF_RECORD_COMPRESSED			= 81,
>  	PERF_RECORD_FINISHED_INIT		= 82,
> +	PERF_RECORD_COMPRESSED2			= 83,
>  	PERF_RECORD_HEADER_MAX
>  };
> 
> @@ -518,6 +529,7 @@ union perf_event {
>  	struct perf_record_time_conv		time_conv;
>  	struct perf_record_header_feature	feat;
>  	struct perf_record_compressed		pack;
> +	struct perf_record_compressed2		pack2;
>  };
> 
>  #endif /* __LIBPERF_EVENT_H */
> diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
> index 010a4edcd384..cd95ba09f727 100644
> --- a/tools/perf/Documentation/perf.data-file-format.txt
> +++ b/tools/perf/Documentation/perf.data-file-format.txt
> @@ -370,7 +370,7 @@ struct {
>  	u32	mmap_len;
>  };
> 
> -Indicates that trace contains records of PERF_RECORD_COMPRESSED type
> +Indicates that trace contains records of PERF_RECORD_COMPRESSED2 type
>  that have perf_events records in compressed form.
> 
>  	HEADER_CPU_PMU_CAPS = 28,
> @@ -602,7 +602,14 @@ struct auxtrace_error_event {
>  Describes a header feature. These are records used in pipe-mode that
>  contain information that otherwise would be in perf.data file's header.
> 
> -	PERF_RECORD_COMPRESSED 			= 81,
> +	PERF_RECORD_COMPRESSED 			= 81, /* deprecated */
> +
> +The header is followed by compressed data frame that can be decompressed
> +into array of perf trace records. The size of the entire compressed event
> +record including the header is limited by the max value of header.size.
> +
> +It is deprecated and new files should use PERF_RECORD_COMPRESSED2 to gurantee
> +8-byte alignment.
> 
>  struct compressed_event {
>  	struct perf_event_header	header;
> @@ -618,10 +625,17 @@ This is used, for instance, to 'perf inject' events after init and before
>  regular events, those emitted by the kernel, to support combining guest and
>  host records.
> 
> +	PERF_RECORD_COMPRESSED2			= 83,
> 
> -The header is followed by compressed data frame that can be decompressed
> -into array of perf trace records. The size of the entire compressed event
> -record including the header is limited by the max value of header.size.
> +8-byte aligned version of `PERF_RECORD_COMPRESSED`. `header.size` indicates the
> +total record size, including padding for 8-byte alignment, and `data_size`
> +specifies the actual size of the compressed data.
> +
> +struct perf_record_compressed2 {
> +	struct perf_event_header	header;
> +	__u64				data_size;
> +	char				data[];
> +};
> 
>  Event types
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 9af3f21fd015..d07ad670daa7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -648,14 +648,27 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size)
>  	struct record *rec = to;
> 
>  	if (record__comp_enabled(rec)) {
> +		struct perf_record_compressed2 *event = map->data;
> +		size_t padding = 0;
> +		u8 pad[8] = {0};
>  		ssize_t compressed = zstd_compress(rec->session, map, map->data,
>  						   mmap__mmap_len(map), bf, size);
> 
>  		if (compressed < 0)
>  			return (int)compressed;
> 
> -		size = compressed;
> -		bf   = map->data;
> +		bf = event;
> +		thread->samples++;
> +
> +		/*
> +		 * The record from `zstd_compress` is not 8 bytes aligned, which would cause asan
> +		 * error. We make it aligned here.
> +		 */
> +		event->data_size = compressed - sizeof(struct perf_record_compressed2);
> +		event->header.size = PERF_ALIGN(compressed, sizeof(u64));
> +		padding = event->header.size - compressed;
> +		return record__write(rec, map, bf, compressed) ||
> +		       record__write(rec, map, &pad, padding);
>  	}
> 
>  	thread->samples++;
> @@ -1534,7 +1547,7 @@ static void record__adjust_affinity(struct record *rec, struct mmap *map)
> 
>  static size_t process_comp_header(void *record, size_t increment)
>  {
> -	struct perf_record_compressed *event = record;
> +	struct perf_record_compressed2 *event = record;
>  	size_t size = sizeof(*event);
> 
>  	if (increment) {
> @@ -1542,7 +1555,7 @@ static size_t process_comp_header(void *record, size_t increment)
>  		return increment;
>  	}
> 
> -	event->header.type = PERF_RECORD_COMPRESSED;
> +	event->header.type = PERF_RECORD_COMPRESSED2;
>  	event->header.size = size;
> 
>  	return size;
> @@ -1552,7 +1565,7 @@ static ssize_t zstd_compress(struct perf_session *session, struct mmap *map,
>  			    void *dst, size_t dst_size, void *src, size_t src_size)
>  {
>  	ssize_t compressed;
> -	size_t max_record_size = PERF_SAMPLE_MAX_SIZE - sizeof(struct perf_record_compressed) - 1;
> +	size_t max_record_size = PERF_SAMPLE_MAX_SIZE - sizeof(struct perf_record_compressed2) - 1;
>  	struct zstd_data *zstd_data = &session->zstd_data;
> 
>  	if (map && map->file)
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index c23b77f8f854..80c9ea682413 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -77,6 +77,7 @@ static const char *perf_event__names[] = {
>  	[PERF_RECORD_HEADER_FEATURE]		= "FEATURE",
>  	[PERF_RECORD_COMPRESSED]		= "COMPRESSED",
>  	[PERF_RECORD_FINISHED_INIT]		= "FINISHED_INIT",
> +	[PERF_RECORD_COMPRESSED2]		= "COMPRESSED2",
>  };
> 
>  const char *perf_event__name(unsigned int id)
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 60fb9997ea0d..db2653322f9f 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1400,7 +1400,9 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  	int err;
> 
>  	perf_sample__init(&sample, /*all=*/true);
> -	if (event->header.type != PERF_RECORD_COMPRESSED || perf_tool__compressed_is_stub(tool))
> +	if ((event->header.type != PERF_RECORD_COMPRESSED &&
> +	     event->header.type != PERF_RECORD_COMPRESSED2) ||
> +	    perf_tool__compressed_is_stub(tool))
>  		dump_event(session->evlist, event, file_offset, &sample, file_path);
> 
>  	/* These events are processed right away */
> @@ -1481,6 +1483,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  		err = tool->feature(session, event);
>  		break;
>  	case PERF_RECORD_COMPRESSED:
> +	case PERF_RECORD_COMPRESSED2:
>  		err = tool->compressed(session, event, file_offset, file_path);
>  		if (err)
>  			dump_event(session->evlist, event, file_offset, &sample, file_path);
> diff --git a/tools/perf/util/tool.c b/tools/perf/util/tool.c
> index 3b7f390f26eb..37bd8ac63b01 100644
> --- a/tools/perf/util/tool.c
> +++ b/tools/perf/util/tool.c
> @@ -43,8 +43,15 @@ static int perf_session__process_compressed_event(struct perf_session *session,
>  		decomp->size = decomp_last_rem;
>  	}
> 
> -	src = (void *)event + sizeof(struct perf_record_compressed);
> -	src_size = event->pack.header.size - sizeof(struct perf_record_compressed);
> +	if (event->header.type == PERF_RECORD_COMPRESSED) {
> +		src = (void *)event + sizeof(struct perf_record_compressed);
> +		src_size = event->pack.header.size - sizeof(struct perf_record_compressed);
> +	} else if (event->header.type == PERF_RECORD_COMPRESSED2) {
> +		src = (void *)event + sizeof(struct perf_record_compressed2);
> +		src_size = event->pack2.data_size;
> +	} else {
> +		return -1;
> +	}
> 
>  	decomp_size = zstd_decompress_stream(session->active_decomp->zstd_decomp, src, src_size,
>  				&(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
> --
> 2.48.1.711.g2feabab25a-goog
> 

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Fri, Mar 14, 2025 at 06:27:05PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > asan runtime error:
> > 
> >   # Build with asan
> >   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
> >   # Test success with many asan runtime errors:
> >   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
> >    83: Zstd perf.data compression/decompression:
> >   ...
> >   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
> >   0x7f69e3f99653: note: pointer points here
> >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> >                 ^
> >   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
> >   0x7f69e3f99653: note: pointer points here
> >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> >                 ^
> >   ...
> > 
> > Since there is no way to align compressed data in zstd compression, this
> > patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> > `data_size` to specify the actual compressed data size. The
> > `header.size` contains the total record size, including the padding at
> > the end to make it 8-byte aligned.
> > 
> > Tested with `Zstd perf.data compression/decompression`
> 
> Looks good to me.
> 
> Arnaldo, are you ok with adding a new record type for this?

Checking the discussion and the patch.

- ARnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Mar 14, 2025 at 06:27:05PM -0700, Namhyung Kim wrote:
> > On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> > > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > > asan runtime error:

> > >   # Build with asan
> > >   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
> > >   # Test success with many asan runtime errors:
> > >   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
> > >    83: Zstd perf.data compression/decompression:
> > >   ...
> > >   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
> > >   0x7f69e3f99653: note: pointer points here
> > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > >                 ^
> > >   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
> > >   0x7f69e3f99653: note: pointer points here
> > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > >                 ^
> > >   ...

> > > Since there is no way to align compressed data in zstd compression, this
> > > patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> > > `data_size` to specify the actual compressed data size. The
> > > `header.size` contains the total record size, including the padding at
> > > the end to make it 8-byte aligned.

> > > Tested with `Zstd perf.data compression/decompression`

> > Looks good to me.
 
> > Arnaldo, are you ok with adding a new record type for this?
 
> Checking the discussion and the patch.

My first impression yesterday when I saw this on the smartphone was: how
will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
ignore it while emitting a warning, since it can be skipped and then
what we will get a partial view?

Having some session output showing how an older perf binary handles
PERF_RECORD_COMPRESS2 would be informative.

I'll try to reproduce/test this all...

- Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Ian Rogers]

On Mon, Mar 17, 2025 at 9:17 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Fri, Mar 14, 2025 at 06:27:05PM -0700, Namhyung Kim wrote:
> > > On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> > > > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > > > asan runtime error:
>
> > > >   # Build with asan
> > > >   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
> > > >   # Test success with many asan runtime errors:
> > > >   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
> > > >    83: Zstd perf.data compression/decompression:
> > > >   ...
> > > >   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
> > > >   0x7f69e3f99653: note: pointer points here
> > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > >                 ^
> > > >   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
> > > >   0x7f69e3f99653: note: pointer points here
> > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > >                 ^
> > > >   ...
>
> > > > Since there is no way to align compressed data in zstd compression, this
> > > > patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> > > > `data_size` to specify the actual compressed data size. The
> > > > `header.size` contains the total record size, including the padding at
> > > > the end to make it 8-byte aligned.
>
> > > > Tested with `Zstd perf.data compression/decompression`
>
> > > Looks good to me.
>
> > > Arnaldo, are you ok with adding a new record type for this?
>
> > Checking the discussion and the patch.
>
> My first impression yesterday when I saw this on the smartphone was: how
> will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> ignore it while emitting a warning, since it can be skipped and then
> what we will get a partial view?
>
> Having some session output showing how an older perf binary handles
> PERF_RECORD_COMPRESS2 would be informative.
>
> I'll try to reproduce/test this all...

I'm not sure we've worried about old perfs being able to read new
perf.data files, but we've worried about new perfs being able to read
old perf.data files. So if a change is additive, which this change is,
then nothing should be impacted.

My thoughts are this way as this patch:
https://lore.kernel.org/all/20220614143353.1559597-7-irogers@google.com/
changed most perf.data cpumap encodings in a way that old perfs won't
be able to handle.

Perhaps testing/documentation should be present for this kind of thing.

Thanks,
Ian

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Namhyung Kim]

On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Fri, Mar 14, 2025 at 06:27:05PM -0700, Namhyung Kim wrote:
> > > On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> > > > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > > > asan runtime error:
> 
> > > >   # Build with asan
> > > >   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
> > > >   # Test success with many asan runtime errors:
> > > >   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
> > > >    83: Zstd perf.data compression/decompression:
> > > >   ...
> > > >   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
> > > >   0x7f69e3f99653: note: pointer points here
> > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > >                 ^
> > > >   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
> > > >   0x7f69e3f99653: note: pointer points here
> > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > >                 ^
> > > >   ...
> 
> > > > Since there is no way to align compressed data in zstd compression, this
> > > > patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> > > > `data_size` to specify the actual compressed data size. The
> > > > `header.size` contains the total record size, including the padding at
> > > > the end to make it 8-byte aligned.
> 
> > > > Tested with `Zstd perf.data compression/decompression`
> 
> > > Looks good to me.
>  
> > > Arnaldo, are you ok with adding a new record type for this?
>  
> > Checking the discussion and the patch.
> 
> My first impression yesterday when I saw this on the smartphone was: how
> will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> ignore it while emitting a warning, since it can be skipped and then
> what we will get a partial view?
> 
> Having some session output showing how an older perf binary handles
> PERF_RECORD_COMPRESS2 would be informative.

I think it'll show the below warning:

  <offset> [<size>]: failed to process type: 83

Thanks,
Namhyung

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 09:32:46AM -0700, Ian Rogers wrote:
> On Mon, Mar 17, 2025 at 9:17 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Fri, Mar 14, 2025 at 06:27:05PM -0700, Namhyung Kim wrote:
> > > > On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> > > > > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > > > > asan runtime error:
> >
> > > > >   # Build with asan
> > > > >   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
> > > > >   # Test success with many asan runtime errors:
> > > > >   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
> > > > >    83: Zstd perf.data compression/decompression:
> > > > >   ...
> > > > >   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
> > > > >   0x7f69e3f99653: note: pointer points here
> > > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > > >                 ^
> > > > >   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
> > > > >   0x7f69e3f99653: note: pointer points here
> > > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > > >                 ^
> > > > >   ...
> >
> > > > > Since there is no way to align compressed data in zstd compression, this
> > > > > patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> > > > > `data_size` to specify the actual compressed data size. The
> > > > > `header.size` contains the total record size, including the padding at
> > > > > the end to make it 8-byte aligned.
> >
> > > > > Tested with `Zstd perf.data compression/decompression`
> >
> > > > Looks good to me.
> >
> > > > Arnaldo, are you ok with adding a new record type for this?
> >
> > > Checking the discussion and the patch.
> >
> > My first impression yesterday when I saw this on the smartphone was: how
> > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > ignore it while emitting a warning, since it can be skipped and then
> > what we will get a partial view?
> >
> > Having some session output showing how an older perf binary handles
> > PERF_RECORD_COMPRESS2 would be informative.
> >
> > I'll try to reproduce/test this all...
> 
> I'm not sure we've worried about old perfs being able to read new
> perf.data files, but we've worried about new perfs being able to read
> old perf.data files. So if a change is additive, which this change is,
> then nothing should be impacted.

Right, its difficult to make it work both ways, even with testing, but
by 'work' I mean that new stuff should be ignored by older versions,
i.e. records skipped and then the results will surely be different.

So I'm just curious how older tools will handle these new files and to,
if not that super difficult, to improve how we handle unknown records so
that in the future, when we add new stuff, we mention that this is
something not handled, please use a new tool.
 
> My thoughts are this way as this patch:
> https://lore.kernel.org/all/20220614143353.1559597-7-irogers@google.com/
> changed most perf.data cpumap encodings in a way that old perfs won't
> be able to handle.
 
> Perhaps testing/documentation should be present for this kind of thing.

Right, but in this specific case it should be a matter of telling the
user that the header.type PERF_RECORD_COMPRESS2 isn't supported and that
the user should try and update their tool.

But now back to figuring out how to generate PERF_RECORD_COMPRESS is
generated, use it, then apply this patch, and then see if how the old
tool copes.

So documentation is also lacking in suggesting that enumerating the
steps needed to test before/after is greatly appreciated.

- Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 04:36:05PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Mar 17, 2025 at 09:32:46AM -0700, Ian Rogers wrote:
> > On Mon, Mar 17, 2025 at 9:17 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > On Fri, Mar 14, 2025 at 06:27:05PM -0700, Namhyung Kim wrote:
> > > > > On Mon, Mar 03, 2025 at 10:32:40AM -0800, Chun-Tse Shao wrote:
> > > > > > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > > > > > asan runtime error:
> > >
> > > > > >   # Build with asan
> > > > > >   $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
> > > > > >   # Test success with many asan runtime errors:
> > > > > >   $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
> > > > > >    83: Zstd perf.data compression/decompression:
> > > > > >   ...
> > > > > >   util/session.c:1959:13: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 13 byte alignment
> > > > > >   0x7f69e3f99653: note: pointer points here
> > > > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > > > >                 ^
> > > > > >   util/session.c:2163:22: runtime error: member access within misaligned address 0x7f69e3f99653 for type 'union perf_event', which requires 8 byte alignment
> > > > > >   0x7f69e3f99653: note: pointer points here
> > > > > >    d0  3a 50 69 44 00 00 00 00  00 08 00 bb 07 00 00 00  00 00 00 44 00 00 00 00  00 00 00 ff 07 00 00
> > > > > >                 ^
> > > > > >   ...
> > >
> > > > > > Since there is no way to align compressed data in zstd compression, this
> > > > > > patch add a new event type `PERF_RECORD_COMPRESSED2`, which adds a field
> > > > > > `data_size` to specify the actual compressed data size. The
> > > > > > `header.size` contains the total record size, including the padding at
> > > > > > the end to make it 8-byte aligned.
> > >
> > > > > > Tested with `Zstd perf.data compression/decompression`
> > >
> > > > > Looks good to me.
> > >
> > > > > Arnaldo, are you ok with adding a new record type for this?
> > >
> > > > Checking the discussion and the patch.
> > >
> > > My first impression yesterday when I saw this on the smartphone was: how
> > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > ignore it while emitting a warning, since it can be skipped and then
> > > what we will get a partial view?
> > >
> > > Having some session output showing how an older perf binary handles
> > > PERF_RECORD_COMPRESS2 would be informative.
> > >
> > > I'll try to reproduce/test this all...
> > 
> > I'm not sure we've worried about old perfs being able to read new
> > perf.data files, but we've worried about new perfs being able to read
> > old perf.data files. So if a change is additive, which this change is,
> > then nothing should be impacted.
> 
> Right, its difficult to make it work both ways, even with testing, but
> by 'work' I mean that new stuff should be ignored by older versions,
> i.e. records skipped and then the results will surely be different.
> 
> So I'm just curious how older tools will handle these new files and to,
> if not that super difficult, to improve how we handle unknown records so
> that in the future, when we add new stuff, we mention that this is
> something not handled, please use a new tool.
>  
> > My thoughts are this way as this patch:
> > https://lore.kernel.org/all/20220614143353.1559597-7-irogers@google.com/
> > changed most perf.data cpumap encodings in a way that old perfs won't
> > be able to handle.
>  
> > Perhaps testing/documentation should be present for this kind of thing.
> 
> Right, but in this specific case it should be a matter of telling the
> user that the header.type PERF_RECORD_COMPRESS2 isn't supported and that
> the user should try and update their tool.
> 
> But now back to figuring out how to generate PERF_RECORD_COMPRESS is
> generated, use it, then apply this patch, and then see if how the old
> tool copes.

Which is present in this series, we have even a perf test shell for it,
so happy to not having figured that before :-)

  # Build with asan
  $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g -fno-omit-frame-pointer -fsanitize=undefined"
  # Test success with many asan runtime errors:
  $ /tmp/perf/perf test "Zstd perf.data compression/decompression" -vv
   83: Zstd perf.data compression/decompression:
 
> So documentation is also lacking in suggesting that enumerating the
> steps needed to test before/after is greatly appreciated.
> 
> - Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Checking the discussion and the patch.
> > 
> > My first impression yesterday when I saw this on the smartphone was: how
> > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > ignore it while emitting a warning, since it can be skipped and then
> > what we will get a partial view?
> > 
> > Having some session output showing how an older perf binary handles
> > PERF_RECORD_COMPRESS2 would be informative.
> 
> I think it'll show the below warning:
> 
>   <offset> [<size>]: failed to process type: 83

Right that is what I got:

⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8 
0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
⬢ [acme@toolbox perf-tools-next]$

I think we should change that to something more informative, like:

0xbf0 [0x250]: failed to process unknown type 83, please update perf.

And then does it stop at that record it doesn't grok?

        if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
                pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
                       head, event->header.size, event->header.type);
                err = -EINVAL;
                goto out_err;
        }

        head += size;

So we're stopping there.

Maybe we can just warn and skip?

Anyway, the series as is seems ok.

I'll test a bit more and send my Tested-by

- Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Chun-Tse Shao]

On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> > On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Checking the discussion and the patch.
> > >
> > > My first impression yesterday when I saw this on the smartphone was: how
> > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > ignore it while emitting a warning, since it can be skipped and then
> > > what we will get a partial view?
> > >
> > > Having some session output showing how an older perf binary handles
> > > PERF_RECORD_COMPRESS2 would be informative.
> >
> > I think it'll show the below warning:
> >
> >   <offset> [<size>]: failed to process type: 83
>
> Right that is what I got:
>
> ⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8
> 0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
> ⬢ [acme@toolbox perf-tools-next]$
>
> I think we should change that to something more informative, like:
>
> 0xbf0 [0x250]: failed to process unknown type 83, please update perf.
>
> And then does it stop at that record it doesn't grok?
>
>         if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
>                 pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
>                        head, event->header.size, event->header.type);
>                 err = -EINVAL;
>                 goto out_err;
>         }
>
>         head += size;
>
> So we're stopping there.
>
> Maybe we can just warn and skip?

Thank you Arnaldo, it is a good suggestion and I will work on this later.

-CT

>
> Anyway, the series as is seems ok.
>
> I'll test a bit more and send my Tested-by
>
> - Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 02:45:39PM -0700, Chun-Tse Shao wrote:
> On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> > > On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Checking the discussion and the patch.
> > > >
> > > > My first impression yesterday when I saw this on the smartphone was: how
> > > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > > ignore it while emitting a warning, since it can be skipped and then
> > > > what we will get a partial view?
> > > >
> > > > Having some session output showing how an older perf binary handles
> > > > PERF_RECORD_COMPRESS2 would be informative.
> > >
> > > I think it'll show the below warning:
> > >
> > >   <offset> [<size>]: failed to process type: 83
> >
> > Right that is what I got:
> >
> > ⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8
> > 0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
> > ⬢ [acme@toolbox perf-tools-next]$
> >
> > I think we should change that to something more informative, like:
> >
> > 0xbf0 [0x250]: failed to process unknown type 83, please update perf.
> >
> > And then does it stop at that record it doesn't grok?
> >
> >         if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
> >                 pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> >                        head, event->header.size, event->header.type);
> >                 err = -EINVAL;
> >                 goto out_err;
> >         }
> >
> >         head += size;
> >
> > So we're stopping there.
> >
> > Maybe we can just warn and skip?
> 
> Thank you Arnaldo, it is a good suggestion and I will work on this later.

Thank you for considering that, really appreciated!

perf deals with so much stuff and code flux that all the help that we
can get is what is needed for it to continue to be relevant and useful.

After all what is the point of a tool that produces bad results? :-)

- Arnaldo

> -CT
> 
> >
> > Anyway, the series as is seems ok.
> >
> > I'll test a bit more and send my Tested-by
> >
> > - Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Namhyung Kim]

Hello,

On Mon, Mar 17, 2025 at 06:49:32PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Mar 17, 2025 at 02:45:39PM -0700, Chun-Tse Shao wrote:
> > On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> > > > On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > Checking the discussion and the patch.
> > > > >
> > > > > My first impression yesterday when I saw this on the smartphone was: how
> > > > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > > > ignore it while emitting a warning, since it can be skipped and then
> > > > > what we will get a partial view?
> > > > >
> > > > > Having some session output showing how an older perf binary handles
> > > > > PERF_RECORD_COMPRESS2 would be informative.
> > > >
> > > > I think it'll show the below warning:
> > > >
> > > >   <offset> [<size>]: failed to process type: 83
> > >
> > > Right that is what I got:
> > >
> > > ⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8
> > > 0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
> > > ⬢ [acme@toolbox perf-tools-next]$
> > >
> > > I think we should change that to something more informative, like:
> > >
> > > 0xbf0 [0x250]: failed to process unknown type 83, please update perf.

That would be nice, but there are cases it can fail even without new
record formats.  So it should also check if the type number is greater
than or equal to the max.

> > >
> > > And then does it stop at that record it doesn't grok?
> > >
> > >         if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
> > >                 pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> > >                        head, event->header.size, event->header.type);
> > >                 err = -EINVAL;
> > >                 goto out_err;
> > >         }
> > >
> > >         head += size;
> > >
> > > So we're stopping there.
> > >
> > > Maybe we can just warn and skip?
> > 
> > Thank you Arnaldo, it is a good suggestion and I will work on this later.
> 
> Thank you for considering that, really appreciated!

It would be hard to process misaligned data though.  Probably we also
want to add a check to make sure it's properly aligned.

Thanks,
Namhyung

> 
> perf deals with so much stuff and code flux that all the help that we
> can get is what is needed for it to continue to be relevant and useful.
> 
> After all what is the point of a tool that produces bad results? :-)
> 
> - Arnaldo
> 
> > -CT
> > 
> > >
> > > Anyway, the series as is seems ok.
> > >
> > > I'll test a bit more and send my Tested-by
> > >
> > > - Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Chun-Tse Shao]

Ping.

For suggestions from Namhyung and Arnaldo, it was merged in:

https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=b1b26ce8bb0eab1d058353ab6fa1a2b652a9a020

Thanks,
CT

On Mon, Mar 17, 2025 at 10:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Mon, Mar 17, 2025 at 06:49:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Mar 17, 2025 at 02:45:39PM -0700, Chun-Tse Shao wrote:
> > > On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
> > > <acme@kernel.org> wrote:
> > > >
> > > > On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> > > > > On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > > Checking the discussion and the patch.
> > > > > >
> > > > > > My first impression yesterday when I saw this on the smartphone was: how
> > > > > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > > > > ignore it while emitting a warning, since it can be skipped and then
> > > > > > what we will get a partial view?
> > > > > >
> > > > > > Having some session output showing how an older perf binary handles
> > > > > > PERF_RECORD_COMPRESS2 would be informative.
> > > > >
> > > > > I think it'll show the below warning:
> > > > >
> > > > >   <offset> [<size>]: failed to process type: 83
> > > >
> > > > Right that is what I got:
> > > >
> > > > ⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8
> > > > 0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
> > > > ⬢ [acme@toolbox perf-tools-next]$
> > > >
> > > > I think we should change that to something more informative, like:
> > > >
> > > > 0xbf0 [0x250]: failed to process unknown type 83, please update perf.
>
> That would be nice, but there are cases it can fail even without new
> record formats.  So it should also check if the type number is greater
> than or equal to the max.
>
> > > >
> > > > And then does it stop at that record it doesn't grok?
> > > >
> > > >         if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
> > > >                 pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> > > >                        head, event->header.size, event->header.type);
> > > >                 err = -EINVAL;
> > > >                 goto out_err;
> > > >         }
> > > >
> > > >         head += size;
> > > >
> > > > So we're stopping there.
> > > >
> > > > Maybe we can just warn and skip?
> > >
> > > Thank you Arnaldo, it is a good suggestion and I will work on this later.
> >
> > Thank you for considering that, really appreciated!
>
> It would be hard to process misaligned data though.  Probably we also
> want to add a check to make sure it's properly aligned.
>
> Thanks,
> Namhyung
>
> >
> > perf deals with so much stuff and code flux that all the help that we
> > can get is what is needed for it to continue to be relevant and useful.
> >
> > After all what is the point of a tool that produces bad results? :-)
> >
> > - Arnaldo
> >
> > > -CT
> > >
> > > >
> > > > Anyway, the series as is seems ok.
> > > >
> > > > I'll test a bit more and send my Tested-by
> > > >
> > > > - Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Arnaldo Carvalho de Melo]

On Fri, May 16, 2025 at 10:10:27AM -0700, Chun-Tse Shao wrote:
> Ping.
> 
> For suggestions from Namhyung and Arnaldo, it was merged in:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=b1b26ce8bb0eab1d058353ab6fa1a2b652a9a020

Testing it now, fixed up this:

+It is deprecated and new files should use PERF_RECORD_COMPRESSED2 to gurantee
                                                                      guarantee
+8-byte alignment.

- Arnaldo
 
> Thanks,
> CT
> 
> On Mon, Mar 17, 2025 at 10:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > On Mon, Mar 17, 2025 at 06:49:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Mon, Mar 17, 2025 at 02:45:39PM -0700, Chun-Tse Shao wrote:
> > > > On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
> > > > <acme@kernel.org> wrote:
> > > > >
> > > > > On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> > > > > > On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > > > Checking the discussion and the patch.
> > > > > > >
> > > > > > > My first impression yesterday when I saw this on the smartphone was: how
> > > > > > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > > > > > ignore it while emitting a warning, since it can be skipped and then
> > > > > > > what we will get a partial view?
> > > > > > >
> > > > > > > Having some session output showing how an older perf binary handles
> > > > > > > PERF_RECORD_COMPRESS2 would be informative.
> > > > > >
> > > > > > I think it'll show the below warning:
> > > > > >
> > > > > >   <offset> [<size>]: failed to process type: 83
> > > > >
> > > > > Right that is what I got:
> > > > >
> > > > > ⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8
> > > > > 0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
> > > > > ⬢ [acme@toolbox perf-tools-next]$
> > > > >
> > > > > I think we should change that to something more informative, like:
> > > > >
> > > > > 0xbf0 [0x250]: failed to process unknown type 83, please update perf.
> >
> > That would be nice, but there are cases it can fail even without new
> > record formats.  So it should also check if the type number is greater
> > than or equal to the max.
> >
> > > > >
> > > > > And then does it stop at that record it doesn't grok?
> > > > >
> > > > >         if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
> > > > >                 pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> > > > >                        head, event->header.size, event->header.type);
> > > > >                 err = -EINVAL;
> > > > >                 goto out_err;
> > > > >         }
> > > > >
> > > > >         head += size;
> > > > >
> > > > > So we're stopping there.
> > > > >
> > > > > Maybe we can just warn and skip?
> > > >
> > > > Thank you Arnaldo, it is a good suggestion and I will work on this later.
> > >
> > > Thank you for considering that, really appreciated!
> >
> > It would be hard to process misaligned data though.  Probably we also
> > want to add a check to make sure it's properly aligned.
> >
> > Thanks,
> > Namhyung
> >
> > >
> > > perf deals with so much stuff and code flux that all the help that we
> > > can get is what is needed for it to continue to be relevant and useful.
> > >
> > > After all what is the point of a tool that produces bad results? :-)
> > >
> > > - Arnaldo
> > >
> > > > -CT
> > > >
> > > > >
> > > > > Anyway, the series as is seems ok.
> > > > >
> > > > > I'll test a bit more and send my Tested-by
> > > > >
> > > > > - Arnaldo

Re: [PATCH v2 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2

[Chun-Tse Shao]

Thank you for the fix, Arnaldo!

-CT

On Fri, May 16, 2025 at 12:16 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Fri, May 16, 2025 at 10:10:27AM -0700, Chun-Tse Shao wrote:
> > Ping.
> >
> > For suggestions from Namhyung and Arnaldo, it was merged in:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=b1b26ce8bb0eab1d058353ab6fa1a2b652a9a020
>
> Testing it now, fixed up this:
>
> +It is deprecated and new files should use PERF_RECORD_COMPRESSED2 to gurantee
>                                                                       guarantee
> +8-byte alignment.
>
> - Arnaldo
>
> > Thanks,
> > CT
> >
> > On Mon, Mar 17, 2025 at 10:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > On Mon, Mar 17, 2025 at 06:49:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > On Mon, Mar 17, 2025 at 02:45:39PM -0700, Chun-Tse Shao wrote:
> > > > > On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
> > > > > <acme@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Mar 17, 2025 at 09:46:40AM -0700, Namhyung Kim wrote:
> > > > > > > On Mon, Mar 17, 2025 at 01:17:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > > > On Mon, Mar 17, 2025 at 12:52:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > > > > Checking the discussion and the patch.
> > > > > > > >
> > > > > > > > My first impression yesterday when I saw this on the smartphone was: how
> > > > > > > > will an old perf binary handle the new PERF_RECORD_COMPRESSED2? Will it
> > > > > > > > ignore it while emitting a warning, since it can be skipped and then
> > > > > > > > what we will get a partial view?
> > > > > > > >
> > > > > > > > Having some session output showing how an older perf binary handles
> > > > > > > > PERF_RECORD_COMPRESS2 would be informative.
> > > > > > >
> > > > > > > I think it'll show the below warning:
> > > > > > >
> > > > > > >   <offset> [<size>]: failed to process type: 83
> > > > > >
> > > > > > Right that is what I got:
> > > > > >
> > > > > > ⬢ [acme@toolbox perf-tools-next]$ perf.old script -i /tmp/perf.data.ck8
> > > > > > 0xbf0 [0x250]: failed to process type: 83 [Invalid argument]
> > > > > > ⬢ [acme@toolbox perf-tools-next]$
> > > > > >
> > > > > > I think we should change that to something more informative, like:
> > > > > >
> > > > > > 0xbf0 [0x250]: failed to process unknown type 83, please update perf.
> > >
> > > That would be nice, but there are cases it can fail even without new
> > > record formats.  So it should also check if the type number is greater
> > > than or equal to the max.
> > >
> > > > > >
> > > > > > And then does it stop at that record it doesn't grok?
> > > > > >
> > > > > >         if ((skip = perf_session__process_event(session, event, head, "pipe")) < 0) {
> > > > > >                 pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> > > > > >                        head, event->header.size, event->header.type);
> > > > > >                 err = -EINVAL;
> > > > > >                 goto out_err;
> > > > > >         }
> > > > > >
> > > > > >         head += size;
> > > > > >
> > > > > > So we're stopping there.
> > > > > >
> > > > > > Maybe we can just warn and skip?
> > > > >
> > > > > Thank you Arnaldo, it is a good suggestion and I will work on this later.
> > > >
> > > > Thank you for considering that, really appreciated!
> > >
> > > It would be hard to process misaligned data though.  Probably we also
> > > want to add a check to make sure it's properly aligned.
> > >
> > > Thanks,
> > > Namhyung
> > >
> > > >
> > > > perf deals with so much stuff and code flux that all the help that we
> > > > can get is what is needed for it to continue to be relevant and useful.
> > > >
> > > > After all what is the point of a tool that produces bad results? :-)
> > > >
> > > > - Arnaldo
> > > >
> > > > > -CT
> > > > >
> > > > > >
> > > > > > Anyway, the series as is seems ok.
> > > > > >
> > > > > > I'll test a bit more and send my Tested-by
> > > > > >
> > > > > > - Arnaldo