Message-ID: <05fa0449-4fd4-41ed-93e8-db825e48268f@intel.com>
Date: Mon, 22 Jul 2024 11:28:44 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>,
 John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>,
 James Clark <james.clark@....com>, Mike Leach <mike.leach@...aro.org>,
 Leo Yan <leo.yan@...ux.dev>, Suzuki K Poulose <suzuki.poulose@....com>,
 Yicong Yang <yangyicong@...ilicon.com>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Nick Terrell <terrelln@...com>, Nick Desaulniers <ndesaulniers@...gle.com>,
 Oliver Upton <oliver.upton@...ux.dev>,
 Anshuman Khandual <anshuman.khandual@....com>, Song Liu <song@...nel.org>,
 Ilkka Koskinen <ilkka@...amperecomputing.com>,
 Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
 Huacai Chen <chenhuacai@...nel.org>, Yanteng Si <siyanteng@...ngson.cn>,
 Sun Haiyong <sunhaiyong@...ngson.cn>, linux-kernel@...r.kernel.org,
 linux-perf-users@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6 00/27] Constify tool pointers

On 19/07/24 19:26, Ian Rogers wrote:
> On Fri, Jul 19, 2024 at 1:51 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
>>
>> On 18/07/24 03:59, Ian Rogers wrote:
>>> struct perf_tool provides a set of function pointers that are called
>>> through when processing perf data. To make filling the pointers less
>>> cumbersome, if they are NULL perf_tools__fill_defaults will add
>>> default do nothing implementations.
>>>
>>> This change refactors struct perf_tool to have an init function that
>>> provides the default implementation. The special use of NULL and
>>> perf_tools__fill_defaults are removed. As a consequence the tool
>>> pointers can then all be made const, which better reflects the
>>> behavior a particular perf command would expect of the tool and to
>>> some extent can reduce the cognitive load on someone working on a
>>> command.
>>>
>>> v6: Rebase adding Adrian's reviewed-by/tested-by and Leo's tested-by.
>>
>> The tags were really meant only for patch 1, the email that was replied to.
>>
>> But now for patches 2 and 3:
>>
>> Reviewed-by: Adrian Hunter <adrian.hunter@...el.com>
> 
> Sorry for that, you'd mentioned that pt and bts testing which is
> impacted by more than just patch 1.
> 
>> Looking at patches 4 to 25, they do not seem to offer any benefit.
>>
>> Instead of patch 26, presumably perf_tool__fill_defaults() could
>> be moved to __perf_session__new(), which perhaps would allow
>> patch 27 as it is.
> 
> What I'm trying to do in the series is make it so that the tool isn't
> mutated during its use by session. Ideally we'd be passing a const
> tool to session_new, that's not possible because there's a hack to fix
> ordered events and pipe mode in session__new:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/session.c?h=perf-tools-next#n275
> Imo, it isn't great to pass a tool to session__new where you say you
> want ordered events and then session just goes to change that for you.
> Altering that behavior was beyond the scope of this clean up, so tool
> is only const after session__new.

Seems like a separate issue.  Since the session is created
by __perf_session__new(), session->tool will always be a pointer
to a const tool once there is:

diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 7f69baeae7fb..7c8dd6956330 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -43,7 +43,7 @@ struct perf_session {
 	u64			one_mmap_offset;
 	struct ordered_events	ordered_events;
 	struct perf_data	*data;
-	struct perf_tool	*tool;
+	const struct perf_tool	*tool;
 	u64			bytes_transferred;
 	u64			bytes_compressed;
 	struct zstd_data	zstd_data;

> 
> The reason for doing this is to make it so that when I have a tool I
> can reason that nobody is doing things to change it under my feet.

It still can be changed by the caller of __perf_session__new(), since
the tool itself is not const.

Anything using container_of() like:

static int process_sample_event(const struct perf_tool *tool,
				union perf_event *event,
				struct perf_sample *sample,
				struct evsel *evsel,
				struct machine *machine)
{
	struct perf_script *scr = container_of(tool, struct perf_script, tool);

can then change scr->tool without even having to cast away const.

Really, 'tool' needs to be defined as const in the first place.

> My
> builtin_cmd is in charge of what the tool is rather than some code
> buried in util that thought it was going to do me a favor. The code is
> a refactor and so the benefit is intended to be for the developer and
> how they reason about the use of tool.

It creates another question though: since there is a lot of code
before perf_tool__init() is called, does the caller mistakenly
change tool before calling perf_tool__init()

> how they reason about the use of tool. We generally use _init
> functions rather than having _fill_defaults, so there is a consistency
> argument.

The caller does not need the "defaults", so why would it set them up.
The session could just as easily do:

	if (tool->cb)
		tool->cb(...);
	else
		cb_stub(...);

> I don't expect any impact in terms of performance... Moving
> perf_tool__fill_defaults to __perf_session__new had issues with the
> existing code where NULL would be written over a function pointer
> expecting the later fill_defaults to fix it up, doesn't address coding
> consistency where _init is the norm, and adds another reason the tool
> passed to session__new can't be const.

perf_tool__init() is not a steeping stone to making 'tool' a
const in the first place.

Powered by blists - more mailing lists