Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 25 Jan 2023 10:32:31 -0800 | From | Krister Johansen <> | Subject | Re: [PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext |
| |
Hi Adrian,
On Wed, Jan 25, 2023 at 09:37:59AM +0200, Adrian Hunter wrote: > On 25/01/23 09:29, Adrian Hunter wrote: > > Also subject line has spelling mistake, and should identify kcore > as the issue e.g. > > perf symbol: Symbol lookup with kcore can fail if multiple segments match stext > > > On 25/01/23 00:35, Krister Johansen wrote: > >> This problem was encountered on an arm64 system with a lot of memory. > >> Without kernel debug symbols installed, and with both kcore and kallsyms > >> available, perf managed to get confused and returned "unknown" for all > >> of the kernel symbols that it tried to look up. > >> > >> On this system, stext fell within the vmalloc segment. The kcore symbol > >> matching code tries to find the first segment that contains stext and > >> uses that to replace the segment generated from just the kallsyms > >> information. In this case, however, there were two: a very large > >> vmalloc segment, and the text segment. This caused perf to get confused > >> because multiple overlapping segments were inserted into the RB tree > >> that holds the discovered segments. However, that alone wasn't > >> sufficient to cause the problem. Even when we could find the segment, > >> the offsets were adjusted in such a way that the newly generated symbols > >> didn't line up with the instruction addresses in the trace. The most > >> obvious solution would be to consult which segment type is text from > >> kcore, but this information is not exposed to users. > >> > >> Instead, select the smallest matching segment that contains stext > >> instead of the first matching segment. This allows us to match the text > >> segment instead of vmalloc, if one is contained within the other. > >> > >> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> > >> --- > >> tools/perf/util/symbol.c | 10 ++++++++-- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > >> index a3a165ae933a..14ac4189eaff 100644 > >> --- a/tools/perf/util/symbol.c > >> +++ b/tools/perf/util/symbol.c > >> @@ -1368,10 +1368,16 @@ static int dso__load_kcore(struct dso *dso, struct map *map, > >> > >> /* Find the kernel map using the '_stext' symbol */ > >> if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) { > >> + u64 replacement_size = 0; > > > > We'd usually put a blank line here > > > >> list_for_each_entry(new_map, &md.maps, node) { > >> - if (stext >= new_map->start && stext < new_map->end) { > >> + u64 new_size = new_map->end - new_map->start; > >> + > >> + if (!(stext >= new_map->start && stext < new_map->end)) > >> + continue; > >> + > > > > Really needs a comment, and please be specific e.g. > > > > ARM64 vmalloc segment overlaps the kernel text segment, so > > choosing the smaller segment will get the kernel text. > > > > > > > >> + if (!replacement_map || new_size < replacement_size) { > >> replacement_map = new_map; > >> - break; > >> + replacement_size = new_size; > >> } > >> } > >> }
Thanks for all the feedback. I'll incorporate these changes and send out a v2 shortly.
-K
| ![\](/images/icornerr.gif) |