linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] perf tools: Some fixes
@ 2015-05-19 13:05 Adrian Hunter
  2015-05-19 13:05 ` [PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y Adrian Hunter
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 13:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y
  2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
@ 2015-05-19 13:05 ` Adrian Hunter
  2015-05-27 16:45   ` [tip:perf/core] " tip-bot for Adrian Hunter
  2015-05-19 13:05 ` [PATCH 2/5] perf build: Fix libunwind feature detection on 32-bit x86 Adrian Hunter
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 13:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/5] perf build: Fix libunwind feature detection on 32-bit x86
  2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
  2015-05-19 13:05 ` [PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y Adrian Hunter
@ 2015-05-19 13:05 ` Adrian Hunter
  2015-05-27 16:46   ` [tip:perf/core] " tip-bot for Adrian Hunter
  2015-05-19 13:05 ` [PATCH 3/5] perf tools: Fix parse_events_error dereferences Adrian Hunter
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 13:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] perf tools: Fix parse_events_error dereferences
  2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
  2015-05-19 13:05 ` [PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y Adrian Hunter
  2015-05-19 13:05 ` [PATCH 2/5] perf build: Fix libunwind feature detection on 32-bit x86 Adrian Hunter
@ 2015-05-19 13:05 ` Adrian Hunter
  2015-05-27 16:46   ` [tip:perf/core] " tip-bot for Adrian Hunter
  2015-05-19 13:05 ` [PATCH 4/5] perf session: Fix perf_session__peek_event() Adrian Hunter
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 13:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] perf session: Fix perf_session__peek_event()
  2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
                   ` (2 preceding siblings ...)
  2015-05-19 13:05 ` [PATCH 3/5] perf tools: Fix parse_events_error dereferences Adrian Hunter
@ 2015-05-19 13:05 ` Adrian Hunter
  2015-05-27 16:46   ` [tip:perf/core] " tip-bot for Adrian Hunter
  2015-05-19 13:05 ` [PATCH 5/5] perf tools: Fix data_read_offset() file opening Adrian Hunter
  2015-05-19 14:00 ` [PATCH 0/5] perf tools: Some fixes Arnaldo Carvalho de Melo
  5 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 13:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] perf tools: Fix data_read_offset() file opening
  2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
                   ` (3 preceding siblings ...)
  2015-05-19 13:05 ` [PATCH 4/5] perf session: Fix perf_session__peek_event() Adrian Hunter
@ 2015-05-19 13:05 ` Adrian Hunter
  2015-05-19 14:48   ` Namhyung Kim
  2015-05-19 14:00 ` [PATCH 0/5] perf tools: Some fixes Arnaldo Carvalho de Melo
  5 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 13:05 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] perf tools: Some fixes
  2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
                   ` (4 preceding siblings ...)
  2015-05-19 13:05 ` [PATCH 5/5] perf tools: Fix data_read_offset() file opening Adrian Hunter
@ 2015-05-19 14:00 ` Arnaldo Carvalho de Melo
  5 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-05-19 14:00 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: linux-kernel, Jiri Olsa, Namhyung Kim

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening
  2015-05-19 13:05 ` [PATCH 5/5] perf tools: Fix data_read_offset() file opening Adrian Hunter
@ 2015-05-19 14:48   ` Namhyung Kim
  2015-05-19 19:58     ` Adrian Hunter
  0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2015-05-19 14:48 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa

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
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening
  2015-05-19 14:48   ` Namhyung Kim
@ 2015-05-19 19:58     ` Adrian Hunter
  2015-05-20  0:44       ` Namhyung Kim
  0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2015-05-19 19:58 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening
  2015-05-19 19:58     ` Adrian Hunter
@ 2015-05-20  0:44       ` Namhyung Kim
  2015-05-20  0:55         ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2015-05-20  0:44 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] perf tools: Fix data_read_offset() file opening
  2015-05-20  0:44       ` Namhyung Kim
@ 2015-05-20  0:55         ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-05-20  0:55 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: Adrian Hunter, linux-kernel, Jiri Olsa

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [tip:perf/core] perf tools: Fix function declarations needed by parse-events.y
  2015-05-19 13:05 ` [PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y Adrian Hunter
@ 2015-05-27 16:45   ` tip-bot for Adrian Hunter
  0 siblings, 0 replies; 15+ messages in thread
From: tip-bot for Adrian Hunter @ 2015-05-27 16:45 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: adrian.hunter, jolsa, linux-kernel, mingo, acme, tglx, namhyung, hpa

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,

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [tip:perf/core] perf tools: Fix parse_events_error dereferences
  2015-05-19 13:05 ` [PATCH 3/5] perf tools: Fix parse_events_error dereferences Adrian Hunter
@ 2015-05-27 16:46   ` tip-bot for Adrian Hunter
  0 siblings, 0 replies; 15+ messages in thread
From: tip-bot for Adrian Hunter @ 2015-05-27 16:46 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, namhyung, adrian.hunter, jolsa, hpa, linux-kernel, acme, mingo

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;

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [tip:perf/core] perf build: Fix libunwind feature detection on 32-bit x86
  2015-05-19 13:05 ` [PATCH 2/5] perf build: Fix libunwind feature detection on 32-bit x86 Adrian Hunter
@ 2015-05-27 16:46   ` tip-bot for Adrian Hunter
  0 siblings, 0 replies; 15+ messages in thread
From: tip-bot for Adrian Hunter @ 2015-05-27 16:46 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, adrian.hunter, namhyung, acme, mingo, jolsa, hpa, tglx

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [tip:perf/core] perf session: Fix perf_session__peek_event()
  2015-05-19 13:05 ` [PATCH 4/5] perf session: Fix perf_session__peek_event() Adrian Hunter
@ 2015-05-27 16:46   ` tip-bot for Adrian Hunter
  0 siblings, 0 replies; 15+ messages in thread
From: tip-bot for Adrian Hunter @ 2015-05-27 16:46 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jolsa, namhyung, linux-kernel, mingo, hpa, tglx, acme, 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)

^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-05-27 16:47 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-19 13:05 [PATCH 0/5] perf tools: Some fixes Adrian Hunter
2015-05-19 13:05 ` [PATCH 1/5] perf tools: Fix function declarations needed by parse-events.y Adrian Hunter
2015-05-27 16:45   ` [tip:perf/core] " tip-bot for Adrian Hunter
2015-05-19 13:05 ` [PATCH 2/5] perf build: Fix libunwind feature detection on 32-bit x86 Adrian Hunter
2015-05-27 16:46   ` [tip:perf/core] " tip-bot for Adrian Hunter
2015-05-19 13:05 ` [PATCH 3/5] perf tools: Fix parse_events_error dereferences Adrian Hunter
2015-05-27 16:46   ` [tip:perf/core] " tip-bot for Adrian Hunter
2015-05-19 13:05 ` [PATCH 4/5] perf session: Fix perf_session__peek_event() Adrian Hunter
2015-05-27 16:46   ` [tip:perf/core] " tip-bot for Adrian Hunter
2015-05-19 13:05 ` [PATCH 5/5] perf tools: Fix data_read_offset() file opening Adrian Hunter
2015-05-19 14:48   ` Namhyung Kim
2015-05-19 19:58     ` Adrian Hunter
2015-05-20  0:44       ` Namhyung Kim
2015-05-20  0:55         ` Arnaldo Carvalho de Melo
2015-05-19 14:00 ` [PATCH 0/5] perf tools: Some fixes Arnaldo Carvalho de Melo

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