[PATCH v2 1/4] perf tools: Handle old data in PERF_RECORD_ATTR

[PATCH v2 1/4] perf tools: Handle old data in PERF_RECORD_ATTR

[Namhyung Kim]

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.

Fixes: 2c46dbb517a10 ("perf: Convert perf header attrs into attr events")
Cc: stable@vger.kernel.org
Cc: Tom Zanussi <zanussi@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
Keep this version before the libperf change so that it can go through
the stable versions.

 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;
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v2 2/4] tools/lib/perf: Add perf_record_header_attr_id()

[Namhyung Kim]

The HEADER_ATTR record has an event attr followed by the id array.  But
perf data from a different version could have different size of attr.

So it cannot just use event->attr.id to access the array.  Let's add the
perf_record_header_attr_id() macro to calculate the start of the array.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/perf/include/perf/event.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index ba2dcf64f4e6..e563dd8c3628 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -151,6 +151,10 @@ struct perf_record_header_attr {
 	__u64			 id[];
 };
 
+/* Returns the pointer to id array based on the actual attr size. */
+#define perf_record_header_attr_id(evt)			\
+	((void *)&(evt)->attr.attr + (evt)->attr.attr.size)
+
 enum {
 	PERF_CPU_MAP__CPUS = 0,
 	PERF_CPU_MAP__MASK = 1,
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v2 3/4] perf tools: Convert to perf_record_header_attr_id()

[Namhyung Kim]

Instead of accessing the attr.id directly, use the
perf_record_header_attr_id() helper to handle old versions.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/header.c           | 2 +-
 tools/perf/util/session.c          | 4 ++--
 tools/perf/util/synthetic-events.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index f89321cbfdee..63967c34157d 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -4408,7 +4408,7 @@ 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;
+	ids = perf_record_header_attr_id(event);
 	for (i = 0; i < n_ids; i++) {
 		perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
 	}
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 00d18c74c090..1e9aa8ed15b6 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -833,8 +833,8 @@ static void perf_event__hdr_attr_swap(union perf_event *event,
 	perf_event__attr_swap(&event->attr.attr);
 
 	size = event->header.size;
-	size -= (void *)&event->attr.id - (void *)event;
-	mem_bswap_64(event->attr.id, size);
+	size -= perf_record_header_attr_id(event) - (void *)event;
+	mem_bswap_64(perf_record_header_attr_id(event), size);
 }
 
 static void perf_event__event_update_swap(union perf_event *event,
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 45714a2785fd..a0579c7d7b9e 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -2145,7 +2145,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *
 		return -ENOMEM;
 
 	ev->attr.attr = *attr;
-	memcpy(ev->attr.id, id, ids * sizeof(u64));
+	memcpy(perf_record_header_attr_id(ev), id, ids * sizeof(u64));
 
 	ev->attr.header.type = PERF_RECORD_HEADER_ATTR;
 	ev->attr.header.size = (u16)size;
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v2 4/4] tools/lib/perf: Get rid of attr.id field

[Namhyung Kim]

Now there's no in-tree user of the field.  To remove the possible bug
later, let's get rid of the 'id' field and add a comment for that.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/perf/include/perf/event.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index e563dd8c3628..ae64090184d3 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -148,7 +148,13 @@ struct perf_record_switch {
 struct perf_record_header_attr {
 	struct perf_event_header header;
 	struct perf_event_attr	 attr;
-	__u64			 id[];
+	/*
+	 * Array of u64 id follows here but we cannot use a flexible array
+	 * because size of attr in the data can be different then current
+	 * version.  Please use perf_record_header_attr_id() below.
+	 *
+	 * __u64		 id[];  // do not use this
+	 */
 };
 
 /* Returns the pointer to id array based on the actual attr size. */
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH v2 3/4] perf tools: Convert to perf_record_header_attr_id()

[Adrian Hunter]

