lkml.org 
[lkml]   [2016]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH perf/core v7 07/21] perf symbol: Cleanup the code flow of dso__find_kallsyms
On Wed, 11 May 2016 12:55:50 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> 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)
>
> ?

Yes, I actually had done that at prototyping. But as the manpage of access
said, access() checks accessibility by real uid/gid, but open() checks
by effective uid/gid. So, I considered this could cause unexpected
issue if we decided to use setuid(). I just wanted to check it was 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?

I agreed in general :) If we don't can use access(R_OK) I'd like to use it.

>
> 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...

Ah, right. At least I had to write a comment about it.

Thank you,

>
> - 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,


--
Masami Hiramatsu <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2016-05-12 04:01    [W:0.083 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site