All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] perf test: Fix dso cache testcase
@ 2015-01-30  2:33 Namhyung Kim
  2015-01-30  2:33 ` [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso Namhyung Kim
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Namhyung Kim @ 2015-01-30  2:33 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML

The current dso cache permits to keep dso->data.fd is open under a
half of open file limit.  But test__dso_data_cache() sets dso_cnt to
limit / 2 + 1 so it'll reach the limit in the loop even though the
loop count is one less than the dso_cnt and it makes the final
dso__data_fd() after the loop meaningless.

I guess the intention was dsos[0]->data.fd is open before the last
open and gets closed after it.  So add an assert before the last open.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/dso-data.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index caaf37f079b1..22a8c428283a 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -243,8 +243,8 @@ int test__dso_data_cache(void)
 	limit = nr * 4;
 	TEST_ASSERT_VAL("failed to set file limit", !set_fd_limit(limit));
 
-	/* and this is now our dso open FDs limit + 1 extra */
-	dso_cnt = limit / 2 + 1;
+	/* and this is now our dso open FDs limit */
+	dso_cnt = limit / 2;
 	TEST_ASSERT_VAL("failed to create dsos\n",
 		!dsos__create(dso_cnt, TEST_FILE_SIZE));
 
@@ -268,7 +268,10 @@ int test__dso_data_cache(void)
 		}
 	}
 
-	/* open +1 dso over the allowed limit */
+	/* verify the first one is already open */
+	TEST_ASSERT_VAL("dsos[0] is not open", dsos[0]->data.fd != -1);
+
+	/* open +1 dso to reach the allowed limit */
 	fd = dso__data_fd(dsos[i], &machine);
 	TEST_ASSERT_VAL("failed to get fd", fd > 0);
 
-- 
2.2.2


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

* [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso
  2015-01-30  2:33 [PATCH 1/3] perf test: Fix dso cache testcase Namhyung Kim
@ 2015-01-30  2:33 ` Namhyung Kim
  2015-01-30 13:56   ` Jiri Olsa
  2015-02-18 18:24   ` [tip:perf/core] perf tests: Do not rely on dso__data_read_offset( ) " tip-bot for Namhyung Kim
  2015-01-30  2:33 ` [PATCH 3/3] perf tools: Fix a dso open fail message Namhyung Kim
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 8+ messages in thread
From: Namhyung Kim @ 2015-01-30  2:33 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML

Do not rely on dso__data_read_offset() will always call dso__data_fd()
internally.  With multi-thread support, accessing a fd will be
protected by a lock and it'll cause a huge contention.  It can be
avoided since we can skip reading from file if there's a data in the
dso cache.

If one needs to call the dso__data_read_offset(), [s]he also needs to
call dso__data_fd() (or set dso->binary_type at least) first like the
dwarf unwind code does.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/dso-data.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 22a8c428283a..513e5febbe5a 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -112,6 +112,9 @@ int test__dso_data(void)
 
 	dso = dso__new((const char *)file);
 
+	TEST_ASSERT_VAL("Failed to access to dso",
+			dso__data_fd(dso, &machine) >= 0);
+
 	/* Basic 10 bytes tests. */
 	for (i = 0; i < ARRAY_SIZE(offsets); i++) {
 		struct test_data_offset *data = &offsets[i];
@@ -252,13 +255,13 @@ int test__dso_data_cache(void)
 		struct dso *dso = dsos[i];
 
 		/*
-		 * Open dsos via dso__data_fd or dso__data_read_offset.
-		 * Both opens the data file and keep it open.
+		 * Open dsos via dso__data_fd(), it opens the data
+		 * file and keep it open (unless open file limit).
 		 */
+		fd = dso__data_fd(dso, &machine);
+		TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
 		if (i % 2) {
-			fd = dso__data_fd(dso, &machine);
-			TEST_ASSERT_VAL("failed to get fd", fd > 0);
-		} else {
 			#define BUFSIZE 10
 			u8 buf[BUFSIZE];
 			ssize_t n;
-- 
2.2.2


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

* [PATCH 3/3] perf tools: Fix a dso open fail message
  2015-01-30  2:33 [PATCH 1/3] perf test: Fix dso cache testcase Namhyung Kim
  2015-01-30  2:33 ` [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso Namhyung Kim
@ 2015-01-30  2:33 ` Namhyung Kim
  2015-02-18 18:25   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2015-01-30 13:49 ` [PATCH 1/3] perf test: Fix dso cache testcase Jiri Olsa
  2015-02-18 18:24 ` [tip:perf/core] " tip-bot for Namhyung Kim
  3 siblings, 1 reply; 8+ messages in thread
From: Namhyung Kim @ 2015-01-30  2:33 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML

It's not related to mmap, remove it from the message.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 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 c2f7d3b90966..a8b3f18db1a5 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -240,7 +240,7 @@ static int do_open(char *name)
 		if (fd >= 0)
 			return fd;
 
-		pr_debug("dso open failed, mmap: %s\n",
+		pr_debug("dso open failed: %s\n",
 			 strerror_r(errno, sbuf, sizeof(sbuf)));
 		if (!dso__data_open_cnt || errno != EMFILE)
 			break;
-- 
2.2.2


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

* Re: [PATCH 1/3] perf test: Fix dso cache testcase
  2015-01-30  2:33 [PATCH 1/3] perf test: Fix dso cache testcase Namhyung Kim
  2015-01-30  2:33 ` [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso Namhyung Kim
  2015-01-30  2:33 ` [PATCH 3/3] perf tools: Fix a dso open fail message Namhyung Kim
@ 2015-01-30 13:49 ` Jiri Olsa
  2015-02-18 18:24 ` [tip:perf/core] " tip-bot for Namhyung Kim
  3 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2015-01-30 13:49 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, LKML

On Fri, Jan 30, 2015 at 11:33:27AM +0900, Namhyung Kim wrote:
> The current dso cache permits to keep dso->data.fd is open under a
> half of open file limit.  But test__dso_data_cache() sets dso_cnt to
> limit / 2 + 1 so it'll reach the limit in the loop even though the
> loop count is one less than the dso_cnt and it makes the final
> dso__data_fd() after the loop meaningless.
> 
> I guess the intention was dsos[0]->data.fd is open before the last
> open and gets closed after it.  So add an assert before the last open.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/tests/dso-data.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
> index caaf37f079b1..22a8c428283a 100644
> --- a/tools/perf/tests/dso-data.c
> +++ b/tools/perf/tests/dso-data.c
> @@ -243,8 +243,8 @@ int test__dso_data_cache(void)
>  	limit = nr * 4;
>  	TEST_ASSERT_VAL("failed to set file limit", !set_fd_limit(limit));
>  
> -	/* and this is now our dso open FDs limit + 1 extra */
> -	dso_cnt = limit / 2 + 1;
> +	/* and this is now our dso open FDs limit */
> +	dso_cnt = limit / 2;
>  	TEST_ASSERT_VAL("failed to create dsos\n",
>  		!dsos__create(dso_cnt, TEST_FILE_SIZE));
>  
> @@ -268,7 +268,10 @@ int test__dso_data_cache(void)
>  		}
>  	}
>  
> -	/* open +1 dso over the allowed limit */
> +	/* verify the first one is already open */
> +	TEST_ASSERT_VAL("dsos[0] is not open", dsos[0]->data.fd != -1);
> +
> +	/* open +1 dso to reach the allowed limit */
>  	fd = dso__data_fd(dsos[i], &machine);
>  	TEST_ASSERT_VAL("failed to get fd", fd > 0);
>  
> -- 
> 2.2.2
> 

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

* Re: [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso
  2015-01-30  2:33 ` [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso Namhyung Kim
@ 2015-01-30 13:56   ` Jiri Olsa
  2015-02-18 18:24   ` [tip:perf/core] perf tests: Do not rely on dso__data_read_offset( ) " tip-bot for Namhyung Kim
  1 sibling, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2015-01-30 13:56 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, LKML

On Fri, Jan 30, 2015 at 11:33:28AM +0900, Namhyung Kim wrote:
> Do not rely on dso__data_read_offset() will always call dso__data_fd()
> internally.  With multi-thread support, accessing a fd will be
> protected by a lock and it'll cause a huge contention.  It can be
> avoided since we can skip reading from file if there's a data in the
> dso cache.
> 
> If one needs to call the dso__data_read_offset(), [s]he also needs to
> call dso__data_fd() (or set dso->binary_type at least) first like the
> dwarf unwind code does.

hum, I dont see the need for this, but I haven't checked your
patchset for the multithread support yet ;-)

anyway, I dont mind tp straighten up the interface:

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

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

* [tip:perf/core] perf test: Fix dso cache testcase
  2015-01-30  2:33 [PATCH 1/3] perf test: Fix dso cache testcase Namhyung Kim
                   ` (2 preceding siblings ...)
  2015-01-30 13:49 ` [PATCH 1/3] perf test: Fix dso cache testcase Jiri Olsa
@ 2015-02-18 18:24 ` tip-bot for Namhyung Kim
  3 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-02-18 18:24 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jolsa, namhyung, a.p.zijlstra, hpa, mingo, linux-kernel, acme, tglx

Commit-ID:  66af43d56345a7ca549ba1089fe11a6953072417
Gitweb:     http://git.kernel.org/tip/66af43d56345a7ca549ba1089fe11a6953072417
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 30 Jan 2015 11:33:27 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 6 Feb 2015 11:46:35 +0100

perf test: Fix dso cache testcase

The current dso cache permits to keep dso->data.fd is open under a half
of open file limit.  But test__dso_data_cache() sets dso_cnt to limit /
2 + 1 so it'll reach the limit in the loop even though the loop count is
one less than the dso_cnt and it makes the final dso__data_fd() after
the loop meaningless.

I guess the intention was dsos[0]->data.fd is open before the last open
and gets closed after it.  So add an assert before the last open.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1422585209-32742-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/dso-data.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index caaf37f..22a8c42 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -243,8 +243,8 @@ int test__dso_data_cache(void)
 	limit = nr * 4;
 	TEST_ASSERT_VAL("failed to set file limit", !set_fd_limit(limit));
 
-	/* and this is now our dso open FDs limit + 1 extra */
-	dso_cnt = limit / 2 + 1;
+	/* and this is now our dso open FDs limit */
+	dso_cnt = limit / 2;
 	TEST_ASSERT_VAL("failed to create dsos\n",
 		!dsos__create(dso_cnt, TEST_FILE_SIZE));
 
@@ -268,7 +268,10 @@ int test__dso_data_cache(void)
 		}
 	}
 
-	/* open +1 dso over the allowed limit */
+	/* verify the first one is already open */
+	TEST_ASSERT_VAL("dsos[0] is not open", dsos[0]->data.fd != -1);
+
+	/* open +1 dso to reach the allowed limit */
 	fd = dso__data_fd(dsos[i], &machine);
 	TEST_ASSERT_VAL("failed to get fd", fd > 0);
 

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

* [tip:perf/core] perf tests: Do not rely on dso__data_read_offset( ) to open dso
  2015-01-30  2:33 ` [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso Namhyung Kim
  2015-01-30 13:56   ` Jiri Olsa
@ 2015-02-18 18:24   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 8+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-02-18 18:24 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: namhyung, acme, jolsa, a.p.zijlstra, tglx, hpa, mingo, linux-kernel

Commit-ID:  63d3c6f3835d011c783c606c8a1583b041f579aa
Gitweb:     http://git.kernel.org/tip/63d3c6f3835d011c783c606c8a1583b041f579aa
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 30 Jan 2015 11:33:28 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 6 Feb 2015 11:46:35 +0100

perf tests: Do not rely on dso__data_read_offset() to open dso

Do not rely on dso__data_read_offset() will always call dso__data_fd()
internally.  With multi-thread support, accessing a fd will be protected
by a lock and it'll cause a huge contention.  It can be avoided since we
can skip reading from file if there's a data in the dso cache.

If one needs to call the dso__data_read_offset(), [s]he also needs to
call dso__data_fd() (or set dso->binary_type at least) first like the
dwarf unwind code does.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1422585209-32742-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/dso-data.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 22a8c42..513e5fe 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -112,6 +112,9 @@ int test__dso_data(void)
 
 	dso = dso__new((const char *)file);
 
+	TEST_ASSERT_VAL("Failed to access to dso",
+			dso__data_fd(dso, &machine) >= 0);
+
 	/* Basic 10 bytes tests. */
 	for (i = 0; i < ARRAY_SIZE(offsets); i++) {
 		struct test_data_offset *data = &offsets[i];
@@ -252,13 +255,13 @@ int test__dso_data_cache(void)
 		struct dso *dso = dsos[i];
 
 		/*
-		 * Open dsos via dso__data_fd or dso__data_read_offset.
-		 * Both opens the data file and keep it open.
+		 * Open dsos via dso__data_fd(), it opens the data
+		 * file and keep it open (unless open file limit).
 		 */
+		fd = dso__data_fd(dso, &machine);
+		TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
 		if (i % 2) {
-			fd = dso__data_fd(dso, &machine);
-			TEST_ASSERT_VAL("failed to get fd", fd > 0);
-		} else {
 			#define BUFSIZE 10
 			u8 buf[BUFSIZE];
 			ssize_t n;

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

* [tip:perf/core] perf tools: Fix a dso open fail message
  2015-01-30  2:33 ` [PATCH 3/3] perf tools: Fix a dso open fail message Namhyung Kim
@ 2015-02-18 18:25   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-02-18 18:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jolsa, mingo, a.p.zijlstra, acme, tglx, linux-kernel, hpa, namhyung

Commit-ID:  a3c0cc2ac03bd9db032f590d59cdbf0b447503b8
Gitweb:     http://git.kernel.org/tip/a3c0cc2ac03bd9db032f590d59cdbf0b447503b8
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 30 Jan 2015 11:33:29 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 6 Feb 2015 11:46:36 +0100

perf tools: Fix a dso open fail message

It's not related to mmap, remove it from the message.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1422585209-32742-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.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 c2f7d3b..a8b3f18 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -240,7 +240,7 @@ static int do_open(char *name)
 		if (fd >= 0)
 			return fd;
 
-		pr_debug("dso open failed, mmap: %s\n",
+		pr_debug("dso open failed: %s\n",
 			 strerror_r(errno, sbuf, sizeof(sbuf)));
 		if (!dso__data_open_cnt || errno != EMFILE)
 			break;

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

end of thread, other threads:[~2015-02-18 18:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30  2:33 [PATCH 1/3] perf test: Fix dso cache testcase Namhyung Kim
2015-01-30  2:33 ` [PATCH 2/3] perf tools: Do not rely on dso__data_read_offset() to open dso Namhyung Kim
2015-01-30 13:56   ` Jiri Olsa
2015-02-18 18:24   ` [tip:perf/core] perf tests: Do not rely on dso__data_read_offset( ) " tip-bot for Namhyung Kim
2015-01-30  2:33 ` [PATCH 3/3] perf tools: Fix a dso open fail message Namhyung Kim
2015-02-18 18:25   ` [tip:perf/core] " tip-bot for Namhyung Kim
2015-01-30 13:49 ` [PATCH 1/3] perf test: Fix dso cache testcase Jiri Olsa
2015-02-18 18:24 ` [tip:perf/core] " tip-bot for Namhyung Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.