[PATCH 1/4] Revert "perf tools: Fix data_read_offset() file opening"

[PATCH 1/4] Revert "perf tools: Fix data_read_offset() file opening"

[Namhyung Kim]

This reverts commit 9ba6765a979f5f7d3f6456f8c79c66f4aef2d66b.

The problem will be fixed by later patch in some other way.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
Arnaldo, you can ignore this if you drop the Adrian's patch in the tree..

 tools/perf/util/dso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index e248f5693a48..1b96c8d18435 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -756,7 +756,7 @@ off_t dso__data_size(struct dso *dso, struct machine *machine)
 static ssize_t data_read_offset(struct dso *dso, struct machine *machine,
 				u64 offset, u8 *data, ssize_t size)
 {
-	if (dso__data_size(dso, machine) < 0)
+	if (data_file_size(dso, machine))
 		return -1;
 
 	/* Check the offset sanity. */
-- 
2.4.0

[PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening

[Namhyung Kim]

When dso__data_read_offset/addr() is called without prior
dso__data_fd() (or other functions which call it internally), it
failed to open dso in data_file_size() since its binary type was not
identified.

However calling dso__data_fd() in dso__data_read_offset() will hurt
performance as it grabs a global lock everytime.  So factor out the
loop on the binary type in dso__data_fd(), and call it from both.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 1b96c8d18435..a3984beca723 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -440,15 +440,7 @@ void dso__data_close(struct dso *dso)
 	pthread_mutex_unlock(&dso__data_open_lock);
 }
 
-/**
- * dso__data_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.
- */
-int dso__data_fd(struct dso *dso, struct machine *machine)
+static void try_to_open_dso(struct dso *dso, struct machine *machine)
 {
 	enum dso_binary_type binary_type_data[] = {
 		DSO_BINARY_TYPE__BUILD_ID_CACHE,
@@ -457,14 +449,6 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
 	};
 	int i = 0;
 
-	if (dso->data.status == DSO_DATA_STATUS_ERROR)
-		return -1;
-
-	pthread_mutex_lock(&dso__data_open_lock);
-
-	if (dso->data.fd >= 0)
-		goto out;
-
 	if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND) {
 		dso->data.fd = open_dso(dso, machine);
 		goto out;
@@ -475,14 +459,34 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
 
 		dso->data.fd = open_dso(dso, machine);
 		if (dso->data.fd >= 0)
-			goto out;
+			break;
 
 	} while (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND);
+
 out:
 	if (dso->data.fd >= 0)
 		dso->data.status = DSO_DATA_STATUS_OK;
 	else
 		dso->data.status = DSO_DATA_STATUS_ERROR;
+}
+
+/**
+ * dso__data_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.
+ */
+int dso__data_fd(struct dso *dso, struct machine *machine)
+{
+	if (dso->data.status == DSO_DATA_STATUS_ERROR)
+		return -1;
+
+	pthread_mutex_lock(&dso__data_open_lock);
+
+	if (dso->data.fd < 0)
+		try_to_open_dso(dso, machine);
 
 	pthread_mutex_unlock(&dso__data_open_lock);
 	return dso->data.fd;
@@ -709,10 +713,10 @@ static int data_file_size(struct dso *dso, struct machine *machine)
 	 * file (dso) due to open file limit (RLIMIT_NOFILE).
 	 */
 	if (dso->data.fd < 0) {
-		dso->data.fd = open_dso(dso, machine);
+		try_to_open_dso(dso, machine);
+
 		if (dso->data.fd < 0) {
 			ret = -errno;
-			dso->data.status = DSO_DATA_STATUS_ERROR;
 			goto out;
 		}
 	}
-- 
2.4.0

Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening

[Adrian Hunter]

On 20/05/15 09:34, Namhyung Kim wrote:
> When dso__data_read_offset/addr() is called without prior
> dso__data_fd() (or other functions which call it internally), it
> failed to open dso in data_file_size() since its binary type was not
> identified.
> 
> However calling dso__data_fd() in dso__data_read_offset() will hurt
> performance as it grabs a global lock everytime.  So factor out the
> loop on the binary type in dso__data_fd(), and call it from both.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Look good. A few points below.

> ---
>  tools/perf/util/dso.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 1b96c8d18435..a3984beca723 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -440,15 +440,7 @@ void dso__data_close(struct dso *dso)
>  	pthread_mutex_unlock(&dso__data_open_lock);
>  }
>  
> -/**
> - * dso__data_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.
> - */
> -int dso__data_fd(struct dso *dso, struct machine *machine)
> +static void try_to_open_dso(struct dso *dso, struct machine *machine)

