| Message ID | 20240626203630.1194748-1-irogers@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Constify tool pointers | expand |
On Wed, Jun 26, 2024 at 01:36:02PM -0700, 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. I thought you actually wanted to make the tool const (rodata) but it seems you leave it as is but treat it as const. I'm curious if we can change the event delivery code something like: if (tool->func) tool->func(...); else stub_func(...); Then probably we don't need to touch the tool and make it const. Thoughts? Thanks, Namhyung > > 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 auxevent: Zero size dummy tool > perf cs-etm: Fix address sanitizer dso build failure > 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 | 14 +- > 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 | 24 +- > tools/perf/util/data-convert-bt.c | 34 ++- > tools/perf/util/data-convert-json.c | 47 ++-- > tools/perf/util/dso.h | 10 + > 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 | 14 +- > tools/perf/util/intel-pt.c | 15 +- > tools/perf/util/jitdump.c | 4 +- > tools/perf/util/s390-cpumsf.c | 11 +- > tools/perf/util/session.c | 342 ++-------------------------- > tools/perf/util/session.h | 6 +- > 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 +- > 54 files changed, 967 insertions(+), 1001 deletions(-) > create mode 100644 tools/perf/util/tool.c > > -- > 2.45.2.741.gdbec12cfda-goog >
On Fri, Jun 28, 2024 at 10:25 AM Namhyung Kim <namhyung@kernel.org> wrote: > > On Wed, Jun 26, 2024 at 01:36:02PM -0700, 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. > > I thought you actually wanted to make the tool const (rodata) but it > seems you leave it as is but treat it as const. So I think that is a next step on top of these changes but it would need something a bit special as we want to default initialize some fields but then initialize others. Something like (which wouldn't work): .tool = DEFAULT_TOOL_STUBS({ .sample = process_sample_event, .fork = perf_event__process_fork, .exit = perf_event__process_exit, .comm = perf_event__process_comm, .namespaces = perf_event__process_namespaces, .mmap = build_id__process_mmap, .mmap2 = build_id__process_mmap2, .itrace_start = process_timestamp_boundary, .aux = process_timestamp_boundary}) Being const is just saying hey all these event callbacks aren't going to mutate the tool, something I wanted to rule out as part of a change I'm working on. > I'm curious if we can change the event delivery code something like: > > if (tool->func) > tool->func(...); > else > stub_func(...); > > Then probably we don't need to touch the tool and make it const. > Thoughts? It works but the approach needs to change all tool func callers. I think it is also more obvious as an API to have a default value and override it, rather than giving special properties to NULL that callers should adhere to - we're doing a kind of poor man's virtual method dispatch and you wouldn't typically expect a NULL check as part of that. Thanks, Ian > Thanks, > Namhyung
On Fri, Jun 28, 2024 at 10:52 AM Ian Rogers <irogers@google.com> wrote: > > On Fri, Jun 28, 2024 at 10:25 AM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Wed, Jun 26, 2024 at 01:36:02PM -0700, 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. > > > > I thought you actually wanted to make the tool const (rodata) but it > > seems you leave it as is but treat it as const. > > So I think that is a next step on top of these changes but it would > need something a bit special as we want to default initialize some > fields but then initialize others. Something like (which wouldn't > work): > > .tool = DEFAULT_TOOL_STUBS({ > .sample = process_sample_event, > .fork = perf_event__process_fork, > .exit = perf_event__process_exit, > .comm = perf_event__process_comm, > .namespaces = perf_event__process_namespaces, > .mmap = build_id__process_mmap, > .mmap2 = build_id__process_mmap2, > .itrace_start = process_timestamp_boundary, > .aux = process_timestamp_boundary}) > > Being const is just saying hey all these event callbacks aren't going > to mutate the tool, something I wanted to rule out as part of a change > I'm working on. > > > I'm curious if we can change the event delivery code something like: > > > > if (tool->func) > > tool->func(...); > > else > > stub_func(...); > > > > Then probably we don't need to touch the tool and make it const. > > Thoughts? > > It works but the approach needs to change all tool func callers. I > think it is also more obvious as an API to have a default value and > override it, rather than giving special properties to NULL that > callers should adhere to - we're doing a kind of poor man's virtual > method dispatch and you wouldn't typically expect a NULL check as part > of that. I guess we only have a few callers in util/session.c. Thanks, Namhyung
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. 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 auxevent: Zero size dummy tool perf cs-etm: Fix address sanitizer dso build failure 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 | 14 +- 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 | 24 +- tools/perf/util/data-convert-bt.c | 34 ++- tools/perf/util/data-convert-json.c | 47 ++-- tools/perf/util/dso.h | 10 + 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 | 14 +- tools/perf/util/intel-pt.c | 15 +- tools/perf/util/jitdump.c | 4 +- tools/perf/util/s390-cpumsf.c | 11 +- tools/perf/util/session.c | 342 ++-------------------------- tools/perf/util/session.h | 6 +- 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 +- 54 files changed, 967 insertions(+), 1001 deletions(-) create mode 100644 tools/perf/util/tool.c