Messages in this thread | | | Date | Wed, 20 May 2015 18:55:54 +0300 | From | Adrian Hunter <> | Subject | Re: [PATCH 4/4] perf tools: Add dso__data_get/put_fd() |
| |
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@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?
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 ;-)
| |