* [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 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 ` 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 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 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 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 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