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

[Adrian Hunter]

On 22/07/24 21:04, Ian Rogers wrote:
> On Mon, Jul 22, 2024 at 10:50 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>>
>> On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>>>
>>> Hi,
>>>
>>> On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>>>>
>>>> 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.
>>>
>>> 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.