This could have 'nolock' in the name e.g. get_fd_nolock

>  {
>  	enum dso_binary_type binary_type_data[] = {
>  		DSO_BINARY_TYPE__BUILD_ID_CACHE,
> @@ -457,14 +449,6 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
>  	};
>  	int i = 0;
>  
> -	if (dso->data.status == DSO_DATA_STATUS_ERROR)
> -		return -1;

Please retain this check. It is needed to prevent repeatedly
trying to open files that aren't there.

> -
> -	pthread_mutex_lock(&dso__data_open_lock);
> -
> -	if (dso->data.fd >= 0)
> -		goto out;

I would retain this check too. The caller shouldn't really have to do it.

> -
>  	if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND) {
>  		dso->data.fd = open_dso(dso, machine);
>  		goto out;
> @@ -475,14 +459,34 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
>  
>  		dso->data.fd = open_dso(dso, machine);
>  		if (dso->data.fd >= 0)
> -			goto out;
> +			break;
>  
>  	} while (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND);
> +
>  out:
>  	if (dso->data.fd >= 0)
>  		dso->data.status = DSO_DATA_STATUS_OK;
>  	else
>  		dso->data.status = DSO_DATA_STATUS_ERROR;
> +}
> +
> +/**
> + * dso__data_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.
> + */
> +int dso__data_fd(struct dso *dso, struct machine *machine)
> +{
> +	if (dso->data.status == DSO_DATA_STATUS_ERROR)
> +		return -1;
> +
> +	pthread_mutex_lock(&dso__data_open_lock);
> +
> +	if (dso->data.fd < 0)
> +		try_to_open_dso(dso, machine);

Having the 'dso->data.fd < 0' check inside try_to_open_dso()
saves a line here.

>  
>  	pthread_mutex_unlock(&dso__data_open_lock);
>  	return dso->data.fd;
> @@ -709,10 +713,10 @@ static int data_file_size(struct dso *dso, struct machine *machine)
>  	 * file (dso) due to open file limit (RLIMIT_NOFILE).
>  	 */
>  	if (dso->data.fd < 0) {
> -		dso->data.fd = open_dso(dso, machine);
> +		try_to_open_dso(dso, machine);
> +

Having the 'dso->data.fd < 0' check inside try_to_open_dso()
saves a couple of lines here.

Really should change dso_cache__read() too.

>  		if (dso->data.fd < 0) {
>  			ret = -errno;
> -			dso->data.status = DSO_DATA_STATUS_ERROR;
>  			goto out;
>  		}
>  	}
> 

Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening

[Namhyung Kim]

On Wed, May 20, 2015 at 11:12:10AM +0300, Adrian Hunter wrote:
> On 20/05/15 09:34, Namhyung Kim wrote:
> > When dso__data_read_offset/addr() is called without prior
> > dso__data_fd() (or other functions which call it internally), it
> > failed to open dso in data_file_size() since its binary type was not
> > identified.
> > 
> > However calling dso__data_fd() in dso__data_read_offset() will hurt
> > performance as it grabs a global lock everytime.  So factor out the
> > loop on the binary type in dso__data_fd(), and call it from both.
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Look good. A few points below.

Thank you.

> 
> > ---
> >  tools/perf/util/dso.c | 44 ++++++++++++++++++++++++--------------------
> >  1 file changed, 24 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 1b96c8d18435..a3984beca723 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -440,15 +440,7 @@ void dso__data_close(struct dso *dso)
> >  	pthread_mutex_unlock(&dso__data_open_lock);
> >  }
> >  
> > -/**
> > - * dso__data_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.
> > - */
> > -int dso__data_fd(struct dso *dso, struct machine *machine)
> > +static void try_to_open_dso(struct dso *dso, struct machine *machine)
> 
> This could have 'nolock' in the name e.g. get_fd_nolock

