Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[Namhyung Kim <namhyung@kernel.org>]

HI Adrian,

Sorry for the late reply.

On Mon, Aug 14, 2023 at 12:11 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/08/23 09:16, Namhyung Kim wrote:
> > The PERF_RECORD_ATTR is used for a pipe mode to describe an event with
> > attribute and IDs.  The ID table comes after the attr and it calculate
> > size of the table using the total record size and the attr size.
> >
> >   n_ids = (total_record_size - end_of_the_attr_field) / sizeof(u64)
> >
> > This is fine for most use cases, but sometimes it saves the pipe output
> > in a file and then process it later.  And it becomes a problem if there
> > is a change in attr size between the record and report.
> >
> >   $ perf record -o- > perf-pipe.data  # old version
> >   $ perf report -i- < perf-pipe.data  # new version
> >
> > For example, if the attr size is 128 and it has 4 IDs, then it would
> > save them in 168 byte like below:
> >
> >    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
> >  128 byte: perf event attr { .size = 128, ... },
> >   32 byte: event IDs [] = { 1234, 1235, 1236, 1237 },
> >
> > But when report later, it thinks the attr size is 136 then it only read
> > the last 3 entries as ID.
> >
> >    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
> >  136 byte: perf event attr { .size = 136, ... },
> >   24 byte: event IDs [] = { 1235, 1236, 1237 },  // 1234 is missing
> >
> > So it should use the recorded version of the attr.  The attr has the
> > size field already then it should honor the size when reading data.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/header.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 52fbf526fe74..f89321cbfdee 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -4381,7 +4381,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
> >                            union perf_event *event,
> >                            struct evlist **pevlist)
> >  {
> > -     u32 i, ids, n_ids;
> > +     u32 i, n_ids;
> > +     u64 *ids;
> >       struct evsel *evsel;
> >       struct evlist *evlist = *pevlist;
> >
> > @@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
> >
> >       evlist__add(evlist, evsel);
> >
> > -     ids = event->header.size;
> > -     ids -= (void *)&event->attr.id - (void *)event;
> > -     n_ids = ids / sizeof(u64);
> > +     n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> > +     n_ids = n_ids / sizeof(u64);
> >       /*
> >        * We don't have the cpu and thread maps on the header, so
> >        * for allocating the perf_sample_id table we fake 1 cpu and
> > @@ -4408,8 +4408,9 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
> >       if (perf_evsel__alloc_id(&evsel->core, 1, n_ids))
> >               return -ENOMEM;
> >
> > +     ids = (void *)&event->attr.attr + event->attr.attr.size;
> >       for (i = 0; i < n_ids; i++) {
> > -             perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, event->attr.id[i]);
> > +             perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
> >       }
> >
> >       return 0;
>
> This is a good catch!
>
> It looks like perf_event__hdr_swap() might also have this problem.

You mean perf_event__hdr_attr_swap(), right?  Yeah, looks so.
I'll change it too in v2.

>
> I wonder if we should remove 'id' from struct perf_record_header_attr
> since the position is not guaranteed?
>
> Probably could use a comment there either way.

Sounds good.  I'll remove the id field and add a comment.

>
> Also perhaps a fixes tag and cc stable

Sure, thanks for the review!

Namhyung