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 <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
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;
> }
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
