Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening

[Namhyung Kim <namhyung@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 <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;
> >  		}
> >  	}
> > 
>