lkml.org 
[lkml]   [2015]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/4] perf tools: Add dso__data_get/put_fd()
    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@intel.com>
    > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
    > > ---
    > > 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))
    > >
    >


    \
     
     \ /
      Last update: 2015-05-20 18:01    [W:4.730 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site