Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230807061652.2492167-1-namhyung@kernel.org>
In-Reply-To: <20230807061652.2492167-1-namhyung@kernel.org>
From:   Ian Rogers <irogers@google.com>
Date:   Thu, 10 Aug 2023 13:56:48 -0700
Message-ID: <CAP-5=fXOjBUkxEPutpcNpe+V1+ADz2U9B3K8fjJy_L_6_nbxpQ@mail.gmail.com>
Subject: Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Aug 6, 2023 at 11:16=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
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 =3D (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 =3D PERF_RECORD_ATTR, .size =3D 168 =
},
>  128 byte: perf event attr { .size =3D 128, ... },
>   32 byte: event IDs [] =3D { 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 =3D PERF_RECORD_ATTR, .size =3D 168 =
},
>  136 byte: perf event attr { .size =3D 136, ... },
>   24 byte: event IDs [] =3D { 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>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  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 =3D *pevlist;
>
> @@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool=
 __maybe_unused,
>
>         evlist__add(evlist, evsel);
>
> -       ids =3D event->header.size;
> -       ids -=3D (void *)&event->attr.id - (void *)event;
> -       n_ids =3D ids / sizeof(u64);
> +       n_ids =3D event->header.size - sizeof(event->header) - event->att=
r.attr.size;
> +       n_ids =3D 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 =3D (void *)&event->attr.attr + event->attr.attr.size;
>         for (i =3D 0; i < n_ids; i++) {
> -               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ev=
ent->attr.id[i]);
> +               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, id=
s[i]);
>         }
>
>         return 0;
> --
> 2.41.0.640.ga95def55d0-goog
>