On 25/08/23 18:25, Namhyung Kim wrote:
> Instead of accessing the attr.id directly, use the
> perf_record_header_attr_id() helper to handle old versions.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/header.c           | 2 +-
>  tools/perf/util/session.c          | 4 ++--
>  tools/perf/util/synthetic-events.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f89321cbfdee..63967c34157d 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -4408,7 +4408,7 @@ 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;
> +	ids = perf_record_header_attr_id(event);
>  	for (i = 0; i < n_ids; i++) {
>  		perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
>  	}
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 00d18c74c090..1e9aa8ed15b6 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -833,8 +833,8 @@ static void perf_event__hdr_attr_swap(union perf_event *event,
>  	perf_event__attr_swap(&event->attr.attr);
>  
>  	size = event->header.size;
> -	size -= (void *)&event->attr.id - (void *)event;
> -	mem_bswap_64(event->attr.id, size);
> +	size -= perf_record_header_attr_id(event) - (void *)event;
> +	mem_bswap_64(perf_record_header_attr_id(event), size);

Should this and below also be a fix for stable?

>  }
>  
>  static void perf_event__event_update_swap(union perf_event *event,
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 45714a2785fd..a0579c7d7b9e 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2145,7 +2145,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *
>  		return -ENOMEM;
>  
>  	ev->attr.attr = *attr;
> -	memcpy(ev->attr.id, id, ids * sizeof(u64));
> +	memcpy(perf_record_header_attr_id(ev), id, ids * sizeof(u64));
>  
>  	ev->attr.header.type = PERF_RECORD_HEADER_ATTR;
>  	ev->attr.header.size = (u16)size;

Re: [PATCH v2 3/4] perf tools: Convert to perf_record_header_attr_id()

[Namhyung Kim]

Hi Adrian,

On Mon, Aug 28, 2023 at 12:59 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 25/08/23 18:25, Namhyung Kim wrote:
> > Instead of accessing the attr.id directly, use the
> > perf_record_header_attr_id() helper to handle old versions.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/header.c           | 2 +-
> >  tools/perf/util/session.c          | 4 ++--
> >  tools/perf/util/synthetic-events.c | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index f89321cbfdee..63967c34157d 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -4408,7 +4408,7 @@ 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;
> > +     ids = perf_record_header_attr_id(event);
> >       for (i = 0; i < n_ids; i++) {
> >               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
> >       }
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 00d18c74c090..1e9aa8ed15b6 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -833,8 +833,8 @@ static void perf_event__hdr_attr_swap(union perf_event *event,
> >       perf_event__attr_swap(&event->attr.attr);
> >
> >       size = event->header.size;
> > -     size -= (void *)&event->attr.id - (void *)event;
> > -     mem_bswap_64(event->attr.id, size);
> > +     size -= perf_record_header_attr_id(event) - (void *)event;
> > +     mem_bswap_64(perf_record_header_attr_id(event), size);
>
> Should this and below also be a fix for stable?

Then it'd need the perf_record_header_attr_id() change as well.
Also I'm not sure if it's really needed for stable as it's a pipe output
(saved to a file though) so very unlikely to have a different endian.

Thanks,
Namhyung

>
> >  }
> >
> >  static void perf_event__event_update_swap(union perf_event *event,
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 45714a2785fd..a0579c7d7b9e 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -2145,7 +2145,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *
> >               return -ENOMEM;
> >
> >       ev->attr.attr = *attr;
> > -     memcpy(ev->attr.id, id, ids * sizeof(u64));
> > +     memcpy(perf_record_header_attr_id(ev), id, ids * sizeof(u64));
> >
> >       ev->attr.header.type = PERF_RECORD_HEADER_ATTR;
> >       ev->attr.header.size = (u16)size;
>

Re: [PATCH v2 3/4] perf tools: Convert to perf_record_header_attr_id()

[Adrian Hunter]

On 30/08/23 05:15, Namhyung Kim wrote:
> Hi Adrian,
> 
> On Mon, Aug 28, 2023 at 12:59 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 25/08/23 18:25, Namhyung Kim wrote:
>>> Instead of accessing the attr.id directly, use the
>>> perf_record_header_attr_id() helper to handle old versions.
>>>
>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>> ---
>>>  tools/perf/util/header.c           | 2 +-
>>>  tools/perf/util/session.c          | 4 ++--
>>>  tools/perf/util/synthetic-events.c | 2 +-
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>>> index f89321cbfdee..63967c34157d 100644
>>> --- a/tools/perf/util/header.c
>>> +++ b/tools/perf/util/header.c
>>> @@ -4408,7 +4408,7 @@ 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;
>>> +     ids = perf_record_header_attr_id(event);
>>>       for (i = 0; i < n_ids; i++) {
>>>               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
>>>       }
>>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>>> index 00d18c74c090..1e9aa8ed15b6 100644
>>> --- a/tools/perf/util/session.c
>>> +++ b/tools/perf/util/session.c
>>> @@ -833,8 +833,8 @@ static void perf_event__hdr_attr_swap(union perf_event *event,
>>>       perf_event__attr_swap(&event->attr.attr);
>>>
>>>       size = event->header.size;
>>> -     size -= (void *)&event->attr.id - (void *)event;
>>> -     mem_bswap_64(event->attr.id, size);
>>> +     size -= perf_record_header_attr_id(event) - (void *)event;
>>> +     mem_bswap_64(perf_record_header_attr_id(event), size);
>>
>> Should this and below also be a fix for stable?
> 
> Then it'd need the perf_record_header_attr_id() change as well.
> Also I'm not sure if it's really needed for stable as it's a pipe output
> (saved to a file though) so very unlikely to have a different endian.
> 
> Thanks,
> Namhyung

Ok

For the patch set:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> 
>>
>>>  }
>>>
>>>  static void perf_event__event_update_swap(union perf_event *event,
>>> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
>>> index 45714a2785fd..a0579c7d7b9e 100644
>>> --- a/tools/perf/util/synthetic-events.c
>>> +++ b/tools/perf/util/synthetic-events.c
>>> @@ -2145,7 +2145,7 @@ int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *
>>>               return -ENOMEM;
>>>
>>>       ev->attr.attr = *attr;
>>> -     memcpy(ev->attr.id, id, ids * sizeof(u64));
>>> +     memcpy(perf_record_header_attr_id(ev), id, ids * sizeof(u64));
>>>
>>>       ev->attr.header.type = PERF_RECORD_HEADER_ATTR;
>>>       ev->attr.header.size = (u16)size;
>>