From: Adrian Hunter <adrian.hunter@intel.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Jiri Olsa <jolsa@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH 4/4] perf tools: Add dso__data_get/put_fd()
Date: Wed, 20 May 2015 18:55:54 +0300 [thread overview]
Message-ID: <555CAE8A.9050905@intel.com> (raw)
In-Reply-To: <20150520153449.GF29162@danjae.kornet>
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 ;-)
next prev parent reply other threads:[~2015-05-20 15:56 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-20 6:34 [PATCH 1/4] Revert "perf tools: Fix data_read_offset() file opening" Namhyung Kim
2015-05-20 6:34 ` [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening Namhyung Kim
2015-05-20 8:12 ` Adrian Hunter
2015-05-20 15:11 ` Namhyung Kim
2015-05-20 15:35 ` Adrian Hunter
2015-05-20 6:34 ` [PATCH 3/4] perf tools: Get rid of dso__data_fd() from dso__data_size() Namhyung Kim
2015-05-20 6:34 ` [PATCH 4/4] perf tools: Add dso__data_get/put_fd() Namhyung Kim
2015-05-20 8:33 ` Adrian Hunter
2015-05-20 15:34 ` Namhyung Kim
2015-05-20 15:55 ` Adrian Hunter [this message]
2015-05-20 13:28 ` [PATCH 1/4] Revert "perf tools: Fix data_read_offset() file opening" Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=555CAE8A.9050905@intel.com \
--to=adrian.hunter@intel.com \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@kernel.org \
--cc=dsahern@gmail.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).