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

[Adrian Hunter <adrian.hunter@intel.com>]

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;
>  		}
>  	}
>