Messages in this thread | | | Date | Mon, 14 Aug 2023 10:11:40 +0300 | Subject | Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR | From | Adrian Hunter <> |
| |
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.
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.
Also perhaps a fixes tag and cc stable
| |