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 <[email protected]> > > Signed-off-by: Namhyung Kim <[email protected]> > > --- > > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
