From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932713AbbETP4E (ORCPT ); Wed, 20 May 2015 11:56:04 -0400 Received: from mga09.intel.com ([134.134.136.24]:55004 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754355AbbETP4B (ORCPT ); Wed, 20 May 2015 11:56:01 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,465,1427785200"; d="scan'208";a="574343408" Message-ID: <555CAE8A.9050905@intel.com> Date: Wed, 20 May 2015 18:55:54 +0300 From: Adrian Hunter User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Namhyung Kim CC: Arnaldo Carvalho de Melo , Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern Subject: Re: [PATCH 4/4] perf tools: Add dso__data_get/put_fd() References: <1432103647-14017-1-git-send-email-namhyung@kernel.org> <1432103647-14017-4-git-send-email-namhyung@kernel.org> <555C46C5.3080102@intel.com> <20150520153449.GF29162@danjae.kornet> In-Reply-To: <20150520153449.GF29162@danjae.kornet> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> Signed-off-by: Namhyung Kim >>> --- >>> 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 ;-)