* [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
@ 2025-02-27 5:34 Chun-Tse Shao
2025-02-27 5:34 ` [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c Chun-Tse Shao
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Chun-Tse Shao @ 2025-02-27 5:34 UTC (permalink / raw)
To: linux-kernel
Cc: Chun-Tse Shao, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang,
terrelln, leo.yan, dvyukov, ak, james.clark, christophe.leroy,
ben.gainey, linux-perf-users
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>
---
tools/lib/perf/Documentation/libperf.txt | 1 +
tools/lib/perf/include/perf/event.h | 12 ++++++++++
.../Documentation/perf.data-file-format.txt | 17 +++++++++++---
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, 59 insertions(+), 11 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..f5faceb0e248 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
PERF_RECORD_COMPRESSED = 81,
+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.
+
struct compressed_event {
struct perf_event_header header;
char data[];
@@ -618,10 +622,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.658.g4767266eb4-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c
2025-02-27 5:34 [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Chun-Tse Shao
@ 2025-02-27 5:34 ` Chun-Tse Shao
2025-02-27 5:58 ` Ian Rogers
2025-02-27 5:57 ` [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Ian Rogers
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Chun-Tse Shao @ 2025-02-27 5:34 UTC (permalink / raw)
To: linux-kernel
Cc: Chun-Tse Shao, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang,
terrelln, leo.yan, dvyukov, ak, james.clark, christophe.leroy,
ben.gainey, linux-perf-users
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.658.g4767266eb4-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 5:34 [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Chun-Tse Shao
2025-02-27 5:34 ` [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c Chun-Tse Shao
@ 2025-02-27 5:57 ` Ian Rogers
2025-02-27 6:04 ` Andi Kleen
2025-02-27 7:12 ` Ian Rogers
3 siblings, 0 replies; 12+ messages in thread
From: Ian Rogers @ 2025-02-27 5:57 UTC (permalink / raw)
To: Chun-Tse Shao
Cc: linux-kernel, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Wed, Feb 26, 2025 at 9:37 PM Chun-Tse Shao <ctshao@google.com> 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`
>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
Thanks for fixing this!
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/lib/perf/Documentation/libperf.txt | 1 +
> tools/lib/perf/include/perf/event.h | 12 ++++++++++
> .../Documentation/perf.data-file-format.txt | 17 +++++++++++---
> 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, 59 insertions(+), 11 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..f5faceb0e248 100644
> --- a/tools/perf/Documentation/perf.data-file-format.txt
> +++ b/tools/perf/Documentation/perf.data-file-format.txt
> @@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
>
> PERF_RECORD_COMPRESSED = 81,
>
> +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.
> +
> struct compressed_event {
> struct perf_event_header header;
> char data[];
> @@ -618,10 +622,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.658.g4767266eb4-goog
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c
2025-02-27 5:34 ` [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c Chun-Tse Shao
@ 2025-02-27 5:58 ` Ian Rogers
0 siblings, 0 replies; 12+ messages in thread
From: Ian Rogers @ 2025-02-27 5:58 UTC (permalink / raw)
To: Chun-Tse Shao
Cc: linux-kernel, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Wed, Feb 26, 2025 at 9:39 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> 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>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> 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.658.g4767266eb4-goog
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 5:34 [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Chun-Tse Shao
2025-02-27 5:34 ` [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c Chun-Tse Shao
2025-02-27 5:57 ` [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Ian Rogers
@ 2025-02-27 6:04 ` Andi Kleen
2025-02-27 6:20 ` Ian Rogers
2025-02-27 7:12 ` Ian Rogers
3 siblings, 1 reply; 12+ messages in thread
From: Andi Kleen @ 2025-02-27 6:04 UTC (permalink / raw)
To: Chun-Tse Shao
Cc: linux-kernel, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang,
terrelln, leo.yan, dvyukov, james.clark, christophe.leroy,
ben.gainey, linux-perf-users
On Wed, Feb 26, 2025 at 09:34:06PM -0800, Chun-Tse Shao wrote:
> The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> asan runtime error:
It seems pointless. Most architectures have cheap unaligned accesses
these days.
Just disable that error?
-Andi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 6:04 ` Andi Kleen
@ 2025-02-27 6:20 ` Ian Rogers
2025-02-27 8:20 ` Namhyung Kim
0 siblings, 1 reply; 12+ messages in thread
From: Ian Rogers @ 2025-02-27 6:20 UTC (permalink / raw)
To: Andi Kleen
Cc: Chun-Tse Shao, linux-kernel, peterz, mingo, acme, namhyung,
mark.rutland, alexander.shishkin, jolsa, adrian.hunter, kan.liang,
terrelln, leo.yan, dvyukov, james.clark, christophe.leroy,
ben.gainey, linux-perf-users
On Wed, Feb 26, 2025 at 10:04 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> On Wed, Feb 26, 2025 at 09:34:06PM -0800, Chun-Tse Shao wrote:
> > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > asan runtime error:
>
> It seems pointless. Most architectures have cheap unaligned accesses
> these days.
>
> Just disable that error?
The perf_event_header in perf_event.h is:
```
struct perf_event_header {
__u32 type;
__u16 misc;
__u16 size;
};
```
so it is assuming at least 4-byte alignment. 8-byte alignment is
assumed in many places in tools/lib/perf/include/perf/event.h. We pad
events to ensure the alignment in about 30 places already:
```
$ grep -r PERF_ALIGN tools/perf|grep u64|wc -l
32
```
Having sanitizers I think is a must, if we allow unaligned events we'd
need to introduce helper functions or memcpys to workaround the
unaligned undefined behavior. I think the padding is a less worse
alternative and one that was already picked.
Thanks,
Ian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 5:34 [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Chun-Tse Shao
` (2 preceding siblings ...)
2025-02-27 6:04 ` Andi Kleen
@ 2025-02-27 7:12 ` Ian Rogers
2025-02-27 8:35 ` Namhyung Kim
3 siblings, 1 reply; 12+ messages in thread
From: Ian Rogers @ 2025-02-27 7:12 UTC (permalink / raw)
To: Chun-Tse Shao
Cc: linux-kernel, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Wed, Feb 26, 2025 at 9:37 PM Chun-Tse Shao <ctshao@google.com> 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`
>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> ---
> tools/lib/perf/Documentation/libperf.txt | 1 +
> tools/lib/perf/include/perf/event.h | 12 ++++++++++
> .../Documentation/perf.data-file-format.txt | 17 +++++++++++---
> 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, 59 insertions(+), 11 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[];
Just to note that data_size has to be u16 or smaller due to
header.size, so I think you can save some bytes by using a u16 or u8
(for the u8 you could just count the amount of padding and: data_size
= header.size - padding_size).
Thanks,
Ian
> +};
> +
> 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..f5faceb0e248 100644
> --- a/tools/perf/Documentation/perf.data-file-format.txt
> +++ b/tools/perf/Documentation/perf.data-file-format.txt
> @@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
>
> PERF_RECORD_COMPRESSED = 81,
>
> +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.
> +
> struct compressed_event {
> struct perf_event_header header;
> char data[];
> @@ -618,10 +622,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.658.g4767266eb4-goog
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 6:20 ` Ian Rogers
@ 2025-02-27 8:20 ` Namhyung Kim
0 siblings, 0 replies; 12+ messages in thread
From: Namhyung Kim @ 2025-02-27 8:20 UTC (permalink / raw)
To: Ian Rogers
Cc: Andi Kleen, Chun-Tse Shao, linux-kernel, peterz, mingo, acme,
mark.rutland, alexander.shishkin, jolsa, adrian.hunter, kan.liang,
terrelln, leo.yan, dvyukov, james.clark, christophe.leroy,
ben.gainey, linux-perf-users
On Wed, Feb 26, 2025 at 10:20:36PM -0800, Ian Rogers wrote:
> On Wed, Feb 26, 2025 at 10:04 PM Andi Kleen <ak@linux.intel.com> wrote:
> >
> > On Wed, Feb 26, 2025 at 09:34:06PM -0800, Chun-Tse Shao wrote:
> > > The original PERF_RECORD_COMPRESS is not 8-byte aligned, which can cause
> > > asan runtime error:
> >
> > It seems pointless. Most architectures have cheap unaligned accesses
> > these days.
> >
> > Just disable that error?
>
> The perf_event_header in perf_event.h is:
> ```
> struct perf_event_header {
> __u32 type;
> __u16 misc;
> __u16 size;
> };
> ```
> so it is assuming at least 4-byte alignment. 8-byte alignment is
> assumed in many places in tools/lib/perf/include/perf/event.h. We pad
> events to ensure the alignment in about 30 places already:
> ```
> $ grep -r PERF_ALIGN tools/perf|grep u64|wc -l
> 32
> ```
I vaguely remember that it needs 8 bytes alignment to deal with partial
mmap-ed data on 32-bit machines so that it can make sure the header is
not across the mmap boundary.
Thanks,
Namhyung
> Having sanitizers I think is a must, if we allow unaligned events we'd
> need to introduce helper functions or memcpys to workaround the
> unaligned undefined behavior. I think the padding is a less worse
> alternative and one that was already picked.
>
> Thanks,
> Ian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 7:12 ` Ian Rogers
@ 2025-02-27 8:35 ` Namhyung Kim
2025-02-27 18:21 ` Chun-Tse Shao
0 siblings, 1 reply; 12+ messages in thread
From: Namhyung Kim @ 2025-02-27 8:35 UTC (permalink / raw)
To: Ian Rogers
Cc: Chun-Tse Shao, linux-kernel, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Wed, Feb 26, 2025 at 11:12:37PM -0800, Ian Rogers wrote:
> On Wed, Feb 26, 2025 at 9:37 PM Chun-Tse Shao <ctshao@google.com> 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`
> >
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > ---
> > tools/lib/perf/Documentation/libperf.txt | 1 +
> > tools/lib/perf/include/perf/event.h | 12 ++++++++++
> > .../Documentation/perf.data-file-format.txt | 17 +++++++++++---
> > 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, 59 insertions(+), 11 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[];
>
> Just to note that data_size has to be u16 or smaller due to
> header.size, so I think you can save some bytes by using a u16 or u8
> (for the u8 you could just count the amount of padding and: data_size
> = header.size - padding_size).
I was about to suggest using the header.misc in the existing
perf_record_compress. As the padding is up to 7 byte, we could
header.type = PERF_RECORD_COMPRESS;
header.size = PERF_ALIGN(real_size, 8);
header.misc = header.size - real_size;
Assuming the old data doesn't set the misc field and have 0. Then it
would be compatible with old data and get the real size in new data.
But unfortunately, I found process_comp_header() didn't reset the misc
field so it may have garbage in old data. Then the above won't work. :(
Thanks,
Namhyung
>
> Thanks,
> Ian
>
> > +};
> > +
> > 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..f5faceb0e248 100644
> > --- a/tools/perf/Documentation/perf.data-file-format.txt
> > +++ b/tools/perf/Documentation/perf.data-file-format.txt
> > @@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
> >
> > PERF_RECORD_COMPRESSED = 81,
> >
> > +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.
> > +
> > struct compressed_event {
> > struct perf_event_header header;
> > char data[];
> > @@ -618,10 +622,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.658.g4767266eb4-goog
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 8:35 ` Namhyung Kim
@ 2025-02-27 18:21 ` Chun-Tse Shao
2025-03-01 0:32 ` Namhyung Kim
0 siblings, 1 reply; 12+ messages in thread
From: Chun-Tse Shao @ 2025-02-27 18:21 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ian Rogers, linux-kernel, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Thu, Feb 27, 2025 at 12:35 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Feb 26, 2025 at 11:12:37PM -0800, Ian Rogers wrote:
> > On Wed, Feb 26, 2025 at 9:37 PM Chun-Tse Shao <ctshao@google.com> 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`
> > >
> > > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > > ---
> > > tools/lib/perf/Documentation/libperf.txt | 1 +
> > > tools/lib/perf/include/perf/event.h | 12 ++++++++++
> > > .../Documentation/perf.data-file-format.txt | 17 +++++++++++---
> > > 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, 59 insertions(+), 11 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[];
> >
> > Just to note that data_size has to be u16 or smaller due to
> > header.size, so I think you can save some bytes by using a u16 or u8
> > (for the u8 you could just count the amount of padding and: data_size
> > = header.size - padding_size).
>
> I was about to suggest using the header.misc in the existing
> perf_record_compress. As the padding is up to 7 byte, we could
>
> header.type = PERF_RECORD_COMPRESS;
> header.size = PERF_ALIGN(real_size, 8);
> header.misc = header.size - real_size;
>
> Assuming the old data doesn't set the misc field and have 0. Then it
> would be compatible with old data and get the real size in new data.
I was thinking the same way and realized the uninitialized `misc`
problem while I was testing this implementation.
Also I think it would be good to have a new type, at least if
something wrong happens unexpectedly, we know that it is caused by old
or new compressed event.
>
> But unfortunately, I found process_comp_header() didn't reset the misc
> field so it may have garbage in old data. Then the above won't work. :(
>
> Thanks,
> Namhyung
>
> >
> > Thanks,
> > Ian
> >
> > > +};
> > > +
> > > 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..f5faceb0e248 100644
> > > --- a/tools/perf/Documentation/perf.data-file-format.txt
> > > +++ b/tools/perf/Documentation/perf.data-file-format.txt
> > > @@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
> > >
> > > PERF_RECORD_COMPRESSED = 81,
> > >
> > > +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.
> > > +
> > > struct compressed_event {
> > > struct perf_event_header header;
> > > char data[];
> > > @@ -618,10 +622,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.658.g4767266eb4-goog
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-02-27 18:21 ` Chun-Tse Shao
@ 2025-03-01 0:32 ` Namhyung Kim
2025-03-03 5:13 ` Chun-Tse Shao
0 siblings, 1 reply; 12+ messages in thread
From: Namhyung Kim @ 2025-03-01 0:32 UTC (permalink / raw)
To: Chun-Tse Shao
Cc: Ian Rogers, linux-kernel, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Thu, Feb 27, 2025 at 10:21:38AM -0800, Chun-Tse Shao wrote:
> On Thu, Feb 27, 2025 at 12:35 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Feb 26, 2025 at 11:12:37PM -0800, Ian Rogers wrote:
> > > On Wed, Feb 26, 2025 at 9:37 PM Chun-Tse Shao <ctshao@google.com> 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`
Do you mean `perf test Zstd`?
> > > >
> > > > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > > > ---
> > > > tools/lib/perf/Documentation/libperf.txt | 1 +
> > > > tools/lib/perf/include/perf/event.h | 12 ++++++++++
> > > > .../Documentation/perf.data-file-format.txt | 17 +++++++++++---
> > > > 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, 59 insertions(+), 11 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[];
> > >
> > > Just to note that data_size has to be u16 or smaller due to
> > > header.size, so I think you can save some bytes by using a u16 or u8
> > > (for the u8 you could just count the amount of padding and: data_size
> > > = header.size - padding_size).
> >
> > I was about to suggest using the header.misc in the existing
> > perf_record_compress. As the padding is up to 7 byte, we could
> >
> > header.type = PERF_RECORD_COMPRESS;
> > header.size = PERF_ALIGN(real_size, 8);
> > header.misc = header.size - real_size;
> >
> > Assuming the old data doesn't set the misc field and have 0. Then it
> > would be compatible with old data and get the real size in new data.
>
> I was thinking the same way and realized the uninitialized `misc`
> problem while I was testing this implementation.
> Also I think it would be good to have a new type, at least if
> something wrong happens unexpectedly, we know that it is caused by old
> or new compressed event.
Ok, makes sense.
>
> >
> > But unfortunately, I found process_comp_header() didn't reset the misc
> > field so it may have garbage in old data. Then the above won't work. :(
> >
> > Thanks,
> > Namhyung
> >
> > >
> > > Thanks,
> > > Ian
> > >
> > > > +};
> > > > +
> > > > 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..f5faceb0e248 100644
> > > > --- a/tools/perf/Documentation/perf.data-file-format.txt
> > > > +++ b/tools/perf/Documentation/perf.data-file-format.txt
> > > > @@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
> > > >
> > > > PERF_RECORD_COMPRESSED = 81,
> > > >
> > > > +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.
Maybe we can say it's deprecated now. New files should use COMPRESSED2
to guarantee 8-byte alignment.
Thanks,
Namhyung
> > > > +
> > > > struct compressed_event {
> > > > struct perf_event_header header;
> > > > char data[];
> > > > @@ -618,10 +622,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.658.g4767266eb4-goog
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2
2025-03-01 0:32 ` Namhyung Kim
@ 2025-03-03 5:13 ` Chun-Tse Shao
0 siblings, 0 replies; 12+ messages in thread
From: Chun-Tse Shao @ 2025-03-03 5:13 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ian Rogers, linux-kernel, peterz, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, adrian.hunter, kan.liang, terrelln,
leo.yan, dvyukov, ak, james.clark, christophe.leroy, ben.gainey,
linux-perf-users
On Fri, Feb 28, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Feb 27, 2025 at 10:21:38AM -0800, Chun-Tse Shao wrote:
> > On Thu, Feb 27, 2025 at 12:35 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Wed, Feb 26, 2025 at 11:12:37PM -0800, Ian Rogers wrote:
> > > > On Wed, Feb 26, 2025 at 9:37 PM Chun-Tse Shao <ctshao@google.com> 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`
>
> Do you mean `perf test Zstd`?
Yes, `Zstd perf.data compression/decompression` is the full name.
>
> > > > >
> > > > > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> > > > > ---
> > > > > tools/lib/perf/Documentation/libperf.txt | 1 +
> > > > > tools/lib/perf/include/perf/event.h | 12 ++++++++++
> > > > > .../Documentation/perf.data-file-format.txt | 17 +++++++++++---
> > > > > 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, 59 insertions(+), 11 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[];
> > > >
> > > > Just to note that data_size has to be u16 or smaller due to
> > > > header.size, so I think you can save some bytes by using a u16 or u8
> > > > (for the u8 you could just count the amount of padding and: data_size
> > > > = header.size - padding_size).
> > >
> > > I was about to suggest using the header.misc in the existing
> > > perf_record_compress. As the padding is up to 7 byte, we could
> > >
> > > header.type = PERF_RECORD_COMPRESS;
> > > header.size = PERF_ALIGN(real_size, 8);
> > > header.misc = header.size - real_size;
> > >
> > > Assuming the old data doesn't set the misc field and have 0. Then it
> > > would be compatible with old data and get the real size in new data.
> >
> > I was thinking the same way and realized the uninitialized `misc`
> > problem while I was testing this implementation.
> > Also I think it would be good to have a new type, at least if
> > something wrong happens unexpectedly, we know that it is caused by old
> > or new compressed event.
>
> Ok, makes sense.
>
> >
> > >
> > > But unfortunately, I found process_comp_header() didn't reset the misc
> > > field so it may have garbage in old data. Then the above won't work. :(
> > >
> > > Thanks,
> > > Namhyung
> > >
> > > >
> > > > Thanks,
> > > > Ian
> > > >
> > > > > +};
> > > > > +
> > > > > 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..f5faceb0e248 100644
> > > > > --- a/tools/perf/Documentation/perf.data-file-format.txt
> > > > > +++ b/tools/perf/Documentation/perf.data-file-format.txt
> > > > > @@ -604,6 +604,10 @@ contain information that otherwise would be in perf.data file's header.
> > > > >
> > > > > PERF_RECORD_COMPRESSED = 81,
> > > > >
> > > > > +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.
>
> Maybe we can say it's deprecated now. New files should use COMPRESSED2
> to guarantee 8-byte alignment.
Thank you, I will send a new patchset with this comment.
>
> Thanks,
> Namhyung
>
> > > > > +
> > > > > struct compressed_event {
> > > > > struct perf_event_header header;
> > > > > char data[];
> > > > > @@ -618,10 +622,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.658.g4767266eb4-goog
> > > > >
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-03-03 5:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 5:34 [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Chun-Tse Shao
2025-02-27 5:34 ` [PATCH v1 2/2] perf record: Fix a asan runtime error in util/maps.c Chun-Tse Shao
2025-02-27 5:58 ` Ian Rogers
2025-02-27 5:57 ` [PATCH v1 1/2] perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2 Ian Rogers
2025-02-27 6:04 ` Andi Kleen
2025-02-27 6:20 ` Ian Rogers
2025-02-27 8:20 ` Namhyung Kim
2025-02-27 7:12 ` Ian Rogers
2025-02-27 8:35 ` Namhyung Kim
2025-02-27 18:21 ` Chun-Tse Shao
2025-03-01 0:32 ` Namhyung Kim
2025-03-03 5:13 ` Chun-Tse Shao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox