From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754851AbbA2Msy (ORCPT ); Thu, 29 Jan 2015 07:48:54 -0500 Received: from mail-oi0-f49.google.com ([209.85.218.49]:51795 "EHLO mail-oi0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751599AbbA2Msx (ORCPT ); Thu, 29 Jan 2015 07:48:53 -0500 MIME-Version: 1.0 In-Reply-To: <20150129123445.GS7220@kernel.org> References: <1422518843-25818-1-git-send-email-namhyung@kernel.org> <1422518843-25818-28-git-send-email-namhyung@kernel.org> <20150129123445.GS7220@kernel.org> From: Namhyung Kim Date: Thu, 29 Jan 2015 21:48:32 +0900 X-Google-Sender-Auth: lKiNd6hWGkXNTR5fueqyHSkjBdE Message-ID: Subject: Re: [PATCH 27/42] perf tools: Protect dso symbol loading using a mutex To: Arnaldo Carvalho de Melo Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Adrian Hunter , Andi Kleen , Stephane Eranian , Frederic Weisbecker Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Arnaldo, On Thu, Jan 29, 2015 at 9:34 PM, Arnaldo Carvalho de Melo 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 >> --- >> 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