From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Darren Hart <dvhart@infradead.org>,
Davidlohr Bueso <dave@stgolabs.net>,
James Clark <james.clark@arm.com>,
John Garry <john.g.garry@oracle.com>,
Riccardo Mancini <rickyman7@gmail.com>,
Yury Norov <yury.norov@gmail.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Andrew Morton <akpm@linux-foundation.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Leo Yan <leo.yan@linaro.org>, Andi Kleen <ak@linux.intel.com>,
Thomas Richter <tmricht@linux.ibm.com>,
Kan Liang <kan.liang@linux.intel.com>,
Madhavan Srinivasan <maddy@linux.ibm.com>,
Shunsuke Nakamura <nakamura.shun@fujitsu.com>,
Song Liu <song@kernel.org>,
Masami Hiramatsu <mhiramat@kernel.org>,
Steven Rostedt <rostedt@goodmis.org>,
Miaoqian Lin <linmq006@gmail.com>,
Stephen Brennan <stephen.s.brennan@oracle.com>,
Kajol Jain <kjain@linux.ibm.com>,
Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>,
German Gomez <german.gomez@arm.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
Eric Dumazet <edumazet@google.com>,
Dmitry Vyukov <dvyukov@google.com>, Hao Luo <haoluo@google.com>,
Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v5 00/17] Reference count checker and related fixes
Date: Tue, 4 Apr 2023 14:02:36 -0300 [thread overview]
Message-ID: <ZCxYLAMrdNyiLVvr@kernel.org> (raw)
In-Reply-To: <CAP-5=fX4=pUmcFpRZ5xFds1awSr7HSo1F9rH4=D7NJXW9OXXVQ@mail.gmail.com>
Em Tue, Apr 04, 2023 at 08:58:55AM -0700, Ian Rogers escreveu:
> Ping. It would be nice to have this landed or at least the first 10
> patches that refactor the map API and are the bulk of the
> lines-of-code changed. Having those landed would make it easier to
> rebase in the future, but I also think the whole series is ready to
> go.
I'm trying to get it to compile:
CC /tmp/build/perf-tools-next/util/bpf-event.o
In file included from util/machine.h:7,
from util/session.h:8,
from util/unwind-libunwind-local.c:35:
util/unwind-libunwind-local.c: In function ‘read_unwind_spec_eh_frame’:
util/maps.h:29:18: error: assignment to ‘struct map *’ from incompatible pointer type ‘struct map_rb_node *’ [-Werror=incompatible-pointer-types]
29 | for (map = maps__first(maps); map; map = map_rb_node__next(map))
| ^
util/unwind-libunwind-local.c:328:9: note: in expansion of macro ‘maps__for_each_entry’
328 | maps__for_each_entry(ui->thread->maps, map) {
| ^~~~~~~~~~~~~~~~~~~~
util/unwind-libunwind-local.c:328:48: error: passing argument 1 of ‘map_rb_node__next’ from incompatible pointer type [-Werror=incompatible-pointer-types]
328 | maps__for_each_entry(ui->thread->maps, map) {
| ^~~
| |
| struct map *
util/maps.h:29:68: note: in definition of macro ‘maps__for_each_entry’
29 | for (map = maps__first(maps); map; map = map_rb_node__next(map))
| ^~~
util/maps.h:24:59: note: expected ‘struct map_rb_node *’ but argument is of type ‘struct map *’
24 | struct map_rb_node *map_rb_node__next(struct map_rb_node *node);
| ~~~~~~~~~~~~~~~~~~~~^~~~
util/maps.h:29:48: error: assignment to ‘struct map *’ from incompatible pointer type ‘struct map_rb_node *’ [-Werror=incompatible-pointer-types]
29 | for (map = maps__first(maps); map; map = map_rb_node__next(map))
| ^
util/unwind-libunwind-local.c:328:9: note: in expansion of macro ‘maps__for_each_entry’
328 | maps__for_each_entry(ui->thread->maps, map) {
| ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/unwind-libunwind-local.o] Error 1
make[4]: *** Waiting for unfinished jobs....
LD /tmp/build/perf-tools-next/util/scripting-engines/perf-in.o
make[3]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:140: util] Error 2
make[2]: *** [Makefile.perf:676: /tmp/build/perf-tools-next/perf-in.o] Error 2
make[1]: *** [Makefile.perf:236: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
Performance counter stats for 'make -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf-tools-next -C tools/perf install-bin':
260622279301 cycles:u
285362743453 instructions:u # 1.09 insn per cycle
6.001315366 seconds time elapsed
62.979105000 seconds user
13.088797000 seconds sys
⬢[acme@toolbox perf-tools-next]$ git log --oneline -1
51a0f26e88c893ac (HEAD) perf maps: Remove rb_node from struct map
⬢[acme@toolbox perf-tools-next]$
I'm also making some changes to reduce the number of patch lines and
conserve the project 'git blame' usefulness, not changing the logic in
your patches.
- Arnaldo
> Thanks,
> Ian
>
> On Mon, Mar 20, 2023 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
> >
> > The perf tool has a class of memory problems where reference counts
> > are used incorrectly. Memory/address sanitizers and valgrind don't
> > provide useful ways to debug these problems, you see a memory leak
> > where the only pertinent information is the original allocation
> > site. What would be more useful is knowing where a get fails to have a
> > corresponding put, where there are double puts, etc.
> >
> > This work was motivated by the roll-back of:
> > https://lore.kernel.org/linux-perf-users/20211118193714.2293728-1-irogers@google.com/
> > where fixing a missed put resulted in a use-after-free in a different
> > context. There was a sense in fixing the issue that a game of
> > wac-a-mole had been embarked upon in adding missed gets and puts.
> >
> > The basic approach of the change is to add a level of indirection at
> > the get and put calls. Get allocates a level of indirection that, if
> > no corresponding put is called, becomes a memory leak (and associated
> > stack trace) that leak sanitizer can report. Similarly if two puts are
> > called for the same get, then a double free can be detected by address
> > sanitizer. This can also detect the use after put, which should also
> > yield a segv without a sanitizer.
> >
> > Adding reference count checking to cpu map was done as a proof of
> > concept, it yielded little other than a location where the use of get
> > could be cleaner by using its result. Reference count checking on
> > nsinfo identified a double free of the indirection layer and the
> > related threads, thereby identifying a data race as discussed here:
> > https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/
> > Accordingly the dso->lock was extended and use to cover the race.
> >
> > The v3 version addresses problems in v2, in particular using macros to
> > avoid #ifdefs. The v3 version applies the reference count checking
> > approach to two more data structures, maps and map. While maps was
> > straightforward, struct map showed a problem where reference counted
> > thing can be on lists and rb-trees that are oblivious to the
> > reference count. To sanitize this, struct map is changed so that it is
> > referenced by either a list or rb-tree node and not part of it. This
> > simplifies the reference count and the patches have caught and fixed a
> > number of missed or mismatched reference counts relating to struct
> > map.
> >
> > The patches are arranged so that API refactors and bug fixes appear
> > first, then the reference count checker itself appears. This allows
> > for the refactor and fixes to be applied upstream first, as has
> > already happened with cpumap.
> >
> > A wider discussion of the approach is on the mailing list:
> > https://lore.kernel.org/linux-perf-users/YffqnynWcc5oFkI5@kernel.org/T/#mf25ccd7a2e03de92cec29d36e2999a8ab5ec7f88
> > Comparing it to a past approach:
> > https://lore.kernel.org/all/20151209021047.10245.8918.stgit@localhost.localdomain/
> > and to ref_tracker:
> > https://lwn.net/Articles/877603/
> >
> > v5. rebase removing 5 merged changes. Add map_list_node__new to the
> > 1st patch (perf map: Move map list node into symbol) as suggested
> > by Arnaldo. Remove unnecessary map__puts from patch 12 (perf map:
> > Changes to reference counting) as suggested by Adrian. A summary
> > of the sizes of the remaining patches is:
> > 74fd7ffafdd0 perf map: Add reference count checking
> > 12 files changed, 136 insertions(+), 114 deletions(-)
> > 4719196db8d3 perf maps: Add reference count checking.
> > 8 files changed, 64 insertions(+), 56 deletions(-)
> > 03943e7594cf perf namespaces: Add reference count checking
> > 7 files changed, 83 insertions(+), 62 deletions(-)
> > 0bb382cc52d7 perf cpumap: Add reference count checking
> > 6 files changed, 81 insertions(+), 71 deletions(-)
> > ef39f550c40d libperf: Add reference count checking macros.
> > 1 file changed, 94 insertions(+)
> > d9ac37c750e0 perf map: Changes to reference counting
> > 11 files changed, 112 insertions(+), 44 deletions(-)
> > 476014bc9b55 perf maps: Modify maps_by_name to hold a reference to a map
> > 2 files changed, 33 insertions(+), 18 deletions(-)
> > 91384676fddd perf test: Add extra diagnostics to maps test
> > 1 file changed, 36 insertions(+), 15 deletions(-)
> > fdc30434f826 perf map: Add accessors for pgoff and reloc
> > 9 files changed, 33 insertions(+), 23 deletions(-)
> > 368fe015adb2 perf map: Add accessors for prot, priv and flags
> > 6 files changed, 28 insertions(+), 12 deletions(-)
> > 2c6a8169826a perf map: Add helper for map_ip and unmap_ip
> > 23 files changed, 80 insertions(+), 65 deletions(-)
> > 929e59d49f4b perf map: Rename map_ip and unmap_ip
> > 6 files changed, 13 insertions(+), 13 deletions(-)
> > 4a38194aaaf5 perf map: Add accessor for start and end
> > 24 files changed, 114 insertions(+), 103 deletions(-)
> > 02b63e5c415e perf map: Add accessor for dso
> > 48 files changed, 404 insertions(+), 293 deletions(-)
> > 9324af6ccf42 perf maps: Add functions to access maps
> > 20 files changed, 175 insertions(+), 111 deletions(-)
> > 5c590d36a308 perf maps: Remove rb_node from struct map
> > 16 files changed, 291 insertions(+), 184 deletions(-)
> > af1d142eb777 perf map: Move map list node into symbol
> > 2 files changed, 63 insertions(+), 35 deletions(-)
> >
> > v4. rebases on to acme's perf-tools-next, fixes more issues with
> > map/maps and breaks apart the accessor functions to reduce
> > individual patch sizes. The accessor functions are mechanical
> > changes where the single biggest one is refactoring use of
> > map->dso to be map__dso(map).
> >
> > The v3 change is available here:
> > https://lore.kernel.org/lkml/20220211103415.2737789-1-irogers@google.com/
> >
> > Ian Rogers (17):
> > perf map: Move map list node into symbol
> > perf maps: Remove rb_node from struct map
> > perf maps: Add functions to access maps
> > perf map: Add accessor for dso
> > perf map: Add accessor for start and end
> > perf map: Rename map_ip and unmap_ip
> > perf map: Add helper for map_ip and unmap_ip
> > perf map: Add accessors for prot, priv and flags
> > perf map: Add accessors for pgoff and reloc
> > perf test: Add extra diagnostics to maps test
> > perf maps: Modify maps_by_name to hold a reference to a map
> > perf map: Changes to reference counting
> > libperf: Add reference count checking macros.
> > perf cpumap: Add reference count checking
> > perf namespaces: Add reference count checking
> > perf maps: Add reference count checking.
> > perf map: Add reference count checking
> >
> > tools/lib/perf/Makefile | 2 +-
> > tools/lib/perf/cpumap.c | 94 ++---
> > tools/lib/perf/include/internal/cpumap.h | 4 +-
> > tools/lib/perf/include/internal/rc_check.h | 94 +++++
> > tools/perf/arch/s390/annotate/instructions.c | 4 +-
> > tools/perf/arch/x86/tests/dwarf-unwind.c | 2 +-
> > tools/perf/arch/x86/util/event.c | 13 +-
> > tools/perf/builtin-annotate.c | 11 +-
> > tools/perf/builtin-buildid-list.c | 4 +-
> > tools/perf/builtin-inject.c | 12 +-
> > tools/perf/builtin-kallsyms.c | 6 +-
> > tools/perf/builtin-kmem.c | 4 +-
> > tools/perf/builtin-lock.c | 4 +-
> > tools/perf/builtin-mem.c | 10 +-
> > tools/perf/builtin-report.c | 26 +-
> > tools/perf/builtin-script.c | 27 +-
> > tools/perf/builtin-top.c | 17 +-
> > tools/perf/builtin-trace.c | 2 +-
> > .../scripts/python/Perf-Trace-Util/Context.c | 13 +-
> > tools/perf/tests/code-reading.c | 37 +-
> > tools/perf/tests/cpumap.c | 4 +-
> > tools/perf/tests/hists_common.c | 8 +-
> > tools/perf/tests/hists_cumulate.c | 14 +-
> > tools/perf/tests/hists_filter.c | 14 +-
> > tools/perf/tests/hists_link.c | 18 +-
> > tools/perf/tests/hists_output.c | 12 +-
> > tools/perf/tests/maps.c | 69 ++--
> > tools/perf/tests/mmap-thread-lookup.c | 3 +-
> > tools/perf/tests/symbols.c | 6 +-
> > tools/perf/tests/thread-maps-share.c | 29 +-
> > tools/perf/tests/vmlinux-kallsyms.c | 54 +--
> > tools/perf/ui/browsers/annotate.c | 9 +-
> > tools/perf/ui/browsers/hists.c | 19 +-
> > tools/perf/ui/browsers/map.c | 4 +-
> > tools/perf/util/annotate.c | 40 ++-
> > tools/perf/util/auxtrace.c | 2 +-
> > tools/perf/util/block-info.c | 4 +-
> > tools/perf/util/bpf-event.c | 10 +-
> > tools/perf/util/bpf_lock_contention.c | 6 +-
> > tools/perf/util/build-id.c | 2 +-
> > tools/perf/util/callchain.c | 24 +-
> > tools/perf/util/cpumap.c | 40 ++-
> > tools/perf/util/data-convert-json.c | 10 +-
> > tools/perf/util/db-export.c | 16 +-
> > tools/perf/util/dlfilter.c | 28 +-
> > tools/perf/util/dso.c | 8 +-
> > tools/perf/util/dsos.c | 2 +-
> > tools/perf/util/event.c | 27 +-
> > tools/perf/util/evsel_fprintf.c | 4 +-
> > tools/perf/util/hist.c | 22 +-
> > tools/perf/util/intel-pt.c | 63 ++--
> > tools/perf/util/machine.c | 252 ++++++++------
> > tools/perf/util/map.c | 217 ++++++------
> > tools/perf/util/map.h | 74 +++-
> > tools/perf/util/maps.c | 318 ++++++++++-------
> > tools/perf/util/maps.h | 67 +++-
> > tools/perf/util/namespaces.c | 132 +++++---
> > tools/perf/util/namespaces.h | 3 +-
> > tools/perf/util/pmu.c | 8 +-
> > tools/perf/util/probe-event.c | 62 ++--
> > .../util/scripting-engines/trace-event-perl.c | 10 +-
> > .../scripting-engines/trace-event-python.c | 26 +-
> > tools/perf/util/sort.c | 67 ++--
> > tools/perf/util/symbol-elf.c | 41 ++-
> > tools/perf/util/symbol.c | 320 +++++++++++-------
> > tools/perf/util/symbol_fprintf.c | 2 +-
> > tools/perf/util/synthetic-events.c | 34 +-
> > tools/perf/util/thread-stack.c | 4 +-
> > tools/perf/util/thread.c | 39 +--
> > tools/perf/util/unwind-libdw.c | 20 +-
> > tools/perf/util/unwind-libunwind-local.c | 16 +-
> > tools/perf/util/unwind-libunwind.c | 33 +-
> > tools/perf/util/vdso.c | 7 +-
> > 73 files changed, 1665 insertions(+), 1044 deletions(-)
> > create mode 100644 tools/lib/perf/include/internal/rc_check.h
> >
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >
--
- Arnaldo
next prev parent reply other threads:[~2023-04-04 17:02 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 21:22 [PATCH v5 00/17] Reference count checker and related fixes Ian Rogers
2023-03-20 21:22 ` [PATCH v5 01/17] perf map: Move map list node into symbol Ian Rogers
2023-03-20 21:22 ` [PATCH v5 02/17] perf maps: Remove rb_node from struct map Ian Rogers
2023-03-20 21:22 ` [PATCH v5 03/17] perf maps: Add functions to access maps Ian Rogers
2023-03-20 21:22 ` [PATCH v5 04/17] perf map: Add accessor for dso Ian Rogers
2023-03-20 21:22 ` [PATCH v5 05/17] perf map: Add accessor for start and end Ian Rogers
2023-03-20 21:22 ` [PATCH v5 06/17] perf map: Rename map_ip and unmap_ip Ian Rogers
2023-03-20 21:22 ` [PATCH v5 07/17] perf map: Add helper for " Ian Rogers
2023-03-20 21:22 ` [PATCH v5 08/17] perf map: Add accessors for prot, priv and flags Ian Rogers
2023-03-20 21:22 ` [PATCH v5 09/17] perf map: Add accessors for pgoff and reloc Ian Rogers
2023-03-20 21:22 ` [PATCH v5 10/17] perf test: Add extra diagnostics to maps test Ian Rogers
2023-03-20 21:22 ` [PATCH v5 11/17] perf maps: Modify maps_by_name to hold a reference to a map Ian Rogers
2023-03-20 21:22 ` [PATCH v5 12/17] perf map: Changes to reference counting Ian Rogers
2023-03-20 21:22 ` [PATCH v5 13/17] libperf: Add reference count checking macros Ian Rogers
2023-03-20 21:22 ` [PATCH v5 14/17] perf cpumap: Add reference count checking Ian Rogers
2023-03-20 21:22 ` [PATCH v5 15/17] perf namespaces: " Ian Rogers
2023-03-20 21:22 ` [PATCH v5 16/17] perf maps: " Ian Rogers
2023-03-20 21:22 ` [PATCH v5 17/17] perf map: " Ian Rogers
2023-04-04 15:58 ` [PATCH v5 00/17] Reference count checker and related fixes Ian Rogers
2023-04-04 17:02 ` Arnaldo Carvalho de Melo [this message]
2023-04-04 17:07 ` Arnaldo Carvalho de Melo
2023-04-04 17:25 ` Adrian Hunter
2023-04-04 17:35 ` Ian Rogers
2023-04-04 18:37 ` Adrian Hunter
2023-04-04 19:22 ` Arnaldo Carvalho de Melo
2023-04-04 19:53 ` Arnaldo Carvalho de Melo
2023-04-04 19:54 ` Arnaldo Carvalho de Melo
2023-04-04 18:41 ` Arnaldo Carvalho de Melo
2023-04-04 18:54 ` Arnaldo Carvalho de Melo
2023-04-05 8:47 ` Adrian Hunter
2023-04-05 13:20 ` Arnaldo Carvalho de Melo
2023-04-05 16:25 ` Adrian Hunter
2023-04-06 12:51 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZCxYLAMrdNyiLVvr@kernel.org \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexey.v.bayduraev@linux.intel.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=dave@stgolabs.net \
--cc=dvhart@infradead.org \
--cc=dvyukov@google.com \
--cc=edumazet@google.com \
--cc=eranian@google.com \
--cc=german.gomez@arm.com \
--cc=haoluo@google.com \
--cc=irogers@google.com \
--cc=james.clark@arm.com \
--cc=john.g.garry@oracle.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=kjain@linux.ibm.com \
--cc=leo.yan@linaro.org \
--cc=linmq006@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=maddy@linux.ibm.com \
--cc=mark.rutland@arm.com \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=nakamura.shun@fujitsu.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rickyman7@gmail.com \
--cc=rostedt@goodmis.org \
--cc=song@kernel.org \
--cc=stephen.s.brennan@oracle.com \
--cc=tglx@linutronix.de \
--cc=tmricht@linux.ibm.com \
--cc=yury.norov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.