From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932339AbbETPNL (ORCPT ); Wed, 20 May 2015 11:13:11 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:34515 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932288AbbETPNH (ORCPT ); Wed, 20 May 2015 11:13:07 -0400 Date: Thu, 21 May 2015 00:11:44 +0900 From: Namhyung Kim To: Adrian Hunter Cc: Arnaldo Carvalho de Melo , Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern Subject: Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening Message-ID: <20150520151144.GE29162@danjae.kornet> References: <1432103647-14017-1-git-send-email-namhyung@kernel.org> <1432103647-14017-2-git-send-email-namhyung@kernel.org> <555C41DA.9090303@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <555C41DA.9090303@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 20, 2015 at 11:12:10AM +0300, Adrian Hunter wrote: > On 20/05/15 09:34, Namhyung Kim wrote: > > When dso__data_read_offset/addr() is called without prior > > dso__data_fd() (or other functions which call it internally), it > > failed to open dso in data_file_size() since its binary type was not > > identified. > > > > However calling dso__data_fd() in dso__data_read_offset() will hurt > > performance as it grabs a global lock everytime. So factor out the > > loop on the binary type in dso__data_fd(), and call it from both. > > > > Cc: Adrian Hunter > > Signed-off-by: Namhyung Kim > > Look good. A few points below. Thank you. > > > --- > > tools/perf/util/dso.c | 44 ++++++++++++++++++++++++-------------------- > > 1 file changed, 24 insertions(+), 20 deletions(-) > > > > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c > > index 1b96c8d18435..a3984beca723 100644 > > --- a/tools/perf/util/dso.c > > +++ b/tools/perf/util/dso.c > > @@ -440,15 +440,7 @@ void dso__data_close(struct dso *dso) > > pthread_mutex_unlock(&dso__data_open_lock); > > } > > > > -/** > > - * dso__data_fd - Get dso's data file descriptor > > - * @dso: dso object > > - * @machine: machine object > > - * > > - * External interface to find dso's file, open it and > > - * returns file descriptor. > > - */ > > -int dso__data_fd(struct dso *dso, struct machine *machine) > > +static void try_to_open_dso(struct dso *dso, struct machine *machine) > > This could have 'nolock' in the name e.g. get_fd_nolock Hmm.. it's always hard for me to choose a good name. But generally I don't like a name ends with nolock or something like it. And, in perf, adding '__' prefix is used more than adding nolock suffix. Do you dislike the name 'try_to_open_dso'? I chose that because it tries to open dso with a path prefix based on for each binary type. Also it's called from other than dso__data_fd(). > > > { > > enum dso_binary_type binary_type_data[] = { > > DSO_BINARY_TYPE__BUILD_ID_CACHE, > > @@ -457,14 +449,6 @@ int dso__data_fd(struct dso *dso, struct machine *machine) > > }; > > int i = 0; > > > > - if (dso->data.status == DSO_DATA_STATUS_ERROR) > > - return -1; > > Please retain this check. It is needed to prevent repeatedly > trying to open files that aren't there. I just move it out of the function, so it'll be checked before entering this function without lock. > > > - > > - pthread_mutex_lock(&dso__data_open_lock); > > - > > - if (dso->data.fd >= 0) > > - goto out; > > I would retain this check too. The caller shouldn't really have to do it. OK. > > > - > > if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND) { > > dso->data.fd = open_dso(dso, machine); > > goto out; > > @@ -475,14 +459,34 @@ int dso__data_fd(struct dso *dso, struct machine *machine) > > > > dso->data.fd = open_dso(dso, machine); > > if (dso->data.fd >= 0) > > - goto out; > > + break; > > > > } while (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND); > > + > > out: > > if (dso->data.fd >= 0) > > dso->data.status = DSO_DATA_STATUS_OK; > > else > > dso->data.status = DSO_DATA_STATUS_ERROR; > > +} > > + > > +/** > > + * dso__data_fd - Get dso's data file descriptor > > + * @dso: dso object > > + * @machine: machine object > > + * > > + * External interface to find dso's file, open it and > > + * returns file descriptor. > > + */ > > +int dso__data_fd(struct dso *dso, struct machine *machine) > > +{ > > + if (dso->data.status == DSO_DATA_STATUS_ERROR) > > + return -1; > > + > > + pthread_mutex_lock(&dso__data_open_lock); > > + > > + if (dso->data.fd < 0) > > + try_to_open_dso(dso, machine); > > Having the 'dso->data.fd < 0' check inside try_to_open_dso() > saves a line here. OK > > > > > pthread_mutex_unlock(&dso__data_open_lock); > > return dso->data.fd; > > @@ -709,10 +713,10 @@ static int data_file_size(struct dso *dso, struct machine *machine) > > * file (dso) due to open file limit (RLIMIT_NOFILE). > > */ > > if (dso->data.fd < 0) { > > - dso->data.fd = open_dso(dso, machine); > > + try_to_open_dso(dso, machine); > > + > > Having the 'dso->data.fd < 0' check inside try_to_open_dso() > saves a couple of lines here. OK > > Really should change dso_cache__read() too. Right, will add. Thanks, Namhyung > > > if (dso->data.fd < 0) { > > ret = -errno; > > - dso->data.status = DSO_DATA_STATUS_ERROR; > > goto out; > > } > > } > > >