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;
Message-ID: <b2332dd5-e01e-dc61-e19c-55cf9a684ca2@intel.com>
Date:   Mon, 14 Aug 2023 10:11:40 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.14.0
Subject: Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR
To:     Namhyung Kim <namhyung@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>
Cc:     Ian Rogers <irogers@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org
References: <20230807061652.2492167-1-namhyung@kernel.org>
Content-Language: en-US
From:   Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <20230807061652.2492167-1-namhyung@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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