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

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 09:32:46AM -0700, Ian Rogers wrote:
> On Mon, Mar 17, 2025 at 9:17 AM Arnaldo Carvalho de Melo <acme@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.

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

So I'm just curious how older tools will handle these new files and to,
if not that super difficult, to improve how we handle unknown records so
that in the future, when we add new stuff, we mention that this is
something not handled, please use a new tool.
 
> My thoughts are this way as this patch:
> https://lore.kernel.org/all/20220614143353.1559597-7-irogers@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.

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

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

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

- Arnaldo