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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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

Right that is what I got:

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

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

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

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

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

        head += size;

So we're stopping there.

Maybe we can just warn and skip?

Anyway, the series as is seems ok.

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

- Arnaldo