Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 29 Jan 2015 21:48:32 +0900 | Subject | Re: [PATCH 27/42] perf tools: Protect dso symbol loading using a mutex |
| |
Hi Arnaldo,
On Thu, Jan 29, 2015 at 9:34 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > Em Thu, Jan 29, 2015 at 05:07:08PM +0900, Namhyung Kim escreveu: >> When multi-thread support for perf report is enabled, it's possible to >> access a dso concurrently. Add a new pthread_mutex to protect it from >> concurrent dso__load(). >> >> Signed-off-by: Namhyung Kim <namhyung@kernel.org> >> --- >> tools/perf/util/dso.c | 2 ++ >> tools/perf/util/dso.h | 1 + >> tools/perf/util/symbol.c | 34 ++++++++++++++++++++++++---------- >> 3 files changed, 27 insertions(+), 10 deletions(-) >> >> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c >> index 45be944d450a..3da75816b8f8 100644 >> --- a/tools/perf/util/dso.c >> +++ b/tools/perf/util/dso.c >> @@ -888,6 +888,7 @@ struct dso *dso__new(const char *name) >> RB_CLEAR_NODE(&dso->rb_node); >> INIT_LIST_HEAD(&dso->node); >> INIT_LIST_HEAD(&dso->data.open_entry); >> + pthread_mutex_init(&dso->lock, NULL); >> } >> >> return dso; >> @@ -917,6 +918,7 @@ void dso__delete(struct dso *dso) >> dso_cache__free(&dso->data.cache); >> dso__free_a2l(dso); >> zfree(&dso->symsrc_filename); >> + pthread_mutex_destroy(&dso->lock); >> free(dso); >> } >> >> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h >> index 3782c82c6e44..ac753594a469 100644 >> --- a/tools/perf/util/dso.h >> +++ b/tools/perf/util/dso.h >> @@ -102,6 +102,7 @@ struct dsos { >> }; >> >> struct dso { >> + pthread_mutex_t lock; >> struct list_head node; >> struct rb_node rb_node; /* rbtree node sorted by long name */ >> struct rb_root symbols[MAP__NR_TYPES]; >> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c >> index a69066865a55..714e20c99354 100644 >> --- a/tools/perf/util/symbol.c >> +++ b/tools/perf/util/symbol.c >> @@ -1357,12 +1357,22 @@ int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) >> struct symsrc *syms_ss = NULL, *runtime_ss = NULL; >> bool kmod; >> >> - dso__set_loaded(dso, map->type); >> + pthread_mutex_lock(&dso->lock); >> + >> + /* check again under the dso->lock */ > > Again? Where was it first checked?
Please see map__load().
> Perhaps we should lock there, so that > we don't have to do two checks, one unlocked, the other locked?
Hmm.. maybe. I just keep it to avoid locking overhead since it'll be called whenever it searches symbols during preprocessing. I didn't measure the overhead but it could be huge IMHO.
Thanks, Namhyung
> >> + if (dso__loaded(dso, map->type)) { >> + ret = 1; >> + goto out; >> + } >> + > > - Arnaldo
| |