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

[Adrian Hunter]

On 20/05/2015 6:34 p.m., Namhyung Kim wrote:
> 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).

Unless there are 'nolock' variants ;-)

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

Sure. Could print an error too.

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

Can change that though ;-)
--
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/