Re: [PATCH v1 2/4] perf script: Add "struct machine" parameter to process_event callback

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: German Gomez <german.gomez@arm.com>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-perf-users@vger.kernel.org,
	John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Leo Yan <leo.yan@linaro.org>, Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	linux-arm-kernel@lists.infradead.org, linux-csky@vger.kernel.org,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH v1 2/4] perf script: Add "struct machine" parameter to process_event callback
Date: Mon, 13 Dec 2021 15:22:52 -0300	[thread overview]
Message-ID: <YbePfOMaiJjoDb3b@kernel.org> (raw)
In-Reply-To: <38fd4992-63ae-4871-ddfd-27d40b5c48d2@arm.com>

Em Fri, Dec 03, 2021 at 12:00:26PM +0000, German Gomez escreveu:
> Hi Athira,
> 
> On 02/12/2021 16:03, Athira Rajeev wrote:
> >
> >> On 01-Dec-2021, at 6:03 PM, German Gomez <german.gomez@arm.com> wrote:
> >>
> >> Include a "struct machine*" parameter to the process_event callback in
> >> the scripting layer. This will allow access to the perf_env from within
> >> this callback.
> >>
> >> Followup patches will build on top of this to report the correct name of
> >> the registers in a perf.data file, consistently with the architecture
> >> the file was recorded in.
> >>
> >> Signed-off-by: German Gomez <german.gomez@arm.com>
> >> ---
> >> tools/perf/builtin-script.c                   |  2 +-
> >> .../util/scripting-engines/trace-event-perl.c |  3 ++-
> >> .../scripting-engines/trace-event-python.c    | 23 +++++++++++--------
> >> tools/perf/util/trace-event-scripting.c       |  3 ++-
> >> tools/perf/util/trace-event.h                 |  3 ++-
> >> 5 files changed, 21 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> >> index 9434367af..711132f0b 100644
> >> --- a/tools/perf/builtin-script.c
> >> +++ b/tools/perf/builtin-script.c
> >> @@ -2256,7 +2256,7 @@ static int process_sample_event(struct perf_tool *tool,
> >> 				thread__resolve(al.thread, &addr_al, sample);
> >> 			addr_al_ptr = &addr_al;
> >> 		}
> >> -		scripting_ops->process_event(event, sample, evsel, &al, addr_al_ptr);
> >> +		scripting_ops->process_event(event, sample, evsel, &al, addr_al_ptr, machine);
> > Hi,
> >
> > Looks like the patch is using “machine” to allow access to perf_env__arch and there by to get the “arch” value.
> > But can we use from evsel, like  "perf_env__arch(evsel__env(evsel))” to get arch value instead of including new parameter for “struct machine” ?
> >
> > Thanks
> > Athira
> 
> Thanks for the suggestion. It looks like we can skip this patch if we
> can get the arch value that way.

So the first one is already upstream, will you post a v2?

- Arnaldo