Hmm.. it's always hard for me to choose a good name.  But generally I
don't like a name ends with nolock or something like it.  And, in
perf, adding '__' prefix is used more than adding nolock suffix.

Do you dislike the name 'try_to_open_dso'?  I chose that because it
tries to open dso with a path prefix based on for each binary type.
Also it's called from other than dso__data_fd().


> 
> >  {
> >  	enum dso_binary_type binary_type_data[] = {
> >  		DSO_BINARY_TYPE__BUILD_ID_CACHE,
> > @@ -457,14 +449,6 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
> >  	};
> >  	int i = 0;
> >  
> > -	if (dso->data.status == DSO_DATA_STATUS_ERROR)
> > -		return -1;
> 
> Please retain this check. It is needed to prevent repeatedly
> trying to open files that aren't there.

I just move it out of the function, so it'll be checked before
entering this function without lock.

> 
> > -
> > -	pthread_mutex_lock(&dso__data_open_lock);
> > -
> > -	if (dso->data.fd >= 0)
> > -		goto out;
> 
> I would retain this check too. The caller shouldn't really have to do it.

OK.

> 
> > -
> >  	if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND) {
> >  		dso->data.fd = open_dso(dso, machine);
> >  		goto out;
> > @@ -475,14 +459,34 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
> >  
> >  		dso->data.fd = open_dso(dso, machine);
> >  		if (dso->data.fd >= 0)
> > -			goto out;
> > +			break;
> >  
> >  	} while (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND);
> > +
> >  out:
> >  	if (dso->data.fd >= 0)
> >  		dso->data.status = DSO_DATA_STATUS_OK;
> >  	else
> >  		dso->data.status = DSO_DATA_STATUS_ERROR;
> > +}
> > +
> > +/**
> > + * dso__data_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.
> > + */
> > +int dso__data_fd(struct dso *dso, struct machine *machine)
> > +{
> > +	if (dso->data.status == DSO_DATA_STATUS_ERROR)
> > +		return -1;
> > +
> > +	pthread_mutex_lock(&dso__data_open_lock);
> > +
> > +	if (dso->data.fd < 0)
> > +		try_to_open_dso(dso, machine);
> 
> Having the 'dso->data.fd < 0' check inside try_to_open_dso()
> saves a line here.

OK

> 
> >  
> >  	pthread_mutex_unlock(&dso__data_open_lock);
> >  	return dso->data.fd;
> > @@ -709,10 +713,10 @@ static int data_file_size(struct dso *dso, struct machine *machine)
> >  	 * file (dso) due to open file limit (RLIMIT_NOFILE).
> >  	 */
> >  	if (dso->data.fd < 0) {
> > -		dso->data.fd = open_dso(dso, machine);
> > +		try_to_open_dso(dso, machine);
> > +
> 
> Having the 'dso->data.fd < 0' check inside try_to_open_dso()
> saves a couple of lines here.

OK


> 
> Really should change dso_cache__read() too.

Right, will add.

Thanks,
Namhyung


> 
> >  		if (dso->data.fd < 0) {
> >  			ret = -errno;
> > -			dso->data.status = DSO_DATA_STATUS_ERROR;
> >  			goto out;
> >  		}
> >  	}
> > 
> 

Re: [PATCH 2/4] perf tools: Fix dso__data_read_offset() file opening

[Adrian Hunter]

On 20/05/2015 6:11 p.m., Namhyung Kim wrote:
> On Wed, May 20, 2015 at 11:12:10AM +0300, Adrian Hunter wrote:
>> On 20/05/15 09:34, Namhyung Kim wrote:
>>>   {
>>>   	enum dso_binary_type binary_type_data[] = {
>>>   		DSO_BINARY_TYPE__BUILD_ID_CACHE,
>>> @@ -457,14 +449,6 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
>>>   	};
>>>   	int i = 0;
>>>
>>> -	if (dso->data.status == DSO_DATA_STATUS_ERROR)
>>> -		return -1;
>>
>> Please retain this check. It is needed to prevent repeatedly
>> trying to open files that aren't there.
>
> I just move it out of the function, so it'll be checked before
> entering this function without lock.

data_file_size() isn't doing that though. It just calls try_to_open_dso()

[PATCH 3/4] perf tools: Get rid of dso__data_fd() from dso__data_size()

[Namhyung Kim]

It seems that the dso__data_fd() was needed to find a binary type
since open in data_file_size() alone used to fail.  But as it can open
the dso fine now, the dso__data_fd() can go away.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index a3984beca723..21fae6908717 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -744,12 +744,6 @@ static int data_file_size(struct dso *dso, struct machine *machine)
  */
 off_t dso__data_size(struct dso *dso, struct machine *machine)
 {
-	int fd;
-
-	fd = dso__data_fd(dso, machine);
-	if (fd < 0)
-		return fd;
-
 	if (data_file_size(dso, machine))
 		return -1;
 
-- 
2.4.0

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

[Namhyung Kim]

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.

The original dso__data_fd() is deprecated and kept only for testing.

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);
+
 	if (dso->data.status == DSO_DATA_STATUS_ERROR)
 		return -1;
 
-	pthread_mutex_lock(&dso__data_open_lock);
-
 	if (dso->data.fd < 0)
 		try_to_open_dso(dso, machine);
 
-	pthread_mutex_unlock(&dso__data_open_lock);
 	return dso->data.fd;
 }
 
+void dso__data_put_fd(struct dso *dso __maybe_unused)
+{
+	pthread_mutex_unlock(&dso__data_open_lock);
+}
+
+/**
+ * dso__data_get_fd - Get dso's data file descriptor
+ * @dso: dso object
+ * @machine: machine object
+ *
+ * Obsolete interface to find dso's file, open it and
+ * returns file descriptor.  It's not thread-safe in that
+ * the returned fd may be reused for other file.
+ */
+int dso__data_fd(struct dso *dso, struct machine *machine)
+{
+	int fd = dso__data_get_fd(dso, machine);
+
+	dso__data_put_fd(dso);
+	return fd;
+}
+
 bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
 {
 	u32 flag = 1 << by;
@@ -1198,12 +1220,14 @@ size_t dso__fprintf(struct dso *dso, enum map_type type, FILE *fp)
 enum dso_type dso__type(struct dso *dso, struct machine *machine)
 {
 	int fd;
+	enum dso_type type = DSO__TYPE_UNKNOWN;
 
-	fd = dso__data_fd(dso, machine);
-	if (fd < 0)
-		return DSO__TYPE_UNKNOWN;
+	fd = dso__data_get_fd(dso, machine);
+	if (fd >= 0)
+		type = dso__type_fd(fd);
+	dso__data_put_fd(dso);
 
-	return dso__type_fd(fd);
+	return type;
 }
 
 int dso__strerror_load(struct dso *dso, char *buf, size_t buflen)
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index b26ec3ab1336..de9d98c44ae2 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -240,7 +240,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
 
 /*
  * The dso__data_* external interface provides following functions:
- *   dso__data_fd
+ *   dso__data_fd (obsolete)
+ *   dso__data_get_fd
+ *   dso__data_put_fd
  *   dso__data_close
  *   dso__data_size
  *   dso__data_read_offset
@@ -257,8 +259,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
  * The current usage of the dso__data_* interface is as follows:
  *
  * Get DSO's fd:
- *   int fd = dso__data_fd(dso, machine);
+ *   int fd = dso__data_get_fd(dso, machine);
  *   USE 'fd' SOMEHOW
+ *   dso__data_put_fd(dso)
  *
  * Read DSO's data:
  *   n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
@@ -278,6 +281,8 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
  * TODO
 */
 int dso__data_fd(struct dso *dso, struct machine *machine);
+int dso__data_get_fd(struct dso *dso, struct machine *machine);
+void dso__data_put_fd(struct dso *dso);
 void dso__data_close(struct dso *dso);
 
 off_t dso__data_size(struct dso *dso, struct machine *machine);
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index 7b09a443a280..6d4ab3251729 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -269,13 +269,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
 	u64 offset = dso->data.eh_frame_hdr_offset;
 
 	if (offset == 0) {
-		fd = dso__data_fd(dso, machine);
-		if (fd < 0)
-			return -EINVAL;
-
-		/* Check the .eh_frame section for unwinding info */
-		offset = elf_section_offset(fd, ".eh_frame_hdr");
-		dso->data.eh_frame_hdr_offset = offset;
+		fd = dso__data_get_fd(dso, machine);
+		if (fd >= 0) {
+			/* Check the .eh_frame section for unwinding info */
+			offset = elf_section_offset(fd, ".eh_frame_hdr");
+			dso->data.eh_frame_hdr_offset = offset;
+		}
+		dso__data_put_fd(dso);
 	}
 
 	if (offset)
@@ -294,13 +294,19 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
 	u64 ofs = dso->data.debug_frame_offset;
 
 	if (ofs == 0) {
-		fd = dso__data_fd(dso, machine);
-		if (fd < 0)
-			return -EINVAL;
-
-		/* Check the .debug_frame section for unwinding info */
-		ofs = elf_section_offset(fd, ".debug_frame");
-		dso->data.debug_frame_offset = ofs;
+		int ret = 0;
+
+		fd = dso__data_get_fd(dso, machine);
+		if (fd >= 0) {
+			/* Check the .debug_frame section for unwinding info */
+			ofs = elf_section_offset(fd, ".debug_frame");
+			dso->data.debug_frame_offset = ofs;
+		} else
+			ret = -EINVAL;
+
+		dso__data_put_fd(dso);
+		if (ret)
+			return ret;
 	}
 
 	*offset = ofs;
@@ -353,10 +359,12 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 #ifndef NO_LIBUNWIND_DEBUG_FRAME
 	/* Check the .debug_frame section for unwinding info */
 	if (!read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
-		int fd = dso__data_fd(map->dso, ui->machine);
+		int fd = dso__data_get_fd(map->dso, ui->machine);
 		int is_exec = elf_is_exec(fd, map->dso->name);
 		unw_word_t base = is_exec ? 0 : map->start;
 
+		dso__data_put_fd(map->dso);
+
 		memset(&di, 0, sizeof(di));
 		if (dwarf_find_debug_frame(0, &di, ip, base, map->dso->name,
 					   map->start, map->end))
-- 
2.4.0

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

[Adrian Hunter]

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.

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

> 
> 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;
> +
>  	if (dso->data.status == DSO_DATA_STATUS_ERROR)
>  		return -1;

The status check can be done before taking the lock.

>  
> -	pthread_mutex_lock(&dso__data_open_lock);
> -
>  	if (dso->data.fd < 0)
>  		try_to_open_dso(dso, machine);
>  
> -	pthread_mutex_unlock(&dso__data_open_lock);
>  	return dso->data.fd;
>  }
>  
> +void dso__data_put_fd(struct dso *dso __maybe_unused)
> +{
> +	pthread_mutex_unlock(&dso__data_open_lock);
> +}
> +
> +/**
> + * dso__data_get_fd - Get dso's data file descriptor
> + * @dso: dso object
> + * @machine: machine object
> + *
> + * Obsolete interface to find dso's file, open it and
> + * returns file descriptor.  It's not thread-safe in that
> + * the returned fd may be reused for other file.
> + */
> +int dso__data_fd(struct dso *dso, struct machine *machine)
> +{
> +	int fd = dso__data_get_fd(dso, machine);
> +
> +	dso__data_put_fd(dso);
> +	return fd;
> +}
> +
>  bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
>  {
>  	u32 flag = 1 << by;
> @@ -1198,12 +1220,14 @@ size_t dso__fprintf(struct dso *dso, enum map_type type, FILE *fp)
>  enum dso_type dso__type(struct dso *dso, struct machine *machine)
>  {
>  	int fd;
> +	enum dso_type type = DSO__TYPE_UNKNOWN;
>  
> -	fd = dso__data_fd(dso, machine);
> -	if (fd < 0)
> -		return DSO__TYPE_UNKNOWN;
> +	fd = dso__data_get_fd(dso, machine);
> +	if (fd >= 0)
> +		type = dso__type_fd(fd);
> +	dso__data_put_fd(dso);
>  
> -	return dso__type_fd(fd);
> +	return type;
>  }
>  
>  int dso__strerror_load(struct dso *dso, char *buf, size_t buflen)
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index b26ec3ab1336..de9d98c44ae2 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -240,7 +240,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>  
>  /*
>   * The dso__data_* external interface provides following functions:
> - *   dso__data_fd
> + *   dso__data_fd (obsolete)
> + *   dso__data_get_fd
> + *   dso__data_put_fd
>   *   dso__data_close
>   *   dso__data_size
>   *   dso__data_read_offset
> @@ -257,8 +259,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>   * The current usage of the dso__data_* interface is as follows:
>   *
>   * Get DSO's fd:
> - *   int fd = dso__data_fd(dso, machine);
> + *   int fd = dso__data_get_fd(dso, machine);
>   *   USE 'fd' SOMEHOW
> + *   dso__data_put_fd(dso)

Usually, with paired functions, the second of the pair is not called if the
first fails. It probably has to be that way anyway if you check the error
return from the lock function.

>   *
>   * Read DSO's data:
>   *   n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
> @@ -278,6 +281,8 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>   * TODO
>  */
>  int dso__data_fd(struct dso *dso, struct machine *machine);
> +int dso__data_get_fd(struct dso *dso, struct machine *machine);
> +void dso__data_put_fd(struct dso *dso);
>  void dso__data_close(struct dso *dso);
>  
>  off_t dso__data_size(struct dso *dso, struct machine *machine);
> diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> index 7b09a443a280..6d4ab3251729 100644
> --- a/tools/perf/util/unwind-libunwind.c
> +++ b/tools/perf/util/unwind-libunwind.c
> @@ -269,13 +269,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
>  	u64 offset = dso->data.eh_frame_hdr_offset;
>  
>  	if (offset == 0) {
> -		fd = dso__data_fd(dso, machine);
> -		if (fd < 0)
> -			return -EINVAL;
> -
> -		/* Check the .eh_frame section for unwinding info */
> -		offset = elf_section_offset(fd, ".eh_frame_hdr");
> -		dso->data.eh_frame_hdr_offset = offset;
> +		fd = dso__data_get_fd(dso, machine);
> +		if (fd >= 0) {
> +			/* Check the .eh_frame section for unwinding info */
> +			offset = elf_section_offset(fd, ".eh_frame_hdr");
> +			dso->data.eh_frame_hdr_offset = offset;
> +		}
> +		dso__data_put_fd(dso);
>  	}
>  
>  	if (offset)
> @@ -294,13 +294,19 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>  	u64 ofs = dso->data.debug_frame_offset;
>  
>  	if (ofs == 0) {
> -		fd = dso__data_fd(dso, machine);
> -		if (fd < 0)
> -			return -EINVAL;
> -
> -		/* Check the .debug_frame section for unwinding info */
> -		ofs = elf_section_offset(fd, ".debug_frame");
> -		dso->data.debug_frame_offset = ofs;
> +		int ret = 0;
> +
> +		fd = dso__data_get_fd(dso, machine);
> +		if (fd >= 0) {
> +			/* Check the .debug_frame section for unwinding info */
> +			ofs = elf_section_offset(fd, ".debug_frame");
> +			dso->data.debug_frame_offset = ofs;
> +		} else
> +			ret = -EINVAL;
> +
> +		dso__data_put_fd(dso);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	*offset = ofs;
> @@ -353,10 +359,12 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
>  #ifndef NO_LIBUNWIND_DEBUG_FRAME
>  	/* Check the .debug_frame section for unwinding info */
>  	if (!read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
> -		int fd = dso__data_fd(map->dso, ui->machine);
> +		int fd = dso__data_get_fd(map->dso, ui->machine);
>  		int is_exec = elf_is_exec(fd, map->dso->name);
>  		unw_word_t base = is_exec ? 0 : map->start;
>  
> +		dso__data_put_fd(map->dso);
> +
>  		memset(&di, 0, sizeof(di));
>  		if (dwarf_find_debug_frame(0, &di, ip, base, map->dso->name,
>  					   map->start, map->end))
> 

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

[Namhyung Kim]

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

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


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

> 
> >  
> > -	pthread_mutex_lock(&dso__data_open_lock);
> > -
> >  	if (dso->data.fd < 0)
> >  		try_to_open_dso(dso, machine);
> >  
> > -	pthread_mutex_unlock(&dso__data_open_lock);
> >  	return dso->data.fd;
> >  }
> >  
> > +void dso__data_put_fd(struct dso *dso __maybe_unused)
> > +{
> > +	pthread_mutex_unlock(&dso__data_open_lock);
> > +}
> > +
> > +/**
> > + * dso__data_get_fd - Get dso's data file descriptor
> > + * @dso: dso object
> > + * @machine: machine object
> > + *
> > + * Obsolete interface to find dso's file, open it and
> > + * returns file descriptor.  It's not thread-safe in that
> > + * the returned fd may be reused for other file.
> > + */
> > +int dso__data_fd(struct dso *dso, struct machine *machine)
> > +{
> > +	int fd = dso__data_get_fd(dso, machine);
> > +
> > +	dso__data_put_fd(dso);
> > +	return fd;
> > +}
> > +
> >  bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
> >  {
> >  	u32 flag = 1 << by;
> > @@ -1198,12 +1220,14 @@ size_t dso__fprintf(struct dso *dso, enum map_type type, FILE *fp)
> >  enum dso_type dso__type(struct dso *dso, struct machine *machine)
> >  {
> >  	int fd;
> > +	enum dso_type type = DSO__TYPE_UNKNOWN;
> >  
> > -	fd = dso__data_fd(dso, machine);
> > -	if (fd < 0)
> > -		return DSO__TYPE_UNKNOWN;
> > +	fd = dso__data_get_fd(dso, machine);
> > +	if (fd >= 0)
> > +		type = dso__type_fd(fd);
> > +	dso__data_put_fd(dso);
> >  
> > -	return dso__type_fd(fd);
> > +	return type;
> >  }
> >  
> >  int dso__strerror_load(struct dso *dso, char *buf, size_t buflen)
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index b26ec3ab1336..de9d98c44ae2 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -240,7 +240,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> >  
> >  /*
> >   * The dso__data_* external interface provides following functions:
> > - *   dso__data_fd
> > + *   dso__data_fd (obsolete)
> > + *   dso__data_get_fd
> > + *   dso__data_put_fd
> >   *   dso__data_close
> >   *   dso__data_size
> >   *   dso__data_read_offset
> > @@ -257,8 +259,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> >   * The current usage of the dso__data_* interface is as follows:
> >   *
> >   * Get DSO's fd:
> > - *   int fd = dso__data_fd(dso, machine);
> > + *   int fd = dso__data_get_fd(dso, machine);
> >   *   USE 'fd' SOMEHOW
> > + *   dso__data_put_fd(dso)
> 
> Usually, with paired functions, the second of the pair is not called if the
> first fails. It probably has to be that way anyway if you check the error
> return from the lock function.

Fair enough.

Thank you for review!
Namhyung


> 
> >   *
> >   * Read DSO's data:
> >   *   n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
> > @@ -278,6 +281,8 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> >   * TODO
> >  */
> >  int dso__data_fd(struct dso *dso, struct machine *machine);
> > +int dso__data_get_fd(struct dso *dso, struct machine *machine);
> > +void dso__data_put_fd(struct dso *dso);
> >  void dso__data_close(struct dso *dso);
> >  
> >  off_t dso__data_size(struct dso *dso, struct machine *machine);
> > diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> > index 7b09a443a280..6d4ab3251729 100644
> > --- a/tools/perf/util/unwind-libunwind.c
> > +++ b/tools/perf/util/unwind-libunwind.c
> > @@ -269,13 +269,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
> >  	u64 offset = dso->data.eh_frame_hdr_offset;
> >  
> >  	if (offset == 0) {
> > -		fd = dso__data_fd(dso, machine);
> > -		if (fd < 0)
> > -			return -EINVAL;
> > -
> > -		/* Check the .eh_frame section for unwinding info */
> > -		offset = elf_section_offset(fd, ".eh_frame_hdr");
> > -		dso->data.eh_frame_hdr_offset = offset;
> > +		fd = dso__data_get_fd(dso, machine);
> > +		if (fd >= 0) {
> > +			/* Check the .eh_frame section for unwinding info */
> > +			offset = elf_section_offset(fd, ".eh_frame_hdr");
> > +			dso->data.eh_frame_hdr_offset = offset;
> > +		}
> > +		dso__data_put_fd(dso);
> >  	}
> >  
> >  	if (offset)
> > @@ -294,13 +294,19 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
> >  	u64 ofs = dso->data.debug_frame_offset;
> >  
> >  	if (ofs == 0) {
> > -		fd = dso__data_fd(dso, machine);
> > -		if (fd < 0)
> > -			return -EINVAL;
> > -
> > -		/* Check the .debug_frame section for unwinding info */
> > -		ofs = elf_section_offset(fd, ".debug_frame");
> > -		dso->data.debug_frame_offset = ofs;
> > +		int ret = 0;
> > +
> > +		fd = dso__data_get_fd(dso, machine);
> > +		if (fd >= 0) {
> > +			/* Check the .debug_frame section for unwinding info */
> > +			ofs = elf_section_offset(fd, ".debug_frame");
> > +			dso->data.debug_frame_offset = ofs;
> > +		} else
> > +			ret = -EINVAL;
> > +
> > +		dso__data_put_fd(dso);
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> >  	*offset = ofs;
> > @@ -353,10 +359,12 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
> >  #ifndef NO_LIBUNWIND_DEBUG_FRAME
> >  	/* Check the .debug_frame section for unwinding info */
> >  	if (!read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
> > -		int fd = dso__data_fd(map->dso, ui->machine);
> > +		int fd = dso__data_get_fd(map->dso, ui->machine);
> >  		int is_exec = elf_is_exec(fd, map->dso->name);
> >  		unw_word_t base = is_exec ? 0 : map->start;
> >  
> > +		dso__data_put_fd(map->dso);
> > +
> >  		memset(&di, 0, sizeof(di));
> >  		if (dwarf_find_debug_frame(0, &di, ip, base, map->dso->name,
> >  					   map->start, map->end))
> > 
> 

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@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 ;-)

Re: [PATCH 1/4] Revert "perf tools: Fix data_read_offset() file opening"

[Arnaldo Carvalho de Melo]

Em Wed, May 20, 2015 at 03:34:04PM +0900, Namhyung Kim escreveu:
> This reverts commit 9ba6765a979f5f7d3f6456f8c79c66f4aef2d66b.
> 
> The problem will be fixed by later patch in some other way.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> Arnaldo, you can ignore this if you drop the Adrian's patch in the tree..

Ok, I did that, now if possible I'd like Adrian's Acked-by for this, ok?

- Arnaldo
 
>  tools/perf/util/dso.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index e248f5693a48..1b96c8d18435 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -756,7 +756,7 @@ off_t dso__data_size(struct dso *dso, struct machine *machine)
>  static ssize_t data_read_offset(struct dso *dso, struct machine *machine,
>  				u64 offset, u8 *data, ssize_t size)
>  {
> -	if (dso__data_size(dso, machine) < 0)
> +	if (data_file_size(dso, machine))
>  		return -1;
>  
>  	/* Check the offset sanity. */
> -- 
> 2.4.0