Re: [PATCH 3/4] perf map: Set kmap->kmaps backpointer for main kernel map chunks
From: Jiri Olsa
Date: Sun Feb 09 2020 - 14:32:56 EST
On Sun, Feb 09, 2020 at 04:01:08PM +0100, Jiri Olsa wrote:
> On Thu, Feb 06, 2020 at 03:10:38PM +0530, Ravi Bangoria wrote:
> > Hi Arnaldo,
> >
> > On 12/23/19 7:02 PM, Arnaldo Carvalho de Melo wrote:
> > > From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > >
> > > When a map is create to represent the main kernel area (vmlinux) with
> > > map__new2() we allocate an extra area to store a pointer to the 'struct
> > > maps' for the kernel maps, so that we can access that struct when
> > > loading ELF files or kallsyms, as we will need to split it in multiple
> > > maps, one per kernel module or ELF section (such as ".init.text").
> > >
> > > So when map->dso->kernel is non-zero, it is expected that
> > > map__kmap(map)->kmaps to be set to the tree of kernel maps (modules,
> > > chunks of the main kernel, bpf progs put in place via
> > > PERF_RECORD_KSYMBOL, the main kernel).
> > >
> > > This was not the case when we were splitting the main kernel into chunks
> > > for its ELF sections, which ended up making 'perf report --children'
> > > processing a perf.data file with callchains to trip on
> > > __map__is_kernel(), when we press ENTER to see the popup menu for main
> > > histogram entries that starts at a symbol in the ".init.text" ELF
> > > section, e.g.:
> > >
> > > - 8.83% 0.00% swapper [kernel.vmlinux].init.text [k] start_kernel
> > > start_kernel
> > > cpu_startup_entry
> > > do_idle
> > > cpuidle_enter
> > > cpuidle_enter_state
> > > intel_idle
> > >
> > > Fix it.
> >
> > perf top from perf/core has started crashing at __map__is_kernel():
> >
> > (gdb) bt
> > #0 __map__is_kernel (map=<optimized out>) at util/map.c:935
> > #1 0x000000000045551d in perf_event__process_sample (machine=0xbab8f8,
> > sample=0x7fffe5ffa6d0, evsel=0xba7570, event=0xbcac50, tool=0x7fffffff84e0)
> > at builtin-top.c:833
> > #2 deliver_event (qe=<optimized out>, qevent=<optimized out>) at builtin-top.c:1192
> > #3 0x000000000050b9fb in do_flush (show_progress=false, oe=0x7fffffff87e0)
> > at util/ordered-events.c:244
> > #4 __ordered_events__flush (oe=oe@entry=0x7fffffff87e0, how=how@entry=OE_FLUSH__TOP,
> > timestamp=timestamp@entry=0) at util/ordered-events.c:323
> > #5 0x000000000050c1b5 in __ordered_events__flush (timestamp=<optimized out>,
> > how=<optimized out>, oe=<optimized out>) at util/ordered-events.c:339
> > #6 ordered_events__flush (how=OE_FLUSH__TOP, oe=0x7fffffff87e0) at util/ordered-events.c:341
> > #7 ordered_events__flush (oe=oe@entry=0x7fffffff87e0, how=how@entry=OE_FLUSH__TOP)
> > at util/ordered-events.c:339
> > #8 0x0000000000454e21 in process_thread (arg=0x7fffffff84e0) at builtin-top.c:1104
> > #9 0x00007ffff7f2c4e2 in start_thread () from /lib64/libpthread.so.0
> > #10 0x00007ffff76086d3 in clone () from /lib64/libc.so.6
> >
> > I haven't debugged it much but seems like the actual patch that's causing the
> > crash is de90d513b246 ("perf map: Use map->dso->kernel + map__kmaps() in
> > map__kmaps()").
> >
> > Did you face this / aware of it?
>
> hum, looks like there are few more places where we don't set
> kmaps pointer, patch below fixes that for me
>
> I'll still need to do more checking and I'll send a fix
>
> jirka
>
>
> ---
I found one more place.. please check the attached patch
jirka
---
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index c8c5410315e8..89b0b7713a73 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -686,6 +686,7 @@ static struct dso *machine__findnew_module_dso(struct machine *machine,
dso__set_module_info(dso, m, machine);
dso__set_long_name(dso, strdup(filename), true);
+ dso->kernel = DSO_TYPE_KERNEL;
}
dso__get(dso);
@@ -726,13 +727,23 @@ static int machine__process_ksymbol_register(struct machine *machine,
struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
if (!map) {
- map = dso__new_map(event->ksymbol.name);
- if (!map)
+ struct dso *dso = dso__new(event->ksymbol.name);
+ struct kmap *kmap;
+
+ if (dso) {
+ dso->kernel = DSO_TYPE_KERNEL;
+ map = map__new2(0, dso);
+ }
+
+ if (!dso || !map)
return -ENOMEM;
map->start = event->ksymbol.addr;
map->end = map->start + event->ksymbol.len;
maps__insert(&machine->kmaps, map);
+
+ kmap = map__kmap(map);
+ kmap->kmaps = &machine->kmaps;
}
sym = symbol__new(map->map_ip(map, map->start),
@@ -774,6 +785,7 @@ static struct map *machine__addnew_module_map(struct machine *machine, u64 start
const char *filename)
{
struct map *map = NULL;
+ struct kmap *kmap;
struct kmod_path m;
struct dso *dso;
@@ -790,6 +802,9 @@ static struct map *machine__addnew_module_map(struct machine *machine, u64 start
maps__insert(&machine->kmaps, map);
+ kmap = map__kmap(map);
+ kmap->kmaps = &machine->kmaps;
+
/* Put the map here because maps__insert alread got it */
map__put(map);
out:
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 3b379b1296f1..c3dd20082734 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -862,6 +862,9 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
return -1;
}
+ if (ndso->kernel)
+ map__kmap(curr_map)->kmaps = kmaps;
+
curr_map->map_ip = curr_map->unmap_ip = identity__map_ip;
maps__insert(kmaps, curr_map);
++kernel_range;
@@ -1145,11 +1148,13 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
struct kcore_mapfn_data {
struct dso *dso;
struct list_head maps;
+ struct maps *kmaps;
};
static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
{
struct kcore_mapfn_data *md = data;
+ struct kmap *kmap;
struct map *map;
map = map__new2(start, md->dso);
@@ -1161,6 +1166,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
list_add(&map->node, &md->maps);
+ kmap = map__kmap(map);
+ kmap->kmaps = md->kmaps;
return 0;
}
@@ -1270,6 +1277,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
if (validate_kcore_addresses(kallsyms_filename, map))
return -EINVAL;
+ md.kmaps = kmaps;
md.dso = dso;
INIT_LIST_HEAD(&md.maps);