Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752171AbcDZOrd (ORCPT ); Tue, 26 Apr 2016 10:47:33 -0400 Received: from mail.kernel.org ([198.145.29.136]:46027 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751798AbcDZOrc (ORCPT ); Tue, 26 Apr 2016 10:47:32 -0400 Date: Tue, 26 Apr 2016 23:47:27 +0900 From: Masami Hiramatsu To: Arnaldo Carvalho de Melo Cc: linux-kernel@vger.kernel.org, Namhyung Kim , Peter Zijlstra , Ingo Molnar , Hemant Kumar , Ananth N Mavinakayanahalli Subject: Re: [PATCH perf/core v4 02/19] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid Message-Id: <20160426234727.d9901784d226debdc95768d0@kernel.org> In-Reply-To: <20160426134538.GA20998@kernel.org> References: <20160426090200.11891.43944.stgit@devbox> <20160426090221.11891.87192.stgit@devbox> <20160426134538.GA20998@kernel.org> X-Mailer: Sylpheed 3.4.3 (GTK+ 2.24.28; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9201 Lines: 252 On Tue, 26 Apr 2016 10:45:38 -0300 Arnaldo Carvalho de Melo wrote: > Em Tue, Apr 26, 2016 at 06:02:22PM +0900, Masami Hiramatsu escreveu: > > From: Masami Hiramatsu > > > > Use path/to/bin/buildid/elf instead of path/to/bin/buildid > > to store corresponding elf binary. > > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms. > > Note that the build-id based symlink changes to point to the > > path/to/bin/buildid, not path/to/bin/buildid/elf. > > Ok, but what happens to existing caches? Not a problem? I.e. the lookups > are made from ~/.debug/.build-id/, so it will just follow that symlink > and if the file was inserted in the cache before this change, it will > find it there, ok. Right, that the next patch does. And I agree with you that it should be merged to this patch. > > If inserted later, ditto. > > Now what do you mean by "Note that the build-id based symlink changes to > point to the path/to/bin/buildid, not path/to/bin/buildid/elf."? Ah, this is just a note that someone directly accessing the cache. But anyway, it is just a design note. Thank you, > > Probably the 'perf buildid-cache --purge" will just follow the symlink, > so it'll work. > > - Arnaldo > > > > Signed-off-by: Masami Hiramatsu > > Signed-off-by: Masami Hiramatsu > > --- > > tools/perf/util/build-id.c | 65 +++++++++++++++++++++++++++++++------------- > > tools/perf/util/dso.h | 5 +++ > > tools/perf/util/symbol.c | 2 + > > 3 files changed, 52 insertions(+), 20 deletions(-) > > > > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c > > index b6ecf87..46a8bcc 100644 > > --- a/tools/perf/util/build-id.c > > +++ b/tools/perf/util/build-id.c > > @@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const char *fmt, ...) > > return ret; > > } > > > > -static char *build_id__filename(const char *sbuild_id, char *bf, size_t size) > > +static char *build_id_cache__linkname(const char *sbuild_id, char *bf, > > + size_t size) > > { > > char *tmp = bf; > > int ret = asnprintf(&bf, size, "%s/.build-id/%.2s/%s", buildid_dir, > > @@ -154,15 +155,35 @@ static char *build_id__filename(const char *sbuild_id, char *bf, size_t size) > > return bf; > > } > > > > +static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso) > > +{ > > + return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf"); > > +} > > + > > char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size) > > { > > - char build_id_hex[SBUILD_ID_SIZE]; > > + bool is_kallsyms = dso__is_kallsyms((struct dso *)dso); > > + bool is_vdso = dso__is_vdso((struct dso *)dso); > > + char sbuild_id[SBUILD_ID_SIZE]; > > + char *linkname; > > + bool alloc = (bf == NULL); > > + int ret; > > > > if (!dso->has_build_id) > > return NULL; > > > > - build_id__sprintf(dso->build_id, sizeof(dso->build_id), build_id_hex); > > - return build_id__filename(build_id_hex, bf, size); > > + build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id); > > + linkname = build_id_cache__linkname(sbuild_id, NULL, 0); > > + if (!linkname) > > + return NULL; > > + > > + ret = asnprintf(&bf, size, "%s/%s", linkname, > > + build_id_cache__basename(is_kallsyms, is_vdso)); > > + if (ret < 0 || (!alloc && size < (unsigned int)ret)) > > + bf = NULL; > > + free(linkname); > > + > > + return bf; > > } > > > > bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size) > > @@ -341,7 +362,8 @@ void disable_buildid_cache(void) > > } > > > > static char *build_id_cache__dirname_from_path(const char *name, > > - bool is_kallsyms, bool is_vdso) > > + bool is_kallsyms, bool is_vdso, > > + const char *sbuild_id) > > { > > char *realname = (char *)name, *filename; > > bool slash = is_kallsyms || is_vdso; > > @@ -352,8 +374,9 @@ static char *build_id_cache__dirname_from_path(const char *name, > > return NULL; > > } > > > > - if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "", > > - is_vdso ? DSO__NAME_VDSO : realname) < 0) > > + if (asprintf(&filename, "%s%s%s%s%s", buildid_dir, slash ? "/" : "", > > + is_vdso ? DSO__NAME_VDSO : realname, > > + sbuild_id ? "/" : "", sbuild_id ?: "") < 0) > > filename = NULL; > > > > if (!slash) > > @@ -372,7 +395,8 @@ int build_id_cache__list_build_ids(const char *pathname, > > int ret = 0; > > > > list = strlist__new(NULL, NULL); > > - dir_name = build_id_cache__dirname_from_path(pathname, false, false); > > + dir_name = build_id_cache__dirname_from_path(pathname, false, false, > > + NULL); > > if (!list || !dir_name) { > > ret = -ENOMEM; > > goto out; > > @@ -407,7 +431,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name, > > { > > const size_t size = PATH_MAX; > > char *realname = NULL, *filename = NULL, *dir_name = NULL, > > - *linkname = zalloc(size), *targetname, *tmp; > > + *linkname = zalloc(size), *tmp; > > int err = -1; > > > > if (!is_kallsyms) { > > @@ -416,14 +440,17 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name, > > goto out_free; > > } > > > > - dir_name = build_id_cache__dirname_from_path(name, is_kallsyms, is_vdso); > > + dir_name = build_id_cache__dirname_from_path(name, is_kallsyms, > > + is_vdso, sbuild_id); > > if (!dir_name) > > goto out_free; > > > > if (mkdir_p(dir_name, 0755)) > > goto out_free; > > > > - if (asprintf(&filename, "%s/%s", dir_name, sbuild_id) < 0) { > > + /* Save the allocated buildid dirname */ > > + if (asprintf(&filename, "%s/%s", dir_name, > > + build_id_cache__basename(is_kallsyms, is_vdso)) < 0) { > > filename = NULL; > > goto out_free; > > } > > @@ -437,7 +464,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name, > > goto out_free; > > } > > > > - if (!build_id__filename(sbuild_id, linkname, size)) > > + if (!build_id_cache__linkname(sbuild_id, linkname, size)) > > goto out_free; > > tmp = strrchr(linkname, '/'); > > *tmp = '\0'; > > @@ -446,10 +473,10 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name, > > goto out_free; > > > > *tmp = '/'; > > - targetname = filename + strlen(buildid_dir) - 5; > > - memcpy(targetname, "../..", 5); > > + tmp = dir_name + strlen(buildid_dir) - 5; > > + memcpy(tmp, "../..", 5); > > > > - if (symlink(targetname, linkname) == 0) > > + if (symlink(tmp, linkname) == 0) > > err = 0; > > out_free: > > if (!is_kallsyms) > > @@ -474,7 +501,7 @@ static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size, > > bool build_id_cache__cached(const char *sbuild_id) > > { > > bool ret = false; > > - char *filename = build_id__filename(sbuild_id, NULL, 0); > > + char *filename = build_id_cache__linkname(sbuild_id, NULL, 0); > > > > if (filename && !access(filename, F_OK)) > > ret = true; > > @@ -493,7 +520,7 @@ int build_id_cache__remove_s(const char *sbuild_id) > > if (filename == NULL || linkname == NULL) > > goto out_free; > > > > - if (!build_id__filename(sbuild_id, linkname, size)) > > + if (!build_id_cache__linkname(sbuild_id, linkname, size)) > > goto out_free; > > > > if (access(linkname, F_OK)) > > @@ -511,7 +538,7 @@ int build_id_cache__remove_s(const char *sbuild_id) > > tmp = strrchr(linkname, '/') + 1; > > snprintf(tmp, size - (tmp - linkname), "%s", filename); > > > > - if (unlink(linkname)) > > + if (rm_rf(linkname)) > > goto out_free; > > > > err = 0; > > @@ -523,7 +550,7 @@ out_free: > > > > static int dso__cache_build_id(struct dso *dso, struct machine *machine) > > { > > - bool is_kallsyms = dso->kernel && dso->long_name[0] != '/'; > > + bool is_kallsyms = dso__is_kallsyms(dso); > > bool is_vdso = dso__is_vdso(dso); > > const char *name = dso->long_name; > > char nm[PATH_MAX]; > > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h > > index 0953280..76d79d0 100644 > > --- a/tools/perf/util/dso.h > > +++ b/tools/perf/util/dso.h > > @@ -349,6 +349,11 @@ static inline bool dso__is_kcore(struct dso *dso) > > dso->binary_type == DSO_BINARY_TYPE__GUEST_KCORE; > > } > > > > +static inline bool dso__is_kallsyms(struct dso *dso) > > +{ > > + return dso->kernel && dso->long_name[0] != '/'; > > +} > > + > > void dso__free_a2l(struct dso *dso); > > > > enum dso_type dso__type(struct dso *dso, struct machine *machine); > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > > index 415c4f6..c57cb47 100644 > > --- a/tools/perf/util/symbol.c > > +++ b/tools/perf/util/symbol.c > > @@ -1685,7 +1685,7 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map) > > if (!find_matching_kcore(map, path, sizeof(path))) > > return strdup(path); > > > > - scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s", > > + scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms", > > buildid_dir, sbuild_id); > > > > if (access(path, F_OK)) { -- Masami Hiramatsu