Re: [PATCH 27/42] perf tools: Protect dso symbol loading using a mutex

From: Arnaldo Carvalho de Melo
Date: Thu Jan 29 2015 - 07:34:44 EST


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@xxxxxxxxxx>
> ---
> 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? Perhaps we should lock there, so that
we don't have to do two checks, one unlocked, the other locked?

> + if (dso__loaded(dso, map->type)) {
> + ret = 1;
> + goto out;
> + }
> +

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/