Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH v1 2/4] perf script: Add "struct machine" parameter to
 process_event callback
From:   Athira Rajeev <atrajeev@linux.vnet.ibm.com>
In-Reply-To: <20211201123334.679131-3-german.gomez@arm.com>
Date:   Thu, 2 Dec 2021 21:33:27 +0530
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org,
        Arnaldo Carvalho de Melo <acme@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
Content-Transfer-Encoding: quoted-printable
Message-Id: <D6A3A8F3-9946-44FE-A70F-42977C6F38A1@linux.vnet.ibm.com>
References: <20211201123334.679131-1-german.gomez@arm.com>
 <20211201123334.679131-3-german.gomez@arm.com>
To:     German Gomez <german.gomez@arm.com>
Precedence: bulk


> On 01-Dec-2021, at 6:03 PM, German Gomez <german.gomez@arm.com> wrote:
>=20
> 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.
>=20
> 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.
>=20
> 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(-)
>=20
> 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 =3D &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 =E2=80=9Cmachine=E2=80=9D to allow access =
to perf_env__arch and there by to get the =E2=80=9Carch=E2=80=9D value.
But can we use from evsel, like  "perf_env__arch(evsel__env(evsel))=E2=80=9D=
 to get arch value instead of including new parameter for =E2=80=9Cstruct =
machine=E2=80=9D ?

