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