Re: [PATCH 4/4] perf tools: Add dso__data_get/put_fd()

[Adrian Hunter]

On 20/05/15 09:34, Namhyung Kim wrote:
> Using dso__data_fd() in multi-thread environment is not safe since
> returned fd can be closed and/or reused anytime.  So convert it to the
> dso__data_get/put_fd() pair to protect the access with lock.

This is good, but ideally dso__data_open_lock should be a rwlock.

> 
> The original dso__data_fd() is deprecated and kept only for testing.

Maybe move it into perf/tests/dso-data.c since that seems to be the only caller.

> 
> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/util/dso.c              | 44 +++++++++++++++++++++++++++++---------
>  tools/perf/util/dso.h              |  9 ++++++--
>  tools/perf/util/unwind-libunwind.c | 38 +++++++++++++++++++-------------
>  3 files changed, 64 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 21fae6908717..5227e41925c2 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -471,27 +471,49 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine)
>  }
>  
>  /**
> - * dso__data_fd - Get dso's data file descriptor
> + * dso__data_get_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.
> + * returns file descriptor.  Should be paired with
> + * dso__data_put_fd().
>   */
> -int dso__data_fd(struct dso *dso, struct machine *machine)
> +int dso__data_get_fd(struct dso *dso, struct machine *machine)
>  {
> +	pthread_mutex_lock(&dso__data_open_lock);

I would check the return on all lock functions and consider failure to be an
error. i.e.

	if (pthread_mutex_lock(&dso__data_open_lock))
		return -1;
> +
>  	if (dso->data.status == DSO_DATA_STATUS_ERROR)
>  		return -1;

The status check can be done before taking the lock.

>  
> -	pthread_mutex_lock(&dso__data_open_lock);
> -
>  	if (dso->data.fd < 0)
>  		try_to_open_dso(dso, machine);
>  
> -	pthread_mutex_unlock(&dso__data_open_lock);
>  	return dso->data.fd;
>  }
>  
> +void dso__data_put_fd(struct dso *dso __maybe_unused)
> +{
> +	pthread_mutex_unlock(&dso__data_open_lock);
> +}
> +
> +/**
> + * dso__data_get_fd - Get dso's data file descriptor
> + * @dso: dso object
> + * @machine: machine object
> + *
> + * Obsolete interface to find dso's file, open it and
> + * returns file descriptor.  It's not thread-safe in that
> + * the returned fd may be reused for other file.
> + */
> +int dso__data_fd(struct dso *dso, struct machine *machine)
> +{
> +	int fd = dso__data_get_fd(dso, machine);
> +
> +	dso__data_put_fd(dso);
> +	return fd;
> +}
> +
>  bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
>  {
>  	u32 flag = 1 << by;
> @@ -1198,12 +1220,14 @@ size_t dso__fprintf(struct dso *dso, enum map_type type, FILE *fp)
>  enum dso_type dso__type(struct dso *dso, struct machine *machine)
>  {
>  	int fd;
> +	enum dso_type type = DSO__TYPE_UNKNOWN;
>  
> -	fd = dso__data_fd(dso, machine);
> -	if (fd < 0)
> -		return DSO__TYPE_UNKNOWN;
> +	fd = dso__data_get_fd(dso, machine);
> +	if (fd >= 0)
> +		type = dso__type_fd(fd);
> +	dso__data_put_fd(dso);
>  
> -	return dso__type_fd(fd);
> +	return type;
>  }
>  
>  int dso__strerror_load(struct dso *dso, char *buf, size_t buflen)
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index b26ec3ab1336..de9d98c44ae2 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -240,7 +240,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>  
>  /*
>   * The dso__data_* external interface provides following functions:
> - *   dso__data_fd
> + *   dso__data_fd (obsolete)
> + *   dso__data_get_fd
> + *   dso__data_put_fd
>   *   dso__data_close
>   *   dso__data_size
>   *   dso__data_read_offset
> @@ -257,8 +259,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>   * The current usage of the dso__data_* interface is as follows:
>   *
>   * Get DSO's fd:
> - *   int fd = dso__data_fd(dso, machine);
> + *   int fd = dso__data_get_fd(dso, machine);
>   *   USE 'fd' SOMEHOW
> + *   dso__data_put_fd(dso)

Usually, with paired functions, the second of the pair is not called if the
first fails. It probably has to be that way anyway if you check the error
return from the lock function.

>   *
>   * Read DSO's data:
>   *   n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
> @@ -278,6 +281,8 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>   * TODO
>  */
>  int dso__data_fd(struct dso *dso, struct machine *machine);
> +int dso__data_get_fd(struct dso *dso, struct machine *machine);
> +void dso__data_put_fd(struct dso *dso);
>  void dso__data_close(struct dso *dso);
>  
>  off_t dso__data_size(struct dso *dso, struct machine *machine);
> diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> index 7b09a443a280..6d4ab3251729 100644
> --- a/tools/perf/util/unwind-libunwind.c
> +++ b/tools/perf/util/unwind-libunwind.c
> @@ -269,13 +269,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
>  	u64 offset = dso->data.eh_frame_hdr_offset;
>  
>  	if (offset == 0) {
> -		fd = dso__data_fd(dso, machine);
> -		if (fd < 0)
> -			return -EINVAL;
> -
> -		/* Check the .eh_frame section for unwinding info */
> -		offset = elf_section_offset(fd, ".eh_frame_hdr");
> -		dso->data.eh_frame_hdr_offset = offset;
> +		fd = dso__data_get_fd(dso, machine);
> +		if (fd >= 0) {
> +			/* Check the .eh_frame section for unwinding info */
> +			offset = elf_section_offset(fd, ".eh_frame_hdr");
> +			dso->data.eh_frame_hdr_offset = offset;
> +		}
> +		dso__data_put_fd(dso);
>  	}
>  
>  	if (offset)
> @@ -294,13 +294,19 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>  	u64 ofs = dso->data.debug_frame_offset;
>  
>  	if (ofs == 0) {
> -		fd = dso__data_fd(dso, machine);
> -		if (fd < 0)
> -			return -EINVAL;
> -
> -		/* Check the .debug_frame section for unwinding info */
> -		ofs = elf_section_offset(fd, ".debug_frame");
> -		dso->data.debug_frame_offset = ofs;
> +		int ret = 0;
> +
> +		fd = dso__data_get_fd(dso, machine);
> +		if (fd >= 0) {
> +			/* Check the .debug_frame section for unwinding info */
> +			ofs = elf_section_offset(fd, ".debug_frame");
> +			dso->data.debug_frame_offset = ofs;
> +		} else
> +			ret = -EINVAL;
> +
> +		dso__data_put_fd(dso);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	*offset = ofs;
> @@ -353,10 +359,12 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
>  #ifndef NO_LIBUNWIND_DEBUG_FRAME
>  	/* Check the .debug_frame section for unwinding info */
>  	if (!read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
> -		int fd = dso__data_fd(map->dso, ui->machine);
> +		int fd = dso__data_get_fd(map->dso, ui->machine);
>  		int is_exec = elf_is_exec(fd, map->dso->name);
>  		unw_word_t base = is_exec ? 0 : map->start;
>  
> +		dso__data_put_fd(map->dso);
> +
>  		memset(&di, 0, sizeof(di));
>  		if (dwarf_find_debug_frame(0, &di, ip, base, map->dso->name,
>  					   map->start, map->end))
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/