[PATCH 0/5] perf tools: Some fixes

[PATCH 0/5] perf tools: Some fixes

[Adrian Hunter]

Hi

Here are some fixes I had to do while testing Intel PT on
32-bit. They relate to changes that are currently queued.

I emailed Jiri about the first one, but he wrote he couldn't
reproduce it, so I have just made a patch.


Adrian Hunter (5):
      perf tools: Fix function declarations needed by parse-events.y
      perf build: Fix libunwind feature detection on 32-bit x86
      perf tools: Fix parse_events_error dereferences
      perf session: Fix perf_session__peek_event()
      perf tools: Fix data_read_offset() file opening

 tools/perf/config/Makefile     |  2 +-
 tools/perf/util/dso.c          |  2 +-
 tools/perf/util/parse-events.c | 18 ++++++++++--------
 tools/perf/util/parse-events.h |  6 ++++++
 tools/perf/util/parse-events.y |  6 ++++--
 tools/perf/util/session.c      |  6 +++---
 6 files changed, 25 insertions(+), 15 deletions(-)


Regards
Adrian

[PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y

[Adrian Hunter]

Patch "perf tools: Add location to pmu event terms"
moved declarations for parse_events_term__num() and
parse_events_term__str() so that they were no longer
visible in parse-events.y. That can result in segfaults
as the arguments no longer need match the function
prototype.

Move the declarations back, changing YYLTYPE pointers
to pointers-to-void because YYLTYPE is not generated
until parse-events.y is processed.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/parse-events.c | 16 ++++++++--------
 tools/perf/util/parse-events.h |  6 ++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 80a50fd..78032d8 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -25,12 +25,6 @@
 extern int parse_events_debug;
 #endif
 int parse_events_parse(void *data, void *scanner);
-int parse_events_term__num(struct parse_events_term **term,
-			   int type_term, char *config, u64 num,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val);
-int parse_events_term__str(struct parse_events_term **term,
-			   int type_term, char *config, char *str,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val);
 
 static struct perf_pmu_event_symbol *perf_pmu_events_list;
 /*
@@ -1601,8 +1595,11 @@ static int new_term(struct parse_events_term **_term, int type_val,
 
 int parse_events_term__num(struct parse_events_term **term,
 			   int type_term, char *config, u64 num,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val)
+			   void *loc_term_, void *loc_val_)
 {
+	YYLTYPE *loc_term = loc_term_;
+	YYLTYPE *loc_val = loc_val_;
+
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_NUM, type_term,
 			config, NULL, num,
 			loc_term ? loc_term->first_column : 0,
@@ -1611,8 +1608,11 @@ int parse_events_term__num(struct parse_events_term **term,
 
 int parse_events_term__str(struct parse_events_term **term,
 			   int type_term, char *config, char *str,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val)
+			   void *loc_term_, void *loc_val_)
 {
+	YYLTYPE *loc_term = loc_term_;
+	YYLTYPE *loc_val = loc_val_;
+
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_STR, type_term,
 			config, str, 0,
 			loc_term ? loc_term->first_column : 0,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index e236f1b..131f29b 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -98,6 +98,12 @@ struct parse_events_terms {
 };
 
 int parse_events__is_hardcoded_term(struct parse_events_term *term);
+int parse_events_term__num(struct parse_events_term **term,
+			   int type_term, char *config, u64 num,
+			   void *loc_term, void *loc_val);
+int parse_events_term__str(struct parse_events_term **term,
+			   int type_term, char *config, char *str,
+			   void *loc_term, void *loc_val);
 int parse_events_term__sym_hw(struct parse_events_term **term,
 			      char *config, unsigned idx);
 int parse_events_term__clone(struct parse_events_term **new,
-- 
1.9.1

[tip:perf/core] perf tools: Fix function declarations needed by parse-events.y

[tip-bot for Adrian Hunter]

Commit-ID:  bb78ce7d0598fb277290f8ee2443b8f4e0eb7cb2
Gitweb:     http://git.kernel.org/tip/bb78ce7d0598fb277290f8ee2443b8f4e0eb7cb2
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Tue, 19 May 2015 16:05:42 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 May 2015 12:21:43 -0300

perf tools: Fix function declarations needed by parse-events.y

Patch "perf tools: Add location to pmu event terms" moved declarations
for parse_events_term__num() and parse_events_term__str() so that they
were no longer visible in parse-events.y. That can result in segfaults
as the arguments no longer need match the function prototype.

Move the declarations back, changing YYLTYPE pointers to
pointers-to-void because YYLTYPE is not generated until parse-events.y
is processed.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@gmail.com>
Link: http://lkml.kernel.org/r/1432040746-1755-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.c | 16 ++++++++--------
 tools/perf/util/parse-events.h |  6 ++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 80a50fd..78032d8 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -25,12 +25,6 @@
 extern int parse_events_debug;
 #endif
 int parse_events_parse(void *data, void *scanner);
-int parse_events_term__num(struct parse_events_term **term,
-			   int type_term, char *config, u64 num,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val);
-int parse_events_term__str(struct parse_events_term **term,
-			   int type_term, char *config, char *str,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val);
 
 static struct perf_pmu_event_symbol *perf_pmu_events_list;
 /*
@@ -1601,8 +1595,11 @@ static int new_term(struct parse_events_term **_term, int type_val,
 
 int parse_events_term__num(struct parse_events_term **term,
 			   int type_term, char *config, u64 num,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val)
+			   void *loc_term_, void *loc_val_)
 {
+	YYLTYPE *loc_term = loc_term_;
+	YYLTYPE *loc_val = loc_val_;
+
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_NUM, type_term,
 			config, NULL, num,
 			loc_term ? loc_term->first_column : 0,
@@ -1611,8 +1608,11 @@ int parse_events_term__num(struct parse_events_term **term,
 
 int parse_events_term__str(struct parse_events_term **term,
 			   int type_term, char *config, char *str,
-			   YYLTYPE *loc_term, YYLTYPE *loc_val)
+			   void *loc_term_, void *loc_val_)
 {
+	YYLTYPE *loc_term = loc_term_;
+	YYLTYPE *loc_val = loc_val_;
+
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_STR, type_term,
 			config, str, 0,
 			loc_term ? loc_term->first_column : 0,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index e236f1b..131f29b 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -98,6 +98,12 @@ struct parse_events_terms {
 };
 
 int parse_events__is_hardcoded_term(struct parse_events_term *term);
+int parse_events_term__num(struct parse_events_term **term,
+			   int type_term, char *config, u64 num,
+			   void *loc_term, void *loc_val);
+int parse_events_term__str(struct parse_events_term **term,
+			   int type_term, char *config, char *str,
+			   void *loc_term, void *loc_val);
 int parse_events_term__sym_hw(struct parse_events_term **term,
 			      char *config, unsigned idx);
 int parse_events_term__clone(struct parse_events_term **new,

[PATCH 2/5] perf build: Fix libunwind feature detection on 32-bit x86

[Adrian Hunter]

The libunwind feature would never detect because of the following error:

$ cat tools/build/feature/test-libunwind.make.output
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_stream_buffer_decode'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_uncompressed_size'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_end'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_buffer_decode'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_stream_footer_decode'
/usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_size'
collect2: error: ld returned 1 exit status

Fix by adding -llzma and re-ordering to match the dependencies.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/config/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 1b957a1..e3b3724 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -32,7 +32,7 @@ ifeq ($(ARCH),x86)
     LIBUNWIND_LIBS = -lunwind -lunwind-x86_64
     $(call detected,CONFIG_X86_64)
   else
-    LIBUNWIND_LIBS = -lunwind -lunwind-x86
+    LIBUNWIND_LIBS = -lunwind-x86 -llzma -lunwind
   endif
   NO_PERF_REGS := 0
 endif
-- 
1.9.1

[tip:perf/core] perf build: Fix libunwind feature detection on 32-bit x86

[tip-bot for Adrian Hunter]

Commit-ID:  05b41775e2edd69a83f592e3534930c934d4038e
Gitweb:     http://git.kernel.org/tip/05b41775e2edd69a83f592e3534930c934d4038e
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Tue, 19 May 2015 16:05:43 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 May 2015 12:21:43 -0300

perf build: Fix libunwind feature detection on 32-bit x86

The libunwind feature would never detect because of the following error:

  $ cat tools/build/feature/test-libunwind.make.output
  /usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_stream_buffer_decode'
  /usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_uncompressed_size'
  /usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_end'
  /usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_buffer_decode'
  /usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_stream_footer_decode'
  /usr/lib/gcc/i686-linux-gnu/4.8/../../../i386-linux-gnu/libunwind-x86.so: undefined reference to `lzma_index_size'
  collect2: error: ld returned 1 exit status

Fix by adding -llzma and re-ordering to match the dependencies.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@gmail.com>
Link: http://lkml.kernel.org/r/1432040746-1755-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/config/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 1b957a1..e3b3724 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -32,7 +32,7 @@ ifeq ($(ARCH),x86)
     LIBUNWIND_LIBS = -lunwind -lunwind-x86_64
     $(call detected,CONFIG_X86_64)
   else
-    LIBUNWIND_LIBS = -lunwind -lunwind-x86
+    LIBUNWIND_LIBS = -lunwind-x86 -llzma -lunwind
   endif
   NO_PERF_REGS := 0
 endif

[PATCH 3/5] perf tools: Fix parse_events_error dereferences

[Adrian Hunter]

Parse errors can be reported in struct parse_events_error
but the pointer passed is optional and can be NULL.
Ensure it is not NULL before dereferencing it.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/parse-events.c | 2 ++
 tools/perf/util/parse-events.y | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 78032d8..2a4d1ec 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1659,6 +1659,8 @@ void parse_events_evlist_error(struct parse_events_evlist *data,
 {
 	struct parse_events_error *err = data->error;
 
+	if (!err)
+		return;
 	err->idx = idx;
 	err->str = strdup(str);
 	WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 3d11e00..591905a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -389,8 +389,10 @@ PE_NAME ':' PE_NAME
 	if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
 		struct parse_events_error *error = data->error;
 
-		error->idx = @1.first_column;
-		error->str = strdup("unknown tracepoint");
+		if (error) {
+			error->idx = @1.first_column;
+			error->str = strdup("unknown tracepoint");
+		}
 		return -1;
 	}
 	$$ = list;
-- 
1.9.1

[tip:perf/core] perf tools: Fix parse_events_error dereferences

[tip-bot for Adrian Hunter]

Commit-ID:  a6ced2be06c302402c52dedba97d169d22cce99c
Gitweb:     http://git.kernel.org/tip/a6ced2be06c302402c52dedba97d169d22cce99c
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Tue, 19 May 2015 16:05:44 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 May 2015 12:21:43 -0300

perf tools: Fix parse_events_error dereferences

Parse errors can be reported in struct parse_events_error but the
pointer passed is optional and can be NULL.  Ensure it is not NULL
before dereferencing it.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@gmail.com>
Link: http://lkml.kernel.org/r/1432040746-1755-4-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.c | 2 ++
 tools/perf/util/parse-events.y | 6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 78032d8..2a4d1ec 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1659,6 +1659,8 @@ void parse_events_evlist_error(struct parse_events_evlist *data,
 {
 	struct parse_events_error *err = data->error;
 
+	if (!err)
+		return;
 	err->idx = idx;
 	err->str = strdup(str);
 	WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 3d11e00..591905a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -389,8 +389,10 @@ PE_NAME ':' PE_NAME
 	if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
 		struct parse_events_error *error = data->error;
 
-		error->idx = @1.first_column;
-		error->str = strdup("unknown tracepoint");
+		if (error) {
+			error->idx = @1.first_column;
+			error->str = strdup("unknown tracepoint");
+		}
 		return -1;
 	}
 	$$ = list;

[PATCH 4/5] perf session: Fix perf_session__peek_event()

[Adrian Hunter]

perf_session__peek_event() generally leverages there
being a single mmap of the perf.data file, however on
32-bit platforms when there is more that 32MiB of
data, then there are multiple mmaps, so
perf_session__peek_event() reads from the file.

In that case a couple of bugs were exposed (note
how the seg. fault appears with >32M of data):

   $ perf record --per-thread -e intel_bts// ../rtit-tests/loopy 1000000
   [ perf record: Woken up 13 times to write data ]
   [ perf record: Captured and wrote 24.568 MB perf.data ]
   $ perf script > /dev/null
   $ perf record --per-thread -e intel_bts// ../rtit-tests/loopy 10000000
   [ perf record: Woken up 136 times to write data ]
   [ perf record: Captured and wrote 270.794 MB perf.data ]
   $ perf script > /dev/null
   Segmentation fault (core dumped)

The wrong address was being passed to the readn() function and
the buffer size was not being checked.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/session.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index e722107..39fe09d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1182,7 +1182,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 		return -1;
 
 	if (lseek(fd, file_offset, SEEK_SET) == (off_t)-1 ||
-	    readn(fd, &buf, hdr_sz) != (ssize_t)hdr_sz)
+	    readn(fd, buf, hdr_sz) != (ssize_t)hdr_sz)
 		return -1;
 
 	event = (union perf_event *)buf;
@@ -1190,12 +1190,12 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 	if (session->header.needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	if (event->header.size < hdr_sz)
+	if (event->header.size < hdr_sz || event->header.size > buf_sz)
 		return -1;
 
 	rest = event->header.size - hdr_sz;
 
-	if (readn(fd, &buf, rest) != (ssize_t)rest)
+	if (readn(fd, buf, rest) != (ssize_t)rest)
 		return -1;
 
 	if (session->header.needs_swap)
-- 
1.9.1

[tip:perf/core] perf session: Fix perf_session__peek_event()

[tip-bot for Adrian Hunter]

Commit-ID:  554e92ed8fcdbcad736ef906c393847d44d52692
Gitweb:     http://git.kernel.org/tip/554e92ed8fcdbcad736ef906c393847d44d52692
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Tue, 19 May 2015 16:05:45 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 May 2015 12:21:43 -0300

perf session: Fix perf_session__peek_event()

perf_session__peek_event() generally leverages there being a single mmap
of the perf.data file, however on 32-bit platforms when there is more
that 32MiB of data, then there are multiple mmaps, so
perf_session__peek_event() reads from the file.

In that case a couple of bugs were exposed (note how the seg. fault
appears with >32M of data):

   $ perf record --per-thread -e intel_bts// ../rtit-tests/loopy 1000000
   [ perf record: Woken up 13 times to write data ]
   [ perf record: Captured and wrote 24.568 MB perf.data ]
   $ perf script > /dev/null
   $ perf record --per-thread -e intel_bts// ../rtit-tests/loopy 10000000
   [ perf record: Woken up 136 times to write data ]
   [ perf record: Captured and wrote 270.794 MB perf.data ]
   $ perf script > /dev/null
   Segmentation fault (core dumped)

The wrong address was being passed to the readn() function and the
buffer size was not being checked.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@gmail.com>
Link: http://lkml.kernel.org/r/1432040746-1755-5-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index e722107..39fe09d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1182,7 +1182,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 		return -1;
 
 	if (lseek(fd, file_offset, SEEK_SET) == (off_t)-1 ||
-	    readn(fd, &buf, hdr_sz) != (ssize_t)hdr_sz)
+	    readn(fd, buf, hdr_sz) != (ssize_t)hdr_sz)
 		return -1;
 
 	event = (union perf_event *)buf;
@@ -1190,12 +1190,12 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 	if (session->header.needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	if (event->header.size < hdr_sz)
+	if (event->header.size < hdr_sz || event->header.size > buf_sz)
 		return -1;
 
 	rest = event->header.size - hdr_sz;
 
-	if (readn(fd, &buf, rest) != (ssize_t)rest)
+	if (readn(fd, buf, rest) != (ssize_t)rest)
 		return -1;
 
 	if (session->header.needs_swap)

[PATCH 5/5] perf tools: Fix data_read_offset() file opening

[Adrian Hunter]

Patch "perf tools: Protect dso cache fd with a mutex"
changed data_file_size() to open the data file if it
was not open.

data_read_offset() was calling data_file_size() to read
the data file size, but data_file_size() can fail to
open the file because the binary_type has not been set up.

The correct function to call is dso__data_size() which
uses dso__data_fd() to open the file correctly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 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 1b96c8d..e248f56 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 (data_file_size(dso, machine))
+	if (dso__data_size(dso, machine) < 0)
 		return -1;
 
 	/* Check the offset sanity. */
-- 
1.9.1

Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening

[Namhyung Kim]

Hi Adrian,

On Tue, May 19, 2015 at 04:05:46PM +0300, Adrian Hunter wrote:
> Patch "perf tools: Protect dso cache fd with a mutex"
> changed data_file_size() to open the data file if it
> was not open.
> 
> data_read_offset() was calling data_file_size() to read
> the data file size, but data_file_size() can fail to
> open the file because the binary_type has not been set up.
> 
> The correct function to call is dso__data_size() which
> uses dso__data_fd() to open the file correctly.

Right, but I worried about the locking overhead.  By using
dso__data_size() it'll call dso__data_fd() everytime which grabs the
dso__data_open_lock that is a global mutex.  It can be a performance
bottleneck on multi-thread report IMHO.

I assumed that correct code should check the data fd or size before
calling read function as I read the comment in dso.h file.

What about adding the missing check in a proper place instead?

Thanks,
Namhyung


> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  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 1b96c8d..e248f56 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 (data_file_size(dso, machine))
> +	if (dso__data_size(dso, machine) < 0)
>  		return -1;
>  
>  	/* Check the offset sanity. */
> -- 
> 1.9.1
> 

Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening

[Adrian Hunter]

On 19/05/2015 5:48 p.m., Namhyung Kim wrote:
> Hi Adrian,
>
> On Tue, May 19, 2015 at 04:05:46PM +0300, Adrian Hunter wrote:
>> Patch "perf tools: Protect dso cache fd with a mutex"
>> changed data_file_size() to open the data file if it
>> was not open.
>>
>> data_read_offset() was calling data_file_size() to read
>> the data file size, but data_file_size() can fail to
>> open the file because the binary_type has not been set up.
>>
>> The correct function to call is dso__data_size() which
>> uses dso__data_fd() to open the file correctly.
>
> Right, but I worried about the locking overhead.  By using
> dso__data_size() it'll call dso__data_fd() everytime which grabs the
> dso__data_open_lock that is a global mutex.  It can be a performance
> bottleneck on multi-thread report IMHO.
>
> I assumed that correct code should check the data fd or size before
> calling read function as I read the comment in dso.h file.
>
> What about adding the missing check in a proper place instead?

It looks to me like you need to change data_file_size() and
dso_cache__read() so that they open the fd properly i.e. call
dso__data_fd() instead of open_dso().

dso__data_fd() should not be taking a lock because the lock must
be held while the fd in being used.

Perhaps there should be dso__get_fd() and dso__put_fd() that lock/open
and the unlock.

But I will have to leave this to you because I am snowed with
my own problems.

>
> Thanks,
> Namhyung
>
>
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>   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 1b96c8d..e248f56 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 (data_file_size(dso, machine))
>> +	if (dso__data_size(dso, machine) < 0)
>>   		return -1;
>>
>>   	/* Check the offset sanity. */
>> --
>> 1.9.1
>>

Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening

[Namhyung Kim]

On Tue, May 19, 2015 at 10:58:43PM +0300, Adrian Hunter wrote:
> On 19/05/2015 5:48 p.m., Namhyung Kim wrote:
> >Hi Adrian,
> >
> >On Tue, May 19, 2015 at 04:05:46PM +0300, Adrian Hunter wrote:
> >>Patch "perf tools: Protect dso cache fd with a mutex"
> >>changed data_file_size() to open the data file if it
> >>was not open.
> >>
> >>data_read_offset() was calling data_file_size() to read
> >>the data file size, but data_file_size() can fail to
> >>open the file because the binary_type has not been set up.
> >>
> >>The correct function to call is dso__data_size() which
> >>uses dso__data_fd() to open the file correctly.
> >
> >Right, but I worried about the locking overhead.  By using
> >dso__data_size() it'll call dso__data_fd() everytime which grabs the
> >dso__data_open_lock that is a global mutex.  It can be a performance
> >bottleneck on multi-thread report IMHO.
> >
> >I assumed that correct code should check the data fd or size before
> >calling read function as I read the comment in dso.h file.
> >
> >What about adding the missing check in a proper place instead?
> 
> It looks to me like you need to change data_file_size() and
> dso_cache__read() so that they open the fd properly i.e. call
> dso__data_fd() instead of open_dso().

OK, I'll change it to have a proper binary type before calling
open_dso() like dso__data_fd() does.  But I still think it's good to
open data fd explicitly before reading from dso cache.


> 
> dso__data_fd() should not be taking a lock because the lock must
> be held while the fd in being used.
> 
> Perhaps there should be dso__get_fd() and dso__put_fd() that lock/open
> and the unlock.

That is already a part of my patchset but not merged yet.


> 
> But I will have to leave this to you because I am snowed with
> my own problems.

No problem.  I can see you're doing hardwork ;-)

Thanks,
Namhyung

Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening

[Arnaldo Carvalho de Melo]

Em Wed, May 20, 2015 at 09:44:37AM +0900, Namhyung Kim escreveu:
> On Tue, May 19, 2015 at 10:58:43PM +0300, Adrian Hunter wrote:
> > On 19/05/2015 5:48 p.m., Namhyung Kim wrote:
> > >Hi Adrian,
> > >
> > >On Tue, May 19, 2015 at 04:05:46PM +0300, Adrian Hunter wrote:
> > >>Patch "perf tools: Protect dso cache fd with a mutex"
> > >>changed data_file_size() to open the data file if it
> > >>was not open.
> > >>
> > >>data_read_offset() was calling data_file_size() to read
> > >>the data file size, but data_file_size() can fail to
> > >>open the file because the binary_type has not been set up.
> > >>
> > >>The correct function to call is dso__data_size() which
> > >>uses dso__data_fd() to open the file correctly.
> > >
> > >Right, but I worried about the locking overhead.  By using
> > >dso__data_size() it'll call dso__data_fd() everytime which grabs the
> > >dso__data_open_lock that is a global mutex.  It can be a performance
> > >bottleneck on multi-thread report IMHO.
> > >
> > >I assumed that correct code should check the data fd or size before
> > >calling read function as I read the comment in dso.h file.
> > >
> > >What about adding the missing check in a proper place instead?
> > 
> > It looks to me like you need to change data_file_size() and
> > dso_cache__read() so that they open the fd properly i.e. call
> > dso__data_fd() instead of open_dso().
> 
> OK, I'll change it to have a proper binary type before calling
> open_dso() like dso__data_fd() does.  But I still think it's good to
> open data fd explicitly before reading from dso cache.
> 
> 
> > 
> > dso__data_fd() should not be taking a lock because the lock must
> > be held while the fd in being used.
> > 
> > Perhaps there should be dso__get_fd() and dso__put_fd() that lock/open
> > and the unlock.
> 
> That is already a part of my patchset but not merged yet.

Ok, I have Adrian's patch in my perf/core branch, tomorrow I'll check
what you guys ended up agreeing on so that I can do the adjustments
before pushing to Ingo, ok?
 
> > But I will have to leave this to you because I am snowed with
> > my own problems.
> 
> No problem.  I can see you're doing hardwork ;-)

:-)

- Arnaldo

Re: [PATCH 0/5] perf tools: Some fixes

[Arnaldo Carvalho de Melo]

Em Tue, May 19, 2015 at 04:05:41PM +0300, Adrian Hunter escreveu:
> Hi
> 
> Here are some fixes I had to do while testing Intel PT on
> 32-bit. They relate to changes that are currently queued.
> 
> I emailed Jiri about the first one, but he wrote he couldn't
> reproduce it, so I have just made a patch.
> 
> 
> Adrian Hunter (5):
>       perf tools: Fix function declarations needed by parse-events.y
>       perf build: Fix libunwind feature detection on 32-bit x86
>       perf tools: Fix parse_events_error dereferences
>       perf session: Fix perf_session__peek_event()
>       perf tools: Fix data_read_offset() file opening

All looks reasonable, applied to my local tree.

Would be good to get Acks from Namhyung on the mutex one and from Jiri
on the parse events stuff, probably the data_readd_offset() as well (the
mutex one).

- Arnaldo