Message-ID: <CAP-5=fW8YMivp1JW8vRKh=OZtGVLPQ4v88z_2ae+cAVnr7RXoQ@mail.gmail.com>
Date: Tue, 23 Jul 2024 05:50:47 -0700
From: Ian Rogers <irogers@...gle.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Namhyung Kim <namhyung@...nel.org>, 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 Tue, Jul 23, 2024 at 2:38 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 22/07/24 21:04, Ian Rogers wrote:
> > 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.
>
> In C, NULL or zero is often used as a special value to mean
> no-value.  Optional callbacks that are NULL is also not remarkable.

Sure NULL means optional and has since the dawn of time, I don't see
this adding anything to the conversation. What is remarkable here is
passing around a collection of function pointers, then the caller of
the function pointer is supposed to check whether what you want to be
an optional value (I don't, nor does the existing code) function
pointer is present, and if not present the caller needs to know to
call another special function. This is complicated enough that the
function calls could use helper macros, e.g.:

#define CALL_TOOL_SAMPLE(tool, event, sample, evsel, machine) \
  if (tool->sample) \
    tool->sample(tool, event, sample, machine); \
  else \
    process_event_sample_stub(tool, event, sample, machine);

you need about 38 of them. Now if I'm a buildtin_cmd writer wanting to
know what gets called on tool, I need to check, do all callers of the
tool use the macro? Perhaps the user decided to do their own NULL test
and call a different function. Cases like tool->finished_round already
need to go to one of two different stubs. There is clearly more
cognitive load on the tool API user as now they must check all uses of
38 function pointers across the code base to understand the behavior
of the tool, they can't just rely on the function pointer in the
struct being the place the code will go. A simple function pointer
call has imo become spaghetti.

There is also the overhead checking the optional value - I've never
seen a C programmer wanting to do this in 30 years of doing paid C
development, the use of the function pointer was to avoid checking,
etc. But maybe the kernel does this and it's an existing kernel style?
I can't find it.

Thanks,
Ian

Powered by blists - more mailing lists