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

[Namhyung Kim]

Hello,

On Mon, Mar 17, 2025 at 06:49:32PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Mar 17, 2025 at 02:45:39PM -0700, Chun-Tse Shao wrote:
> > On Mon, Mar 17, 2025 at 1:35 PM Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx> wrote:
> > >
> > > 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.

That would be nice, but there are cases it can fail even without new
record formats.  So it should also check if the type number is greater
than or equal to the max.

> > >
> > > 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?
> > 
> > Thank you Arnaldo, it is a good suggestion and I will work on this later.
> 
> Thank you for considering that, really appreciated!

It would be hard to process misaligned data though.  Probably we also
want to add a check to make sure it's properly aligned.

Thanks,
Namhyung

> 
> perf deals with so much stuff and code flux that all the help that we
> can get is what is needed for it to continue to be relevant and useful.
> 
> After all what is the point of a tool that produces bad results? :-)
> 
> - Arnaldo
> 
> > -CT
> > 
> > >
> > > Anyway, the series as is seems ok.
> > >
> > > I'll test a bit more and send my Tested-by
> > >
> > > - Arnaldo