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

[Namhyung Kim]

On Wed, May 20, 2015 at 11:33:09AM +0300, Adrian Hunter wrote:
> 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.

Agreed.  But as far as I can see, it might be a recursive mutex since
it needs to allow to call dso__data_* functions while keeping fd open
(ie. the dso__data_open_lock held).

> 
> > 
> > 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.

Okay.

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

Ah, forgot to check the locking operation itself.  So do you suggest
that we should check the return value of the locking in every place?


> > +
> >  	if (dso->data.status == DSO_DATA_STATUS_ERROR)
> >  		return -1;
> 
> The status check can be done before taking the lock.

Right.  But I did it this way since I'd like to make sure to grab the
lock unconditionally when calling the get() function.  See below.

> 
> >  
> > -	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.

Fair enough.

Thank you for review!
Namhyung


> 
> >   *
> >   * 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/