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