| Message ID | 20240718010023.1495687-1-irogers@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Constify tool pointers | expand |
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> 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. > v5: Rebase dropping asan fix merged by Namhyung. > v4: Simplify perf_session__deliver_synth_attr_event following Adrian's > suggestions. > v3: Just remove auxtrace dummy tools [Adrian] and make s390-cpumsf > struct removal its own patch [Adrian]. > v2: Remove dummy tool initialization [Adrian] and make zero sized. Add > cs-etm fix for address sanitizer build, found necessary when > testing dummy tool change. > > Ian Rogers (27): > perf auxtrace: Remove dummy tools > perf s390-cpumsf: Remove unused struct > perf tool: Constify tool pointers > perf tool: Move fill defaults into tool.c > perf tool: Add perf_tool__init > perf kmem: Use perf_tool__init > perf buildid-list: Use perf_tool__init > perf kvm: Use perf_tool__init > perf lock: Use perf_tool__init > perf evlist: Use perf_tool__init > perf record: Use perf_tool__init > perf c2c: Use perf_tool__init > perf script: Use perf_tool__init > perf inject: Use perf_tool__init > perf report: Use perf_tool__init > perf stat: Use perf_tool__init > perf annotate: Use perf_tool__init > perf sched: Use perf_tool__init > perf mem: Use perf_tool__init > perf timechart: Use perf_tool__init > perf diff: Use perf_tool__init > perf data convert json: Use perf_tool__init > perf data convert ctf: Use perf_tool__init > perf test event_update: Ensure tools is initialized > perf kwork: Use perf_tool__init > perf tool: Remove perf_tool__fill_defaults > perf session: Constify tool > > tools/perf/arch/x86/util/event.c | 4 +- > tools/perf/bench/synthesize.c | 2 +- > tools/perf/builtin-annotate.c | 44 ++-- > tools/perf/builtin-buildid-list.c | 10 + > tools/perf/builtin-c2c.c | 33 ++- > tools/perf/builtin-diff.c | 30 ++- > tools/perf/builtin-evlist.c | 10 +- > tools/perf/builtin-inject.c | 159 ++++++------ > tools/perf/builtin-kmem.c | 20 +- > tools/perf/builtin-kvm.c | 19 +- > tools/perf/builtin-kwork.c | 33 ++- > tools/perf/builtin-lock.c | 41 ++-- > tools/perf/builtin-mem.c | 37 +-- > tools/perf/builtin-record.c | 47 ++-- > tools/perf/builtin-report.c | 67 +++-- > tools/perf/builtin-sched.c | 50 ++-- > tools/perf/builtin-script.c | 106 ++++---- > tools/perf/builtin-stat.c | 26 +- > tools/perf/builtin-timechart.c | 25 +- > tools/perf/builtin-top.c | 2 +- > tools/perf/builtin-trace.c | 4 +- > tools/perf/tests/cpumap.c | 6 +- > tools/perf/tests/dlfilter-test.c | 2 +- > tools/perf/tests/dwarf-unwind.c | 2 +- > tools/perf/tests/event_update.c | 9 +- > tools/perf/tests/stat.c | 6 +- > tools/perf/tests/thread-map.c | 2 +- > tools/perf/util/Build | 1 + > tools/perf/util/arm-spe.c | 55 +---- > tools/perf/util/auxtrace.c | 12 +- > tools/perf/util/auxtrace.h | 20 +- > tools/perf/util/bpf-event.c | 4 +- > tools/perf/util/build-id.c | 34 +-- > tools/perf/util/build-id.h | 8 +- > tools/perf/util/cs-etm.c | 39 +-- > tools/perf/util/data-convert-bt.c | 34 ++- > tools/perf/util/data-convert-json.c | 47 ++-- > tools/perf/util/event.c | 54 ++-- > tools/perf/util/event.h | 38 +-- > tools/perf/util/header.c | 6 +- > tools/perf/util/header.h | 4 +- > tools/perf/util/hisi-ptt.c | 6 +- > tools/perf/util/intel-bts.c | 37 +-- > tools/perf/util/intel-pt.c | 30 +-- > tools/perf/util/jitdump.c | 4 +- > tools/perf/util/s390-cpumsf.c | 11 +- > tools/perf/util/session.c | 366 +++------------------------- > tools/perf/util/session.h | 9 +- > tools/perf/util/synthetic-events.c | 80 +++--- > tools/perf/util/synthetic-events.h | 70 +++--- > tools/perf/util/tool.c | 294 ++++++++++++++++++++++ > tools/perf/util/tool.h | 18 +- > tools/perf/util/tsc.c | 2 +- > 53 files changed, 977 insertions(+), 1102 deletions(-) > create mode 100644 tools/perf/util/tool.c >
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. 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. 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. We generally use _init functions rather than having _fill_defaults, so there is a consistency argument. 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. Thanks, Ian > > v5: Rebase dropping asan fix merged by Namhyung. > > v4: Simplify perf_session__deliver_synth_attr_event following Adrian's > > suggestions. > > v3: Just remove auxtrace dummy tools [Adrian] and make s390-cpumsf > > struct removal its own patch [Adrian]. > > v2: Remove dummy tool initialization [Adrian] and make zero sized. Add > > cs-etm fix for address sanitizer build, found necessary when > > testing dummy tool change. > > > > Ian Rogers (27): > > perf auxtrace: Remove dummy tools > > perf s390-cpumsf: Remove unused struct > > perf tool: Constify tool pointers > > perf tool: Move fill defaults into tool.c > > perf tool: Add perf_tool__init > > perf kmem: Use perf_tool__init > > perf buildid-list: Use perf_tool__init > > perf kvm: Use perf_tool__init > > perf lock: Use perf_tool__init > > perf evlist: Use perf_tool__init > > perf record: Use perf_tool__init > > perf c2c: Use perf_tool__init > > perf script: Use perf_tool__init > > perf inject: Use perf_tool__init > > perf report: Use perf_tool__init > > perf stat: Use perf_tool__init > > perf annotate: Use perf_tool__init > > perf sched: Use perf_tool__init > > perf mem: Use perf_tool__init > > perf timechart: Use perf_tool__init > > perf diff: Use perf_tool__init > > perf data convert json: Use perf_tool__init > > perf data convert ctf: Use perf_tool__init > > perf test event_update: Ensure tools is initialized > > perf kwork: Use perf_tool__init > > perf tool: Remove perf_tool__fill_defaults > > perf session: Constify tool > > > > tools/perf/arch/x86/util/event.c | 4 +- > > tools/perf/bench/synthesize.c | 2 +- > > tools/perf/builtin-annotate.c | 44 ++-- > > tools/perf/builtin-buildid-list.c | 10 + > > tools/perf/builtin-c2c.c | 33 ++- > > tools/perf/builtin-diff.c | 30 ++- > > tools/perf/builtin-evlist.c | 10 +- > > tools/perf/builtin-inject.c | 159 ++++++------ > > tools/perf/builtin-kmem.c | 20 +- > > tools/perf/builtin-kvm.c | 19 +- > > tools/perf/builtin-kwork.c | 33 ++- > > tools/perf/builtin-lock.c | 41 ++-- > > tools/perf/builtin-mem.c | 37 +-- > > tools/perf/builtin-record.c | 47 ++-- > > tools/perf/builtin-report.c | 67 +++-- > > tools/perf/builtin-sched.c | 50 ++-- > > tools/perf/builtin-script.c | 106 ++++---- > > tools/perf/builtin-stat.c | 26 +- > > tools/perf/builtin-timechart.c | 25 +- > > tools/perf/builtin-top.c | 2 +- > > tools/perf/builtin-trace.c | 4 +- > > tools/perf/tests/cpumap.c | 6 +- > > tools/perf/tests/dlfilter-test.c | 2 +- > > tools/perf/tests/dwarf-unwind.c | 2 +- > > tools/perf/tests/event_update.c | 9 +- > > tools/perf/tests/stat.c | 6 +- > > tools/perf/tests/thread-map.c | 2 +- > > tools/perf/util/Build | 1 + > > tools/perf/util/arm-spe.c | 55 +---- > > tools/perf/util/auxtrace.c | 12 +- > > tools/perf/util/auxtrace.h | 20 +- > > tools/perf/util/bpf-event.c | 4 +- > > tools/perf/util/build-id.c | 34 +-- > > tools/perf/util/build-id.h | 8 +- > > tools/perf/util/cs-etm.c | 39 +-- > > tools/perf/util/data-convert-bt.c | 34 ++- > > tools/perf/util/data-convert-json.c | 47 ++-- > > tools/perf/util/event.c | 54 ++-- > > tools/perf/util/event.h | 38 +-- > > tools/perf/util/header.c | 6 +- > > tools/perf/util/header.h | 4 +- > > tools/perf/util/hisi-ptt.c | 6 +- > > tools/perf/util/intel-bts.c | 37 +-- > > tools/perf/util/intel-pt.c | 30 +-- > > tools/perf/util/jitdump.c | 4 +- > > tools/perf/util/s390-cpumsf.c | 11 +- > > tools/perf/util/session.c | 366 +++------------------------- > > tools/perf/util/session.h | 9 +- > > tools/perf/util/synthetic-events.c | 80 +++--- > > tools/perf/util/synthetic-events.h | 70 +++--- > > tools/perf/util/tool.c | 294 ++++++++++++++++++++++ > > tools/perf/util/tool.h | 18 +- > > tools/perf/util/tsc.c | 2 +- > > 53 files changed, 977 insertions(+), 1102 deletions(-) > > create mode 100644 tools/perf/util/tool.c > > >
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.
On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > 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; 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
Hi, On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@google.com> wrote: > > On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > 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; > > 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? Thanks, Namhyung
On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim <namhyung@kernel.org> wrote: > > Hi, > > On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@google.com> wrote: > > > > On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > > > 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; > > > > 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. Thanks, Ian
On Mon, Jul 22, 2024 at 10:50 AM Ian Rogers <irogers@google.com> wrote: > > On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Hi, > > > > On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@google.com> wrote: > > > > > > On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > > > > > 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; > > > > > > 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
On 22/07/24 21:04, Ian Rogers wrote: > On Mon, Jul 22, 2024 at 10:50 AM Ian Rogers <irogers@google.com> wrote: >> >> On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim <namhyung@kernel.org> wrote: >>> >>> Hi, >>> >>> On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@google.com> wrote: >>>> >>>> On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> >>>>> 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; >>>> >>>> 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.
On Tue, Jul 23, 2024 at 2:38 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 22/07/24 21:04, Ian Rogers wrote: > > On Mon, Jul 22, 2024 at 10:50 AM Ian Rogers <irogers@google.com> wrote: > >> > >> On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim <namhyung@kernel.org> wrote: > >>> > >>> Hi, > >>> > >>> On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers <irogers@google.com> wrote: > >>>> > >>>> On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > >>>>> > >>>>> 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; > >>>> > >>>> 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
On Fri, Jul 19, 2024 at 09:26:57AM -0700, 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> Applied 1-3, 4 is not applying, I'll look at it later. - Arnaldo > > 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. > > 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. 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. We generally use _init > functions rather than having _fill_defaults, so there is a consistency > argument. 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. > > Thanks, > Ian > > > > v5: Rebase dropping asan fix merged by Namhyung. > > > v4: Simplify perf_session__deliver_synth_attr_event following Adrian's > > > suggestions. > > > v3: Just remove auxtrace dummy tools [Adrian] and make s390-cpumsf > > > struct removal its own patch [Adrian]. > > > v2: Remove dummy tool initialization [Adrian] and make zero sized. Add > > > cs-etm fix for address sanitizer build, found necessary when > > > testing dummy tool change. > > > > > > Ian Rogers (27): > > > perf auxtrace: Remove dummy tools > > > perf s390-cpumsf: Remove unused struct > > > perf tool: Constify tool pointers > > > perf tool: Move fill defaults into tool.c > > > perf tool: Add perf_tool__init > > > perf kmem: Use perf_tool__init > > > perf buildid-list: Use perf_tool__init > > > perf kvm: Use perf_tool__init > > > perf lock: Use perf_tool__init > > > perf evlist: Use perf_tool__init > > > perf record: Use perf_tool__init > > > perf c2c: Use perf_tool__init > > > perf script: Use perf_tool__init > > > perf inject: Use perf_tool__init > > > perf report: Use perf_tool__init > > > perf stat: Use perf_tool__init > > > perf annotate: Use perf_tool__init > > > perf sched: Use perf_tool__init > > > perf mem: Use perf_tool__init > > > perf timechart: Use perf_tool__init > > > perf diff: Use perf_tool__init > > > perf data convert json: Use perf_tool__init > > > perf data convert ctf: Use perf_tool__init > > > perf test event_update: Ensure tools is initialized > > > perf kwork: Use perf_tool__init > > > perf tool: Remove perf_tool__fill_defaults > > > perf session: Constify tool > > > > > > tools/perf/arch/x86/util/event.c | 4 +- > > > tools/perf/bench/synthesize.c | 2 +- > > > tools/perf/builtin-annotate.c | 44 ++-- > > > tools/perf/builtin-buildid-list.c | 10 + > > > tools/perf/builtin-c2c.c | 33 ++- > > > tools/perf/builtin-diff.c | 30 ++- > > > tools/perf/builtin-evlist.c | 10 +- > > > tools/perf/builtin-inject.c | 159 ++++++------ > > > tools/perf/builtin-kmem.c | 20 +- > > > tools/perf/builtin-kvm.c | 19 +- > > > tools/perf/builtin-kwork.c | 33 ++- > > > tools/perf/builtin-lock.c | 41 ++-- > > > tools/perf/builtin-mem.c | 37 +-- > > > tools/perf/builtin-record.c | 47 ++-- > > > tools/perf/builtin-report.c | 67 +++-- > > > tools/perf/builtin-sched.c | 50 ++-- > > > tools/perf/builtin-script.c | 106 ++++---- > > > tools/perf/builtin-stat.c | 26 +- > > > tools/perf/builtin-timechart.c | 25 +- > > > tools/perf/builtin-top.c | 2 +- > > > tools/perf/builtin-trace.c | 4 +- > > > tools/perf/tests/cpumap.c | 6 +- > > > tools/perf/tests/dlfilter-test.c | 2 +- > > > tools/perf/tests/dwarf-unwind.c | 2 +- > > > tools/perf/tests/event_update.c | 9 +- > > > tools/perf/tests/stat.c | 6 +- > > > tools/perf/tests/thread-map.c | 2 +- > > > tools/perf/util/Build | 1 + > > > tools/perf/util/arm-spe.c | 55 +---- > > > tools/perf/util/auxtrace.c | 12 +- > > > tools/perf/util/auxtrace.h | 20 +- > > > tools/perf/util/bpf-event.c | 4 +- > > > tools/perf/util/build-id.c | 34 +-- > > > tools/perf/util/build-id.h | 8 +- > > > tools/perf/util/cs-etm.c | 39 +-- > > > tools/perf/util/data-convert-bt.c | 34 ++- > > > tools/perf/util/data-convert-json.c | 47 ++-- > > > tools/perf/util/event.c | 54 ++-- > > > tools/perf/util/event.h | 38 +-- > > > tools/perf/util/header.c | 6 +- > > > tools/perf/util/header.h | 4 +- > > > tools/perf/util/hisi-ptt.c | 6 +- > > > tools/perf/util/intel-bts.c | 37 +-- > > > tools/perf/util/intel-pt.c | 30 +-- > > > tools/perf/util/jitdump.c | 4 +- > > > tools/perf/util/s390-cpumsf.c | 11 +- > > > tools/perf/util/session.c | 366 +++------------------------- > > > tools/perf/util/session.h | 9 +- > > > tools/perf/util/synthetic-events.c | 80 +++--- > > > tools/perf/util/synthetic-events.h | 70 +++--- > > > tools/perf/util/tool.c | 294 ++++++++++++++++++++++ > > > tools/perf/util/tool.h | 18 +- > > > tools/perf/util/tsc.c | 2 +- > > > 53 files changed, 977 insertions(+), 1102 deletions(-) > > > create mode 100644 tools/perf/util/tool.c > > > > >
On Mon, Aug 12, 2024 at 6:53 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > On Fri, Jul 19, 2024 at 09:26:57AM -0700, 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> > > Applied 1-3, 4 is not applying, I'll look at it later. I have a rebase and can resend. I haven't addressed Adrian's feedback as I prefer callers of the tool function pointers not to have to do NULL tests, I'm trying to minimize spaghetti. Not sure if you're applying the whole series here or just the beginning. Thanks, Ian
On Mon, Aug 12, 2024 at 11:10:02AM -0700, Ian Rogers wrote: > On Mon, Aug 12, 2024 at 6:53 AM Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: > > > > On Fri, Jul 19, 2024 at 09:26:57AM -0700, 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> > > > > Applied 1-3, 4 is not applying, I'll look at it later. > > I have a rebase and can resend. I haven't addressed Adrian's feedback Please resend then. - Arnaldo > as I prefer callers of the tool function pointers not to have to do > NULL tests, I'm trying to minimize spaghetti. Not sure if you're > applying the whole series here or just the beginning. > > Thanks, > Ian
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. v5: Rebase dropping asan fix merged by Namhyung. v4: Simplify perf_session__deliver_synth_attr_event following Adrian's suggestions. v3: Just remove auxtrace dummy tools [Adrian] and make s390-cpumsf struct removal its own patch [Adrian]. v2: Remove dummy tool initialization [Adrian] and make zero sized. Add cs-etm fix for address sanitizer build, found necessary when testing dummy tool change. Ian Rogers (27): perf auxtrace: Remove dummy tools perf s390-cpumsf: Remove unused struct perf tool: Constify tool pointers perf tool: Move fill defaults into tool.c perf tool: Add perf_tool__init perf kmem: Use perf_tool__init perf buildid-list: Use perf_tool__init perf kvm: Use perf_tool__init perf lock: Use perf_tool__init perf evlist: Use perf_tool__init perf record: Use perf_tool__init perf c2c: Use perf_tool__init perf script: Use perf_tool__init perf inject: Use perf_tool__init perf report: Use perf_tool__init perf stat: Use perf_tool__init perf annotate: Use perf_tool__init perf sched: Use perf_tool__init perf mem: Use perf_tool__init perf timechart: Use perf_tool__init perf diff: Use perf_tool__init perf data convert json: Use perf_tool__init perf data convert ctf: Use perf_tool__init perf test event_update: Ensure tools is initialized perf kwork: Use perf_tool__init perf tool: Remove perf_tool__fill_defaults perf session: Constify tool tools/perf/arch/x86/util/event.c | 4 +- tools/perf/bench/synthesize.c | 2 +- tools/perf/builtin-annotate.c | 44 ++-- tools/perf/builtin-buildid-list.c | 10 + tools/perf/builtin-c2c.c | 33 ++- tools/perf/builtin-diff.c | 30 ++- tools/perf/builtin-evlist.c | 10 +- tools/perf/builtin-inject.c | 159 ++++++------ tools/perf/builtin-kmem.c | 20 +- tools/perf/builtin-kvm.c | 19 +- tools/perf/builtin-kwork.c | 33 ++- tools/perf/builtin-lock.c | 41 ++-- tools/perf/builtin-mem.c | 37 +-- tools/perf/builtin-record.c | 47 ++-- tools/perf/builtin-report.c | 67 +++-- tools/perf/builtin-sched.c | 50 ++-- tools/perf/builtin-script.c | 106 ++++---- tools/perf/builtin-stat.c | 26 +- tools/perf/builtin-timechart.c | 25 +- tools/perf/builtin-top.c | 2 +- tools/perf/builtin-trace.c | 4 +- tools/perf/tests/cpumap.c | 6 +- tools/perf/tests/dlfilter-test.c | 2 +- tools/perf/tests/dwarf-unwind.c | 2 +- tools/perf/tests/event_update.c | 9 +- tools/perf/tests/stat.c | 6 +- tools/perf/tests/thread-map.c | 2 +- tools/perf/util/Build | 1 + tools/perf/util/arm-spe.c | 55 +---- tools/perf/util/auxtrace.c | 12 +- tools/perf/util/auxtrace.h | 20 +- tools/perf/util/bpf-event.c | 4 +- tools/perf/util/build-id.c | 34 +-- tools/perf/util/build-id.h | 8 +- tools/perf/util/cs-etm.c | 39 +-- tools/perf/util/data-convert-bt.c | 34 ++- tools/perf/util/data-convert-json.c | 47 ++-- tools/perf/util/event.c | 54 ++-- tools/perf/util/event.h | 38 +-- tools/perf/util/header.c | 6 +- tools/perf/util/header.h | 4 +- tools/perf/util/hisi-ptt.c | 6 +- tools/perf/util/intel-bts.c | 37 +-- tools/perf/util/intel-pt.c | 30 +-- tools/perf/util/jitdump.c | 4 +- tools/perf/util/s390-cpumsf.c | 11 +- tools/perf/util/session.c | 366 +++------------------------- tools/perf/util/session.h | 9 +- tools/perf/util/synthetic-events.c | 80 +++--- tools/perf/util/synthetic-events.h | 70 +++--- tools/perf/util/tool.c | 294 ++++++++++++++++++++++ tools/perf/util/tool.h | 18 +- tools/perf/util/tsc.c | 2 +- 53 files changed, 977 insertions(+), 1102 deletions(-) create mode 100644 tools/perf/util/tool.c