Messages in this thread | | | Date | Thu, 24 Sep 2015 12:56:31 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH/RFC] perf buildid: Cache kernel DSO created when reading buildid header table |
| |
Em Fri, Sep 25, 2015 at 12:01:12AM +0900, Namhyung Kim escreveu: > Hi Arnaldo, > > On Thu, Sep 24, 2015 at 10:42:28AM -0300, Arnaldo Carvalho de Melo wrote: > > Namhyung, > > > > Can you take a look and perhaps give me your Acked-by? > > The dso->kernel and event->cpumode is always confusing for modules..
Right, my idea is to delete dso->kernel, to remove this confusion.
Please see my tmp.perf/core branch, there are a few patches there using alternative methods in places previously using the botched dso->kernel.
> In this case, it seems that mmap events set kernel cpumode but > build-id events don't.
in the previous sentence you think build-id events don't set perf_event_attr.misc bits related to cpumode...
> So kernel cpumode in a bulid-id event > indicates that it is a kernel (vmlinux) dso, right?
While here you ask if it does?
All these are synthesized, it is a matter of looking at the routines synthesizing them :-)
> I'll test this tomorrow..
Thanks, over time there were areas where multiple people touched and added different semantics that are biting now, I'm trying to, while fixing a bug, the one Wang Nan reported, clarify those things.
It is taking more time than I antecipated, as I'm stumbling in what look like other bugs in the perf-probe codebase :-/
- Arnaldo
> Thanks, > Namhyung > > > > > > From: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > No need to traverse it all again using the filename extension to > > disambiguate kernel from kernel modules, just cache it when > > reading the buildid table. > > > > This also fixes a refcount bug in the error path, i.e. in the error path > > for the __machine__create_kernel_maps() call we do a dso__put(), which > > is ok and expected if that dso came from machine__findnew_dso(), but > > wasn't when we found it by traversing the machine dso list without > > grabbing a dso refcount (dso__get()) before dropping the list lock. > > > > Fixes: b837a8bdc489 ("perf tools: Fix build-id matching on vmlinux") > > Link: http://lkml.kernel.org/n/tip-48fh3cf6pvs4zs2fj4nhc4b5@git.kernel.org > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > --- > > tools/perf/util/header.c | 7 +++++++ > > tools/perf/util/machine.c | 34 +++------------------------------- > > tools/perf/util/machine.h | 1 + > > 3 files changed, 11 insertions(+), 31 deletions(-) > > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > > index 43838003c1a1..6bc92ae1e99a 100644 > > --- a/tools/perf/util/header.c > > +++ b/tools/perf/util/header.c > > @@ -1260,6 +1260,13 @@ static int __event_process_build_id(struct build_id_event *bev, > > > > if (!is_kernel_module(filename, cpumode)) > > dso->kernel = dso_type; > > + /* > > + * We don't need to grab a reference as long as > > + * the kernel dso is in the machine dso list. > > + */ > > + if (cpumode == PERF_RECORD_MISC_KERNEL || > > + cpumode == PERF_RECORD_MISC_GUEST_KERNEL) > > + machine->kernel_dso = dso; > > > > build_id__sprintf(dso->build_id, sizeof(dso->build_id), > > sbuild_id); > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > > index d07a38678e14..f6e689b2c83c 100644 > > --- a/tools/perf/util/machine.c > > +++ b/tools/perf/util/machine.c > > @@ -62,6 +62,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid) > > } > > > > machine->current_tid = NULL; > > + machine->kernel_dso = NULL; > > > > return 0; > > } > > @@ -1180,39 +1181,10 @@ static int machine__process_kernel_mmap_event(struct machine *machine, > > * Should be there already, from the build-id table in > > * the header. > > */ > > - struct dso *kernel = NULL; > > - struct dso *dso; > > + struct dso *kernel; > > > > pthread_rwlock_rdlock(&machine->dsos.lock); > > - > > - list_for_each_entry(dso, &machine->dsos.head, node) { > > - > > - /* > > - * The cpumode passed to is_kernel_module is not the > > - * cpumode of *this* event. If we insist on passing > > - * correct cpumode to is_kernel_module, we should > > - * record the cpumode when we adding this dso to the > > - * linked list. > > - * > > - * However we don't really need passing correct > > - * cpumode. We know the correct cpumode must be kernel > > - * mode (if not, we should not link it onto kernel_dsos > > - * list). > > - * > > - * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN. > > - * is_kernel_module() treats it as a kernel cpumode. > > - */ > > - > > - if (!dso->kernel || > > - is_kernel_module(dso->long_name, > > - PERF_RECORD_MISC_CPUMODE_UNKNOWN)) > > - continue; > > - > > - > > - kernel = dso; > > - break; > > - } > > - > > + kernel = dso__get(machine->kernel_dso); > > pthread_rwlock_unlock(&machine->dsos.lock); > > > > if (kernel == NULL) > > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h > > index 9dfc4281f940..d7b044a1046f 100644 > > --- a/tools/perf/util/machine.h > > +++ b/tools/perf/util/machine.h > > @@ -38,6 +38,7 @@ struct machine { > > struct dsos dsos; > > struct map_groups kmaps; > > struct map *vmlinux_maps[MAP__NR_TYPES]; > > + struct dso *kernel_dso; > > u64 kernel_start; > > symbol_filter_t symbol_filter; > > pid_t *current_tid; > > -- > > 2.1.0 > >
| |