Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754412AbdGCSos (ORCPT ); Mon, 3 Jul 2017 14:44:48 -0400 Received: from mail.kernel.org ([198.145.29.99]:37682 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754359AbdGCSon (ORCPT ); Mon, 3 Jul 2017 14:44:43 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B6DA421C98 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org Date: Mon, 3 Jul 2017 15:44:34 -0300 From: Arnaldo Carvalho de Melo To: Krister Johansen Cc: Peter Zijlstra , Ingo Molnar , Alexander Shishkin , linux-kernel@vger.kernel.org Subject: Re: [PATCH tip/perf/core 2/7] perf maps: lookup maps in both intitial mountns and inner mountns. Message-ID: <20170703184434.GB27350@kernel.org> References: <1498875539-4200-1-git-send-email-kjlx@templeofstupid.com> <1498875539-4200-3-git-send-email-kjlx@templeofstupid.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1498875539-4200-3-git-send-email-kjlx@templeofstupid.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14246 Lines: 462 Em Fri, Jun 30, 2017 at 07:18:54PM -0700, Krister Johansen escreveu: > If a process is in a mountns and has symbols in /tmp/perf-.map, > look first in the namespace using the tgid for the pidns that the > process might be in. If the map isn't found there, try looking in > the mountns where perf is running, and use the tgid that's appropriate > for perf's pid namespace. If all else fails, use the original pid. > > This allows us to locate a symbol map file in the mount namespace, if > it was generated there. However, we also try the tool's /tmp in case > it's there instead. > > Signed-off-by: Krister Johansen > --- > tools/perf/util/machine.c | 19 ++++------ > tools/perf/util/map.c | 29 ++++++++++++---- > tools/perf/util/map.h | 8 ++--- > tools/perf/util/namespaces.c | 83 ++++++++++++++++++++++++++++++++++++++++---- > tools/perf/util/namespaces.h | 5 ++- > tools/perf/util/symbol.c | 71 ++++++++++++++++++++++++++++++------- > 6 files changed, 172 insertions(+), 43 deletions(-) > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index d7f31cb..ed98881 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -1388,13 +1388,10 @@ int machine__process_mmap2_event(struct machine *machine, > else > type = MAP__FUNCTION; > > - map = map__new(machine, event->mmap2.start, > - event->mmap2.len, event->mmap2.pgoff, > - event->mmap2.pid, event->mmap2.maj, > - event->mmap2.min, event->mmap2.ino, > - event->mmap2.ino_generation, > - event->mmap2.prot, > - event->mmap2.flags, > + map = map__new(machine, event->mmap2.start, event->mmap2.len, > + event->mmap2.pgoff, event->mmap2.maj, event->mmap2.min, > + event->mmap2.ino, event->mmap2.ino_generation, > + event->mmap2.prot, event->mmap2.flags, > event->mmap2.filename, type, thread); Try not to reflow like that, it makes it harder to spot _just what changed_, so you just removed the .pid parameter, ok, it should be: map = map__new(machine, event->mmap2.start, event->mmap2.len, event->mmap2.pgoff, - event->mmap2.pid, event->mmap2.maj, + event->mmap2.maj, event->mmap2.min, event->mmap2.ino, event->mmap2.ino_generation, event->mmap2.prot, event->mmap2.flags, event->mmap2.filename, type, thread); > > if (map == NULL) > @@ -1446,11 +1443,9 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event > else > type = MAP__FUNCTION; > > - map = map__new(machine, event->mmap.start, > - event->mmap.len, event->mmap.pgoff, > - event->mmap.pid, 0, 0, 0, 0, 0, 0, > - event->mmap.filename, > - type, thread); > + map = map__new(machine, event->mmap.start, event->mmap.len, > + event->mmap.pgoff, 0, 0, 0, 0, 0, 0, > + event->mmap.filename, type, thread); Ditto. > > if (map == NULL) > goto out_problem_map; > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c > index 5dc60ca..2bd20cb 100644 > --- a/tools/perf/util/map.c > +++ b/tools/perf/util/map.c > @@ -145,12 +145,14 @@ void map__init(struct map *map, enum map_type type, > refcount_set(&map->refcnt, 1); > } > > -struct map *map__new(struct machine *machine, u64 start, u64 len, > - u64 pgoff, u32 pid, u32 d_maj, u32 d_min, u64 ino, > - u64 ino_gen, u32 prot, u32 flags, char *filename, > - enum map_type type, struct thread *thread) > +struct map *map__new(struct machine *machine, u64 start, u64 len, u64 pgoff, > + u32 d_maj, u32 d_min, u64 ino, u64 ino_gen, u32 prot, > + u32 flags, char *filename, enum map_type type, > + struct thread *thread) Ditto > { > struct map *map = malloc(sizeof(*map)); > + struct nsinfo *nsi = NULL; > + struct nsinfo *nnsi; > > if (map != NULL) { > char newfilename[PATH_MAX]; > @@ -168,9 +170,11 @@ struct map *map__new(struct machine *machine, u64 start, u64 len, > map->ino_generation = ino_gen; > map->prot = prot; > map->flags = flags; > + nsi = nsinfo__get(thread->nsinfo); > > - if ((anon || no_dso) && type == MAP__FUNCTION) { > - snprintf(newfilename, sizeof(newfilename), "/tmp/perf-%d.map", pid); > + if ((anon || no_dso) && nsi && type == MAP__FUNCTION) { > + snprintf(newfilename, sizeof(newfilename), > + "/tmp/perf-%d.map", nsi->pid); > filename = newfilename; > } > > @@ -180,6 +184,16 @@ struct map *map__new(struct machine *machine, u64 start, u64 len, > } > > if (vdso) { > + /* The vdso maps are always on the host and not the > + * container. Ensure that we don't use setns to look > + * them up. > + */ > + nnsi = nsinfo__copy(nsi); > + if (nnsi) { > + nsinfo__put(nsi); > + nnsi->need_setns = false; > + nsi = nnsi; > + } > pgoff = 0; > dso = machine__findnew_vdso(machine, thread); > } else > @@ -201,11 +215,12 @@ struct map *map__new(struct machine *machine, u64 start, u64 len, > if (type != MAP__FUNCTION) > dso__set_loaded(dso, map->type); > } > - dso->nsinfo = nsinfo__get(thread->nsinfo); > + dso->nsinfo = nsi; > dso__put(dso); > } > return map; > out_delete: > + nsinfo__put(nsi); > free(map); > return NULL; > } > diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h > index f9e8ac8..bf015a93 100644 > --- a/tools/perf/util/map.h > +++ b/tools/perf/util/map.h > @@ -140,10 +140,10 @@ struct thread; > > void map__init(struct map *map, enum map_type type, > u64 start, u64 end, u64 pgoff, struct dso *dso); > -struct map *map__new(struct machine *machine, u64 start, u64 len, > - u64 pgoff, u32 pid, u32 d_maj, u32 d_min, u64 ino, > - u64 ino_gen, u32 prot, u32 flags, > - char *filename, enum map_type type, struct thread *thread); > +struct map *map__new(struct machine *machine, u64 start, u64 len, u64 pgoff, > + u32 d_maj, u32 d_min, u64 ino, u64 ino_gen, u32 prot, > + u32 flags, char *filename, enum map_type type, > + struct thread *thread); Ditto > struct map *map__new2(u64 start, struct dso *dso, enum map_type type); > void map__delete(struct map *map); > struct map *map__clone(struct map *map); > diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c > index d7f31e0..cad7d8b 100644 > --- a/tools/perf/util/namespaces.c > +++ b/tools/perf/util/namespaces.c > @@ -40,18 +40,23 @@ void namespaces__free(struct namespaces *namespaces) > free(namespaces); > } > > -void nsinfo__init(struct nsinfo *nsi) > +int nsinfo__init(struct nsinfo *nsi) > { > char oldns[PATH_MAX]; > + char spath[PATH_MAX]; > char *newns = NULL; > + char *statln = NULL; > struct stat old_stat; > struct stat new_stat; > + FILE *f = NULL; > + size_t linesz = 0; > + int rv = -1; > > if (snprintf(oldns, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX) > - return; > + return rv; > > if (asprintf(&newns, "/proc/%d/ns/mnt", nsi->pid) == -1) > - return; > + return rv; > > if (stat(oldns, &old_stat) < 0) > goto out; > @@ -68,24 +73,89 @@ void nsinfo__init(struct nsinfo *nsi) > newns = NULL; > } > > + /* If we're dealing with a process that is in a different PID namespace, > + * attempt to work out the innermost tgid for the process. > + */ > + if (snprintf(spath, PATH_MAX, "/proc/%d/status", nsi->pid) >= PATH_MAX) > + goto out; > + > + f = fopen(spath, "r"); > + if (f == NULL) > + goto out; > + > + while (getline(&statln, &linesz, f) != -1) { > + /* Use tgid if CONFIG_PID_NS is not defined. */ > + if (strstr(statln, "Tgid:") != NULL) { > + nsi->tgid = (pid_t)strtol(strrchr(statln, '\t'), > + NULL, 10); > + nsi->nstgid = nsi->tgid; > + } > + > + if (strstr(statln, "NStgid:") != NULL) { > + nsi->nstgid = (pid_t)strtol(strrchr(statln, '\t'), > + NULL, 10); > + break; > + } > + } > + rv = 0; > + > out: > + if (f != NULL) > + (void) fclose(f); > + free(statln); > free(newns); > + return rv; > } > > struct nsinfo *nsinfo__new(pid_t pid) > { > - struct nsinfo *nsi = calloc(1, sizeof(*nsi)); > + struct nsinfo *nsi; > > + if (pid == 0) > + return NULL; > + > + nsi = calloc(1, sizeof(*nsi)); > if (nsi != NULL) { > nsi->pid = pid; > + nsi->tgid = pid; > + nsi->nstgid = pid; > nsi->need_setns = false; > - nsinfo__init(nsi); > + /* Init may fail if the process exits while we're trying to look > + * at its proc information. In that case, save the pid but > + * don't try to enter the namespace. > + */ > + if (nsinfo__init(nsi) == -1) > + nsi->need_setns = false; > + > refcount_set(&nsi->refcnt, 1); > } > > return nsi; > } > > +struct nsinfo *nsinfo__copy(struct nsinfo *nsi) > +{ > + struct nsinfo *nnsi; > + > + nnsi = calloc(1, sizeof(*nnsi)); > + if (nnsi != NULL) { > + nnsi->pid = nsi->pid; > + nnsi->tgid = nsi->tgid; > + nnsi->nstgid = nsi->nstgid; > + nnsi->need_setns = nsi->need_setns; > + if (nsi->mntns_path) { > + nnsi->mntns_path = strdup(nsi->mntns_path); > + if (!nnsi->mntns_path) { > + free(nnsi); > + return NULL; > + } > + } > + refcount_set(&nnsi->refcnt, 1); > + } > + > + return nnsi; > +} > + > void nsinfo__delete(struct nsinfo *nsi) > { > free(nsi->mntns_path); > @@ -105,7 +175,8 @@ void nsinfo__put(struct nsinfo *nsi) > nsinfo__delete(nsi); > } > > -void nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc) > +void nsinfo__mountns_enter(struct nsinfo *nsi, > + struct nscookie *nc) > { > char curpath[PATH_MAX]; > int oldns = -1; > diff --git a/tools/perf/util/namespaces.h b/tools/perf/util/namespaces.h > index b20f6ea..f19aa41 100644 > --- a/tools/perf/util/namespaces.h > +++ b/tools/perf/util/namespaces.h > @@ -26,6 +26,8 @@ void namespaces__free(struct namespaces *namespaces); > > struct nsinfo { > pid_t pid; > + pid_t tgid; > + pid_t nstgid; > bool need_setns; > char *mntns_path; > refcount_t refcnt; > @@ -36,8 +38,9 @@ struct nscookie { > int newns; > }; > > -void nsinfo__init(struct nsinfo *nsi); > +int nsinfo__init(struct nsinfo *nsi); > struct nsinfo *nsinfo__new(pid_t pid); > +struct nsinfo *nsinfo__copy(struct nsinfo *nsi); > void nsinfo__delete(struct nsinfo *nsi); > > struct nsinfo *nsinfo__get(struct nsinfo *nsi); > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > index 60a9eaa..97e454f 100644 > --- a/tools/perf/util/symbol.c > +++ b/tools/perf/util/symbol.c > @@ -19,7 +19,6 @@ > #include "strlist.h" > #include "intlist.h" > #include "namespaces.h" > -#include "vdso.h" > #include "header.h" > #include "path.h" > #include "sane_ctype.h" > @@ -1327,14 +1326,15 @@ int dso__load_kallsyms(struct dso *dso, const char *filename, > return __dso__load_kallsyms(dso, filename, map, false); > } > > -static int dso__load_perf_map(struct dso *dso, struct map *map) > +static int dso__load_perf_map(const char *map_path, struct dso *dso, > + struct map *map) > { > char *line = NULL; > size_t n; > FILE *file; > int nr_syms = 0; > > - file = fopen(dso->long_name, "r"); > + file = fopen(map_path, "r"); > if (file == NULL) > goto out_failure; > > @@ -1426,6 +1426,45 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod, > } > } > > +/* Checks for the existence of the perf-.map file in two different > + * locations. First, if the process is a separate mount namespace, check in > + * that namespace using the pid of the innermost pid namespace. If's not in a > + * namespace, or the file can't be found there, try in the mount namespace of > + * the tracing process using our view of its pid. > + */ > +static int dso__find_perf_map(char *filebuf, size_t bufsz, > + struct nsinfo **nsip) > +{ > + struct nscookie nsc; > + struct nsinfo *nsi; > + struct nsinfo *nnsi; > + int rc = -1; > + > + nsi = *nsip; > + > + if (nsi->need_setns) { > + snprintf(filebuf, bufsz, "/tmp/perf-%d.map", nsi->nstgid); > + nsinfo__mountns_enter(nsi, &nsc); > + rc = access(filebuf, R_OK); > + nsinfo__mountns_exit(&nsc); > + if (rc == 0) > + return rc; > + } > + > + nnsi = nsinfo__copy(nsi); > + if (nnsi) { > + nsinfo__put(nsi); > + > + nnsi->need_setns = false; > + snprintf(filebuf, bufsz, "/tmp/perf-%d.map", nnsi->tgid); > + *nsip = nnsi; > + rc = 0; > + } > + > + return rc; > +} > + > + > int dso__load(struct dso *dso, struct map *map) > { > char *name; > @@ -1437,18 +1476,23 @@ int dso__load(struct dso *dso, struct map *map) > struct symsrc ss_[2]; > struct symsrc *syms_ss = NULL, *runtime_ss = NULL; > bool kmod; > + bool perfmap; > unsigned char build_id[BUILD_ID_SIZE]; > struct nscookie nsc; > + char newmapname[PATH_MAX]; > + const char *map_path = dso->long_name; > + > + perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0; > + if (perfmap) { > + if (dso->nsinfo && (dso__find_perf_map(newmapname, > + sizeof(newmapname), &dso->nsinfo) == 0)) { > + map_path = newmapname; > + } > + } > > nsinfo__mountns_enter(dso->nsinfo, &nsc); > pthread_mutex_lock(&dso->lock); > > - /* The vdso files always live in the host container, so don't go looking > - * for them in the container's mount namespace. > - */ > - if (dso__is_vdso(dso)) > - nsinfo__mountns_exit(&nsc); > - > /* check again under the dso->lock */ > if (dso__loaded(dso, map->type)) { > ret = 1; > @@ -1471,19 +1515,20 @@ int dso__load(struct dso *dso, struct map *map) > > dso->adjust_symbols = 0; > > - if (strncmp(dso->name, "/tmp/perf-", 10) == 0) { > + if (perfmap) { > struct stat st; > > - if (lstat(dso->name, &st) < 0) > + if (lstat(map_path, &st) < 0) > goto out; > > if (!symbol_conf.force && st.st_uid && (st.st_uid != geteuid())) { > pr_warning("File %s not owned by current user or root, " > - "ignoring it (use -f to override).\n", dso->name); > + "ignoring it (use -f to override).\n", > + map_path); Keep on the same line, that way we see straight away that you're replacing dso->name with map_path, as they will be aligned. > goto out; > } > > - ret = dso__load_perf_map(dso, map); > + ret = dso__load_perf_map(map_path, dso, map); > dso->symtab_type = ret > 0 ? DSO_BINARY_TYPE__JAVA_JIT : > DSO_BINARY_TYPE__NOT_FOUND; > goto out; > -- > 2.7.4