On Fri, Jun 23, 2017 at 11:26:04AM -0300, Arnaldo Carvalho de Melo wrote: > Em Fri, Jun 23, 2017 at 02:48:22PM +0900, Namhyung Kim escreveu: > > When loading kernel symbols from /proc/kallsyms, it might have different > > addresses for modules. We should honor the mmap event recorded in a > > perf.data so load the module symbols when it sees the event so that it > > cannot be overridden by symbols in /proc/kallsyms later. > > > Cc: Adrian Hunter <adrian.hun...@intel.com> > > Cc: Wang Nan <wangn...@huawei.com> > > Signed-off-by: Namhyung Kim <namhy...@kernel.org> > > --- > > tools/perf/util/machine.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > > index d838f893e639..799efe920f0c 100644 > > --- a/tools/perf/util/machine.c > > +++ b/tools/perf/util/machine.c > > @@ -656,6 +656,9 @@ struct map *machine__findnew_module_map(struct machine > > *machine, u64 start, > > > > map_groups__insert(&machine->kmaps, map); > > > > + if (dso->has_build_id) > > + map__load(map); > > How could this be set at this point? I just tried processing a > PERF_RECORD_MMAP event for a kernel module and at this point it is set > to false :-\ > > Humm, but I did it for video.ko, that has no hits in this case, so this > must be coming from reading the perf.data build-id table... /me tries > that... yeah, when processing the buildid table in perf.data we > populate the machine->dsos list and set the dso->has_build_id flag: > > [root@jouet ~]# perf buildid-list | grep .ko > 57a47c2f0d85ef5726b80e8f55403c3f508a4eac > /lib/modules/4.12.0-rc4+/kernel/drivers/net/wireless/intel/iwlwifi/iwlwifi.ko > [root@jouet ~]# > (gdb) bt > #0 machine__findnew_module_map (machine=0x21c6c78, > start=18446744072647282688, filename=0x7fd8fc057a00 > "/lib/modules/4.12.0-rc4+/kernel/drivers/net/wireless/intel/iwlwifi/iwlwifi.ko") > at util/machine.c:650 > #1 0x00000000005094a3 in machine__process_kernel_mmap_event > (machine=0x21c6c78, event=0x7fd8fc0579d8) at util/machine.c:1274 > #2 0x00000000005099de in machine__process_mmap_event (machine=0x21c6c78, > event=0x7fd8fc0579d8, sample=0x7fffffffbef0) at util/machine.c:1433 > #3 0x00000000004cc021 in perf_event__process_mmap (tool=0x7fffffffc440, > event=0x7fd8fc0579d8, sample=0x7fffffffbef0, machine=0x21c6c78) at > util/event.c:1265 > #4 0x0000000000512686 in machines__deliver_event (machines=0x21c6c78, > evlist=0x21c6f30, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, > tool=0x7fffffffc440, file_offset=10712) > at util/session.c:1250 > #5 0x00000000005129ce in perf_session__deliver_event (session=0x21c6b90, > event=0x7fd8fc0579d8, sample=0x7fffffffbef0, tool=0x7fffffffc440, > file_offset=10712) at util/session.c:1310 > #6 0x0000000000513255 in perf_session__process_event (session=0x21c6b90, > event=0x7fd8fc0579d8, file_offset=10712) at util/session.c:1490 > #7 0x0000000000513ff4 in __perf_session__process_events (session=0x21c6b90, > data_offset=232, data_size=23120, file_size=23352) at util/session.c:1867 > #8 0x00000000005141f2 in perf_session__process_events (session=0x21c6b90) at > util/session.c:1921 > #9 0x00000000004427c9 in __cmd_report (rep=0x7fffffffc440) at > builtin-report.c:575 > #10 0x000000000044427c in cmd_report (argc=0, argv=0x7fffffffe160) at > builtin-report.c:1054 > #11 0x00000000004bad91 in run_builtin (p=0xa28fd0 <commands+240>, argc=2, > argv=0x7fffffffe160) at perf.c:296 > #12 0x00000000004baffe in handle_internal_command (argc=2, > argv=0x7fffffffe160) at perf.c:348 > #13 0x00000000004bb150 in run_argv (argcp=0x7fffffffdfbc, > argv=0x7fffffffdfb0) at perf.c:392 > #14 0x00000000004bb52a in main (argc=2, argv=0x7fffffffe160) at perf.c:530 > (gdb) p dso > $127 = (struct dso *) 0x21c7dc0 > (gdb) p dso->has_build_id > $128 = 1 '\001' > (gdb) > > So what you're doing is to load it here... perhaps we should not do > that, and instead in the kallsyms loading code, when processing the > symbols for this specific module, discard them and leave its symbol > table unpopulated, then, later, when resolving a sample in this module, > it will see that it is not loaded, will see that it has a build-id and > will use that, and _refuse_ to get it from anywhere else than an ELF > file with that build-id. > > I.e. in general, when we have a build-id for a given DSO, we should not > read it from kallsyms or any other file that don't have a matching > build-id. > > The only possibility would be: hey, I have a build id, and reading the > currently loaded module (/sys/module/e1000e/notes/.note.gnu.build-id) I > see it _still_ has that same build-id, ok, I can read it from kallsyms. > > What do you think?
I'm ok with that, will change to compare build-id before loading symbols. Thanks, Namhyung
