From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752769AbbETIPQ (ORCPT ); Wed, 20 May 2015 04:15:16 -0400 Received: from mga14.intel.com ([192.55.52.115]:50832 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752453AbbETIOb (ORCPT ); Wed, 20 May 2015 04:14:31 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,464,1427785200"; d="scan'208";a="495942094" Message-ID: <555C41DA.9090303@intel.com> Date: Wed, 20 May 2015 11:12:10 +0300 From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Namhyung Kim , Arnaldo Carvalho de Melo CC: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern Subject: Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening References: <1432103647-14017-1-git-send-email-namhyung@kernel.org> <1432103647-14017-2-git-send-email-namhyung@kernel.org> In-Reply-To: <1432103647-14017-2-git-send-email-namhyung@kernel.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > --- > 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 > { > 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. > - > - 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. > - > 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. > > 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. Really should change dso_cache__read() too. > if (dso->data.fd < 0) { > ret = -errno; > - dso->data.status = DSO_DATA_STATUS_ERROR; > goto out; > } > } >