Thanks
Athira=20
> 	} else {
> 		process_event(scr, sample, evsel, &al, &addr_al, =
machine);
> 	}
> diff --git a/tools/perf/util/scripting-engines/trace-event-perl.c =
b/tools/perf/util/scripting-engines/trace-event-perl.c
> index 32a721b3e..6017c4660 100644
> --- a/tools/perf/util/scripting-engines/trace-event-perl.c
> +++ b/tools/perf/util/scripting-engines/trace-event-perl.c
> @@ -454,7 +454,8 @@ static void perl_process_event(union perf_event =
*event,
> 			       struct perf_sample *sample,
> 			       struct evsel *evsel,
> 			       struct addr_location *al,
> -			       struct addr_location *addr_al)
> +			       struct addr_location *addr_al,
> +			       struct machine *machine __maybe_unused)
> {
> 	scripting_context__update(scripting_context, event, sample, =
evsel, al, addr_al);
> 	perl_process_tracepoint(sample, evsel, al);
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c =
b/tools/perf/util/scripting-engines/trace-event-python.c
> index c0c010350..e164f8d00 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -708,7 +708,8 @@ static void regs_map(struct regs_dump *regs, =
uint64_t mask, char *bf, int size)
>=20
> static void set_regs_in_dict(PyObject *dict,
> 			     struct perf_sample *sample,
> -			     struct evsel *evsel)
> +			     struct evsel *evsel,
> +			     struct machine *machine __maybe_unused)
> {
> 	struct perf_event_attr *attr =3D &evsel->core.attr;
>=20
> @@ -776,6 +777,7 @@ static PyObject *get_perf_sample_dict(struct =
perf_sample *sample,
> 					 struct evsel *evsel,
> 					 struct addr_location *al,
> 					 struct addr_location *addr_al,
> +					 struct machine *machine,
> 					 PyObject *callchain)
> {
> 	PyObject *dict, *dict_sample, *brstack, *brstacksym;
> @@ -849,7 +851,7 @@ static PyObject *get_perf_sample_dict(struct =
perf_sample *sample,
> 			PyLong_FromUnsignedLongLong(sample->cyc_cnt));
> 	}
>=20
> -	set_regs_in_dict(dict, sample, evsel);
> +	set_regs_in_dict(dict, sample, evsel, machine);
>=20
> 	return dict;
> }
> @@ -857,7 +859,8 @@ static PyObject *get_perf_sample_dict(struct =
perf_sample *sample,
> static void python_process_tracepoint(struct perf_sample *sample,
> 				      struct evsel *evsel,
> 				      struct addr_location *al,
> -				      struct addr_location *addr_al)
> +				      struct addr_location *addr_al,
> +				      struct machine *machine)
> {
> 	struct tep_event *event =3D evsel->tp_format;
> 	PyObject *handler, *context, *t, *obj =3D NULL, *callchain;
> @@ -964,7 +967,7 @@ static void python_process_tracepoint(struct =
perf_sample *sample,
> 		PyTuple_SetItem(t, n++, dict);
>=20
> 	if (get_argument_count(handler) =3D=3D (int) n + 1) {
> -		all_entries_dict =3D get_perf_sample_dict(sample, evsel, =
al, addr_al,
> +		all_entries_dict =3D get_perf_sample_dict(sample, evsel, =
al, addr_al, machine,
> 			callchain);
> 		PyTuple_SetItem(t, n++,	all_entries_dict);
> 	} else {
> @@ -1366,7 +1369,8 @@ static int python_process_call_return(struct =
call_return *cr, u64 *parent_db_id,
> static void python_process_general_event(struct perf_sample *sample,
> 					 struct evsel *evsel,
> 					 struct addr_location *al,
> -					 struct addr_location *addr_al)
> +					 struct addr_location *addr_al,
> +					 struct machine *machine)
> {
> 	PyObject *handler, *t, *dict, *callchain;
> 	static char handler_name[64];
> @@ -1388,7 +1392,7 @@ static void python_process_general_event(struct =
perf_sample *sample,
>=20
> 	/* ip unwinding */
> 	callchain =3D python_process_callchain(sample, evsel, al);
> -	dict =3D get_perf_sample_dict(sample, evsel, al, addr_al, =
callchain);
> +	dict =3D get_perf_sample_dict(sample, evsel, al, addr_al, =
machine, callchain);
>=20
> 	PyTuple_SetItem(t, n++, dict);
> 	if (_PyTuple_Resize(&t, n) =3D=3D -1)
> @@ -1403,7 +1407,8 @@ static void python_process_event(union =
perf_event *event,
> 				 struct perf_sample *sample,
> 				 struct evsel *evsel,
> 				 struct addr_location *al,
> -				 struct addr_location *addr_al)
> +				 struct addr_location *addr_al,
> +				 struct machine *machine)
> {
> 	struct tables *tables =3D &tables_global;
>=20
> @@ -1411,14 +1416,14 @@ static void python_process_event(union =
perf_event *event,
>=20
> 	switch (evsel->core.attr.type) {
> 	case PERF_TYPE_TRACEPOINT:
> -		python_process_tracepoint(sample, evsel, al, addr_al);
> +		python_process_tracepoint(sample, evsel, al, addr_al, =
machine);
> 		break;
> 	/* Reserve for future process_hw/sw/raw APIs */
> 	default:
> 		if (tables->db_export_mode)
> 			db_export__sample(&tables->dbe, event, sample, =
evsel, al, addr_al);
> 		else
> -			python_process_general_event(sample, evsel, al, =
addr_al);
> +			python_process_general_event(sample, evsel, al, =
addr_al, machine);
> 	}
> }
>=20
> diff --git a/tools/perf/util/trace-event-scripting.c =
b/tools/perf/util/trace-event-scripting.c
> index 7172ca052..089a2c905 100644
> --- a/tools/perf/util/trace-event-scripting.c
> +++ b/tools/perf/util/trace-event-scripting.c
> @@ -51,7 +51,8 @@ static void process_event_unsupported(union =
perf_event *event __maybe_unused,
> 				      struct perf_sample *sample =
__maybe_unused,
> 				      struct evsel *evsel =
__maybe_unused,
> 				      struct addr_location *al =
__maybe_unused,
> -				      struct addr_location *addr_al =
__maybe_unused)
> +				      struct addr_location *addr_al =
__maybe_unused,
> +				      struct machine *machine =
__maybe_unused)
> {
> }
>=20
> diff --git a/tools/perf/util/trace-event.h =
b/tools/perf/util/trace-event.h
> index 640981105..6f5b1a6d5 100644
> --- a/tools/perf/util/trace-event.h
> +++ b/tools/perf/util/trace-event.h
> @@ -81,7 +81,8 @@ struct scripting_ops {
> 			       struct perf_sample *sample,
> 			       struct evsel *evsel,
> 			       struct addr_location *al,
> -			       struct addr_location *addr_al);
> +			       struct addr_location *addr_al,
> +			       struct machine *machine);
> 	void (*process_switch)(union perf_event *event,
> 			       struct perf_sample *sample,
> 			       struct machine *machine);
> --=20
> 2.25.1
>=20
>=20