Messages in this thread | | | Date | Wed, 11 May 2016 12:55:50 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH perf/core v7 07/21] perf symbol: Cleanup the code flow of dso__find_kallsyms |
| |
Em Wed, May 11, 2016 at 10:52:27PM +0900, Masami Hiramatsu escreveu: > Cleanup the code flow of dso__find_kallsyms() to remove > redundant checking code and add some comment for readability. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > --- > tools/perf/util/symbol.c | 60 +++++++++++++++++++++------------------------- > 1 file changed, 27 insertions(+), 33 deletions(-) > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > index e112bbd..b2b06b7 100644 > --- a/tools/perf/util/symbol.c > +++ b/tools/perf/util/symbol.c > @@ -1629,6 +1629,15 @@ static int find_matching_kcore(struct map *map, char *dir, size_t dir_sz) > return ret; > } > > +static bool filename__readable(const char *file) > +{ > + int fd = open(file, O_RDONLY); > + if (fd < 0) > + return false; > + close(fd); > + return true; > +}
Do we really have to use this big hammer just for checking if a file is readable? What is wronte with:
access(pathname, R_OK)
?
I see, you're keeping the existing logic, but since you've gone to the trouble of introducing a seemingly generic function like filename__readable(), you could as well use the canonical way to check if a file is readable, namely access(R_OK), no?
Adrian, is there something magical about /proc/kcore for this test to be done that way? If so, we better document not to waste our time next time we look at this...
- Arnaldo
> + > static char *dso__find_kallsyms(struct dso *dso, struct map *map) > { > u8 host_build_id[BUILD_ID_SIZE]; > @@ -1648,45 +1657,33 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map) > sizeof(host_build_id)) == 0) > is_host = dso__build_id_equal(dso, host_build_id); > > - build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id); > - > - scnprintf(path, sizeof(path), "%s/%s/%s", buildid_dir, > - DSO__NAME_KCORE, sbuild_id); > - > - /* Use /proc/kallsyms if possible */ > + /* Try a fast path for /proc/kallsyms if possible */
How that improves the previous comment?
> if (is_host) { > - DIR *d; > - int fd; > - > - /* If no cached kcore go with /proc/kallsyms */ > - d = opendir(path); > - if (!d) > - goto proc_kallsyms; > - closedir(d); > - > /* > - * Do not check the build-id cache, until we know we cannot use > - * /proc/kcore. > + * Do not check the build-id cache, unless we know we cannot use > + * /proc/kcore or module maps don't match to /proc/kallsyms. > */ > - fd = open("/proc/kcore", O_RDONLY); > - if (fd != -1) { > - close(fd); > - /* If module maps match go with /proc/kallsyms */ > - if (!validate_kcore_addresses("/proc/kallsyms", map)) > - goto proc_kallsyms; > - } > - > - /* Find kallsyms in build-id cache with kcore */ > - if (!find_matching_kcore(map, path, sizeof(path))) > - return strdup(path); > - > - goto proc_kallsyms; > + if (filename__readable("/proc/kcore") && > + !validate_kcore_addresses("/proc/kallsyms", map)) > + goto proc_kallsyms; > } > > + build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id); > + > /* Find kallsyms in build-id cache with kcore */ > + scnprintf(path, sizeof(path), "%s/%s/%s", > + buildid_dir, DSO__NAME_KCORE, sbuild_id); > + > if (!find_matching_kcore(map, path, sizeof(path))) > return strdup(path); > > + /* Use current /proc/kallsyms if possible */ > + if (is_host) { > +proc_kallsyms: > + return strdup("/proc/kallsyms"); > + } > + > + /* Finally, find a cache of kallsyms */ > scnprintf(path, sizeof(path), "%s/%s/%s", > buildid_dir, DSO__NAME_KALLSYMS, sbuild_id); > > @@ -1697,9 +1694,6 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map) > } > > return strdup(path); > - > -proc_kallsyms: > - return strdup("/proc/kallsyms"); > } > > static int dso__load_kernel_sym(struct dso *dso, struct map *map,
| |