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

[Ian Rogers]

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

That's the case after these changes, but not before.

> >
> > 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.

Agreed, but such things happen in builtin_cmd where the tool is
defined and presumably they know what they are doing. My objection is
to code in util mutating the tool as I want the tool to have
predictable behavior. As callers that take a tool can call fill in
defaults (not all) then the tool has to be mutable and I don't want
this to be the case.

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

I'd like this. The problem is initializing all the function pointers
and making such initialization robust to extra functions being added
to the tool API. It can be done in a long winded way but I couldn't
devise macro magic to do it. The other problem is around variables
like ordered_events that can't currently be const. The patches move us
closer to this being a possibility.

> > 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()

If they do this their function pointers will be clobbered and their
code won't behave as expected, which I'd expect to be easy to observe.
In C++ if you were to initialize memory and then use the memory for a
placement new to create an object which would call the constructor,
the expected behavior would be that the initialized memory's values
would get overridden. I see the use of _init and _exit in the code as
being our poor man replacements of constructors and destructors.

> > 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(...);

Multiplied by every stub, we'd probably need a helper function, how to
handle argument passing. There's nothing wrong with this as an idea
but I think of this code as trying to create a visitor pattern and
this is a visitor pattern with a hard time for the caller.

> > 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.

It is because the patch series gets rid of fill in defaults which is
why we have a mutable tool passed around. I don't think this is up for
debate as the patch series clearly goes from a non-const
tool to a const tool at the end. Changing perf_tool__init to make all
the function pointers NULL and then making every caller have to do a:

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

I think it is a less elegant solution at the end, it is also a large
and more invasive change. The various refactorings to make tool const,
changing the aux use of tool, etc. wouldn't be impacted by such a
change but I think it is out of scope for this patch series.

Thanks,
Ian