| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fU=5LxF0SKuAqVP+xtmdERCCgxh_mdpw5okMi1fmvpE+Q@mail.gmail.com> Date: Fri, 19 Jul 2024 09:26:57 -0700 From: Ian Rogers <irogers@...gle.com> To: Adrian Hunter <adrian.hunter@...el.com> Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>, James Clark <james.clark@....com>, Mike Leach <mike.leach@...aro.org>, Leo Yan <leo.yan@...ux.dev>, Suzuki K Poulose <suzuki.poulose@....com>, Yicong Yang <yangyicong@...ilicon.com>, Jonathan Cameron <jonathan.cameron@...wei.com>, Nick Terrell <terrelln@...com>, Nick Desaulniers <ndesaulniers@...gle.com>, Oliver Upton <oliver.upton@...ux.dev>, Anshuman Khandual <anshuman.khandual@....com>, Song Liu <song@...nel.org>, Ilkka Koskinen <ilkka@...amperecomputing.com>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, Huacai Chen <chenhuacai@...nel.org>, Yanteng Si <siyanteng@...ngson.cn>, Sun Haiyong <sunhaiyong@...ngson.cn>, linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, linux-arm-kernel@...ts.infradead.org Subject: Re: [PATCH v6 00/27] Constify tool pointers On Fri, Jul 19, 2024 at 1:51 AM Adrian Hunter <adrian.hunter@...el.com> wrote: > > On 18/07/24 03:59, Ian Rogers wrote: > > struct perf_tool provides a set of function pointers that are called > > through when processing perf data. To make filling the pointers less > > cumbersome, if they are NULL perf_tools__fill_defaults will add > > default do nothing implementations. > > > > This change refactors struct perf_tool to have an init function that > > provides the default implementation. The special use of NULL and > > perf_tools__fill_defaults are removed. As a consequence the tool > > pointers can then all be made const, which better reflects the > > behavior a particular perf command would expect of the tool and to > > some extent can reduce the cognitive load on someone working on a > > command. > > > > v6: Rebase adding Adrian's reviewed-by/tested-by and Leo's tested-by. > > The tags were really meant only for patch 1, the email that was replied to. > > But now for patches 2 and 3: > > Reviewed-by: Adrian Hunter <adrian.hunter@...el.com> Sorry for that, you'd mentioned that pt and bts testing which is impacted by more than just patch 1. > Looking at patches 4 to 25, they do not seem to offer any benefit. > > Instead of patch 26, presumably perf_tool__fill_defaults() could > be moved to __perf_session__new(), which perhaps would allow > patch 27 as it is. What I'm trying to do in the series is make it so that the tool isn't mutated during its use by session. Ideally we'd be passing a const tool to session_new, that's not possible because there's a hack to fix ordered events and pipe mode in session__new: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/session.c?h=perf-tools-next#n275 Imo, it isn't great to pass a tool to session__new where you say you want ordered events and then session just goes to change that for you. Altering that behavior was beyond the scope of this clean up, so tool is only const after session__new. 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 > > >
Powered by blists - more mailing lists