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

Chun-Tse Shao
2025-05-16T21:44:14Z

Thank you for the fix, Arnaldo!

-CT

On Fri, May 16, 2025 at 12:16 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Fri, May 16, 2025 at 10:10:27AM -0700, Chun-Tse Shao wrote:
> > Ping.
> >
> > For suggestions from Namhyung and Arnaldo, it was merged in:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=b1b26ce8bb0eab1d058353ab6fa1a2b652a9a020
>
> Testing it now, fixed up this:
>
> +It is deprecated and new files should use PERF_RECORD_COMPRESSED2 to gurantee
> guarantee
> +8-byte alignment.
>
> - Arnaldo
>
> > Thanks,
> > CT
> >
> > On Mon, Mar 17, 2025 at 10:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > 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@kernel.org> 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