Received-SPF: pass (google.com: domain of linux-kernel+bounces-62329-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240207223639.3139601-1-irogers@google.com> <CAM9d7chBixXozCQztM2WKGbfs_8C70vy6ROzKpwLSqq-upz5iQ@mail.gmail.com>
 <CAP-5=fUVkaq3dDoeMYYEN1N-ghnL-GiP8PV3N3pWpjQKpDTCHw@mail.gmail.com>
 <CAP-5=fXs8=HvjGpkLwuZBi0Hh8jtmz7=0Tp7HRgU8FOFN0GZvg@mail.gmail.com> <CAM9d7choe-CruqcdkLMPC1Eu4Oca0CBaaq-uiCd=csiLY60NBw@mail.gmail.com>
In-Reply-To: <CAM9d7choe-CruqcdkLMPC1Eu4Oca0CBaaq-uiCd=csiLY60NBw@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Mon, 12 Feb 2024 12:22:15 -0800
Message-ID: <CAP-5=fXAkbnpKpkCM8soy7pHzCZZJ8VWYrd9ewFtZoaoA6vZnQ@mail.gmail.com>
Subject: Re: [PATCH v2 0/6] maps memory improvements and fixes
To: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Song Liu <song@kernel.org>, 
	Miguel Ojeda <ojeda@kernel.org>, Liam Howlett <liam.howlett@oracle.com>, 
	Colin Ian King <colin.i.king@gmail.com>, K Prateek Nayak <kprateek.nayak@amd.com>, 
	Artem Savkov <asavkov@redhat.com>, Changbin Du <changbin.du@huawei.com>, 
	Masami Hiramatsu <mhiramat@kernel.org>, Athira Rajeev <atrajeev@linux.vnet.ibm.com>, 
	Yang Jihong <yangjihong1@huawei.com>, Tiezhu Yang <yangtiezhu@loongson.cn>, 
	James Clark <james.clark@arm.com>, Leo Yan <leo.yan@linaro.org>, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, 
	bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 12, 2024 at 12:10=E2=80=AFPM Namhyung Kim <namhyung@kernel.org>=
 wrote:
>
> On Sat, Feb 10, 2024 at 10:08=E2=80=AFAM Ian Rogers <irogers@google.com> =
wrote:
> >
> > On Fri, Feb 9, 2024 at 6:46=E2=80=AFPM Ian Rogers <irogers@google.com> =
wrote:
> > >
> > > On Thu, Feb 8, 2024 at 9:44=E2=80=AFAM Namhyung Kim <namhyung@kernel.=
org> wrote:
> > > >
> > > > Hi Ian,
> > > >
> > > > On Wed, Feb 7, 2024 at 2:37=E2=80=AFPM Ian Rogers <irogers@google.c=
om> wrote:
> > > > >
> > > > > First 6 patches from:
> > > > > https://lore.kernel.org/lkml/20240202061532.1939474-1-irogers@goo=
gle.com/
> > > > >
> > > > > v2. Fix NO_LIBUNWIND=3D1 build issue.
> > > > >
> > > > > Ian Rogers (6):
> > > > >   perf maps: Switch from rbtree to lazily sorted array for addres=
ses
> > > > >   perf maps: Get map before returning in maps__find
> > > > >   perf maps: Get map before returning in maps__find_by_name
> > > > >   perf maps: Get map before returning in maps__find_next_entry
> > > > >   perf maps: Hide maps internals
> > > > >   perf maps: Locking tidy up of nr_maps
> > > >
> > > > Now I see a perf test failure on the vmlinux test:
> > > >
> > > > $ sudo ./perf test -v vmlinux
> > > >   1: vmlinux symtab matches kallsyms                               =
  :
> > > > --- start ---
> > > > test child forked, pid 4164115
> > > > /proc/{kallsyms,modules} inconsistency while looking for
> > > > "[__builtin__kprobes]" module!
> > > > /proc/{kallsyms,modules} inconsistency while looking for
> > > > "[__builtin__kprobes]" module!
> > > > /proc/{kallsyms,modules} inconsistency while looking for
> > > > "[__builtin__ftrace]" module!
> > > > Looking at the vmlinux_path (8 entries long)
> > > > Using /usr/lib/debug/boot/vmlinux-6.5.13-1rodete2-amd64 for symbols
> > > > perf: Segmentation fault
> > > > Obtained 16 stack frames.
> > > > ./perf(+0x1b7dcd) [0x55c40be97dcd]
> > > > ./perf(+0x1b7eb7) [0x55c40be97eb7]
> > > > /lib/x86_64-linux-gnu/libc.so.6(+0x3c510) [0x7f33d7a5a510]
> > > > ./perf(+0x1c2e9c) [0x55c40bea2e9c]
> > > > ./perf(+0x1c43f6) [0x55c40bea43f6]
> > > > ./perf(+0x1c4649) [0x55c40bea4649]
> > > > ./perf(+0x1c46d3) [0x55c40bea46d3]
> > > > ./perf(+0x1c7303) [0x55c40bea7303]
> > > > ./perf(+0x1c70b5) [0x55c40bea70b5]
> > > > ./perf(+0x1c73e6) [0x55c40bea73e6]
> > > > ./perf(+0x11833e) [0x55c40bdf833e]
> > > > ./perf(+0x118f78) [0x55c40bdf8f78]
> > > > ./perf(+0x103d49) [0x55c40bde3d49]
> > > > ./perf(+0x103e75) [0x55c40bde3e75]
> > > > ./perf(+0x1044c0) [0x55c40bde44c0]
> > > > ./perf(+0x104de0) [0x55c40bde4de0]
> > > > test child interrupted
> > > > ---- end ----
> > > > vmlinux symtab matches kallsyms: FAILED!
> > >
> > > Ah, tripped over a latent bug summarized in this part of an asan stac=
k trace:
> > > ```
> > > freed by thread T0 here:
> > >    #0 0x7fa13bcd74b5 in __interceptor_realloc
> > > ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:85
> > >    #1 0x561d66377713 in __maps__insert util/maps.c:353
> > >    #2 0x561d66377b89 in maps__insert util/maps.c:413
> > >    #3 0x561d6652911d in dso__process_kernel_symbol util/symbol-elf.c:=
1460
> > >    #4 0x561d6652aaae in dso__load_sym_internal util/symbol-elf.c:1675
> > >    #5 0x561d6652b6dc in dso__load_sym util/symbol-elf.c:1771
> > >    #6 0x561d66321a4e in dso__load util/symbol.c:1914
> > >    #7 0x561d66372cd9 in map__load util/map.c:353
> > >    #8 0x561d663730e7 in map__find_symbol_by_name_idx util/map.c:397
> > >    #9 0x561d663731e7 in map__find_symbol_by_name util/map.c:410
> > >    #10 0x561d66378208 in maps__find_symbol_by_name_cb util/maps.c:524
> > >    #11 0x561d66377f49 in maps__for_each_map util/maps.c:471
> > >    #12 0x561d663784a0 in maps__find_symbol_by_name util/maps.c:546
> > >    #13 0x561d662093e8 in machine__find_kernel_symbol_by_name util/mac=
hine.h:243
> > >    #14 0x561d6620abbd in test__vmlinux_matches_kallsyms
> > > tests/vmlinux-kallsyms.c:330
> > > ...
> > > ```
> > > dso__process_kernel_symbol rewrites the kernel maps here:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.=
git/tree/tools/perf/util/symbol-elf.c#n1378
> > > which resizes the maps_by_address array causing the maps__for_each_ma=
p
> > > iteration in frame 11 to be iterating over a stale/freed value.
> > >
> > > The most correct solutions would be to clone the maps_by_address arra=
y
> > > prior to iteration, or reference count maps_by_address and its size.
> > > Neither of these solutions particularly appeal, so just reloading the
> > > maps_by_address and size on each iteration also fixes the problem, bu=
t
> > > possibly causes some maps to be skipped/repeated. I think this is
> > > acceptable correctness for the performance.
>
> Can we move map__load() out of maps__for_each_map() ?
> I think the callback should just return the map and break the loop.
> And it can call the map__load() out of the read lock.

It would need a rewrite of map__find_symbol_by_name which is being
called by a callback from maps__find_symbol_by_name. Perhaps an
initial pass to ensure everything is loaded and a safe version of the
loop that copies the maps_by_address ahead of copying it. It'd be of a
scope that'd be worth its own patch set.

> >
> > An aside, shouldn't taking a write lock to modify the maps deadlock
> > with holding the read lock for iteration? Well no because
> > perf_singlethreaded is true for the test:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.gi=
t/tree/tools/perf/util/rwsem.c#n17
> > Another perf_singlethreaded considered evil :-) Note, just getting rid
> > of perf_singlethreaded means latent bugs like this will pop up and
> > will need resolution.
>
> Yeah, maybe.  How about turning it on in the test code?

Agreed, but I think it should be a follow up.

Thanks,
Ian

> Thanks,
> Namhyung