Re: [PATCH v6 00/27] Constify tool pointers

Adrian Hunter
2024-07-22T08:29:16Z

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