Messages in this thread | | | Date | Wed, 20 May 2015 11:12:10 +0300 | From | Adrian Hunter <> | Subject | Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening |
| |
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 <adrian.hunter@intel.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
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; > } > } >
| |