[PATCH v6 00/27] Constify tool pointers

Adrian Hunter adrian.hunter at intel.com
Mon Jul 22 01:28:44 PDT 2024


On 19/07/24 19:26, Ian Rogers wrote:
> On Fri, Jul 19, 2024 at 1:51 AM Adrian Hunter <adrian.hunter at intel.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 at intel.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.




More information about the linux-arm-kernel mailing list