Message-ID: <CAP-5=fW4LmOMo6fD9d9Ymc9stDttxQv-Pjg-=Xeq41-LGumwSA@mail.gmail.com>
Date: Mon, 22 Jul 2024 11:04:57 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Adrian Hunter <adrian.hunter@...el.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...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 Mon, Jul 22, 2024 at 10:50 AM Ian Rogers <irogers@...gle.com> wrote:
>
> On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
> > > >
> > > > 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;
> > >
> > > 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.
> >
> > I don't think it's a large and invasive change.  The tools are mostly
> > zero-initialized so we don't need to reset to NULL.  And tool->cb is
> > called mostly from two functions: machines__deliver_event() and
> > perf_session__process_user_event().  Can we change them to check
> > NULL and get rid of perf_tool__fill_defaults() to keep it const?
>
> As I said above, I don't think that is good style and is out of scope
> here. It clearly can be done as follow up, but I don't see how that
> fixes the style issue.

Just to be clear on what the style issue is. We already have code:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-record.c?h=perf-tools-next#n1461
```
if (rec->buildid_all && !rec->timestamp_boundary)
    rec->tool.sample = NULL;
```
that relies on the special behavior of NULL in a function pointer
being changed at dispatch time - a simple reading of that code would
be anyone calling the function pointer would get a segv. I'm trying to
make it so that NULL isn't magic in the context of tool and you can
simply look at the tool to understand what its behavior is, much as a
virtual method table would work if we could do proper object-oriented
development.

Thanks,
Ian

> Thanks,
> Ian

Powered by blists - more mailing lists