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

[Ian Rogers]

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

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

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

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

Thanks,
Ian