[PATCHv2 00/14] perf tools: Speedup DWARF unwind

[PATCHv2 00/14] perf tools: Speedup DWARF unwind

[Jiri Olsa]

hi,
trying to speedup DWARF unwind report code by factoring
related code:
  - caching sample's registers access
  - keep dso data file descriptor open for the
    life of the dso object
  - replace dso cache code by mapping dso data file
    directly for the life of the dso object

The speedup is mainly for libunwind unwind. The libdw will benefit
mainly from cached registers access, because it handles dso data
accesses by itself.. and anyway it's still faster ;-).

v2 changes:
  - adding limit for open dso objects with sort of LRU
    mechanism to pick up and close dso objects if we
    are over the limit
  - file mmaping changes are omitted, because I couldn't prove
    the improvement exactly, will resubmit later
  - added dso close logic in case of no memory
  - added tests

On 10GB perf data file with dwarf unwind data I've got
around 30% speed up.

Also reachable in here:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/core_unwind_speedup

thanks,
jirka

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
Jiri Olsa (14):
      perf tools: Cache register accesses for unwind processing
      perf tools: Separate dso data related variables
      perf tools: Add data_fd into dso object
      perf tools: Add global list of opened dso objects
      perf tools: Add global count of opened dso objects
      perf tools: Cache dso data file descriptor
      perf tools: Add file size check and factor dso__data_read_offset
      perf tools: Allow to close dso fd in case of open failure
      perf tools: Add dso__data_* interface descriptons
      perf tests: Spawn child for each test
      perf tests: Allow reuse of test_file function
      perf tests: Add test interface for dso data fd limit
      perf tests: Add test for caching dso file descriptors
      perf tests: Add test for closing dso objects on EMFILE error

 tools/perf/tests/builtin-test.c    |  42 ++++++++++++++-
 tools/perf/tests/dso-data.c        | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 tools/perf/tests/tests.h           |   2 +
 tools/perf/util/dso.c              | 274 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 tools/perf/util/dso.h              |  56 +++++++++++++++++++-
 tools/perf/util/event.h            |   5 ++
 tools/perf/util/perf_regs.c        |  10 +++-
 tools/perf/util/perf_regs.h        |   4 +-
 tools/perf/util/unwind-libunwind.c |   4 +-
 9 files changed, 617 insertions(+), 36 deletions(-)

[PATCH 01/14] perf tools: Cache register accesses for unwind processing

[Jiri Olsa]

Caching registers value into an array. Got about 4% speed up
of perf_reg_value function for report command processing
dwarf unwind stacks.

Output from report over 1.5 GB data with DWARF unwind stacks:
(TODO fix perf diff)

  current code:
   5.84%     perf  perf                       [.] perf_reg_value
  change:
   1.94%     perf  perf                       [.] perf_reg_value

And little bit of overall speed up:
(perf stat -r 5 -e '{cycles,instructions}:u' ...)

  current code:
   310,298,611,754      cycles                     ( +-  0.33% )
   439,669,689,341      instructions               ( +-  0.03% )

     188.656753166 seconds time elapsed            ( +-  0.82% )

  change:
   291,315,329,878      cycles                     ( +-  0.22% )
   391,763,485,304      instructions               ( +-  0.03%  )

     180.742249687 seconds time elapsed            ( +-  0.64% )

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/event.h     |  5 +++++
 tools/perf/util/perf_regs.c | 10 +++++++++-
 tools/perf/util/perf_regs.h |  4 +++-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index d970232..d369ad9 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -7,6 +7,7 @@
 #include "../perf.h"
 #include "map.h"
 #include "build-id.h"
+#include "perf_regs.h"
 
 struct mmap_event {
 	struct perf_event_header header;
@@ -87,6 +88,10 @@ struct regs_dump {
 	u64 abi;
 	u64 mask;
 	u64 *regs;
+
+	/* Cached values/mask filled by first register access. */
+	u64 cache_regs[PERF_REGS_MAX];
+	u64 cache_mask;
 };
 
 struct stack_dump {
diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
index a3539ef..43168fb 100644
--- a/tools/perf/util/perf_regs.c
+++ b/tools/perf/util/perf_regs.c
@@ -1,11 +1,15 @@
 #include <errno.h>
 #include "perf_regs.h"
+#include "event.h"
 
 int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
 {
 	int i, idx = 0;
 	u64 mask = regs->mask;
 
+	if (regs->cache_mask & (1 << id))
+		goto out;
+
 	if (!(mask & (1 << id)))
 		return -EINVAL;
 
@@ -14,6 +18,10 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
 			idx++;
 	}
 
-	*valp = regs->regs[idx];
+	regs->cache_mask |= (1 << id);
+	regs->cache_regs[id] = regs->regs[idx];
+
+out:
+	*valp = regs->cache_regs[id];
 	return 0;
 }
diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
index 79c78f7..980dbf7 100644
--- a/tools/perf/util/perf_regs.h
+++ b/tools/perf/util/perf_regs.h
@@ -2,7 +2,8 @@
 #define __PERF_REGS_H
 
 #include <linux/types.h>
-#include "event.h"
+
+struct regs_dump;
 
 #ifdef HAVE_PERF_REGS_SUPPORT
 #include <perf_regs.h>
@@ -11,6 +12,7 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id);
 
 #else
 #define PERF_REGS_MASK	0
+#define PERF_REGS_MAX	0
 
 static inline const char *perf_reg_name(int id __maybe_unused)
 {
-- 
1.8.3.1

[PATCH 02/14] perf tools: Separate dso data related variables

[Jiri Olsa]

Add separated structure/namespace for data related
variables. We are going to add mode of them, so this
way they will be clearly separated.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 8 ++++----
 tools/perf/util/dso.h | 7 ++++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 64453d6..1c3cdaf 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -292,7 +292,7 @@ dso_cache__read(struct dso *dso, struct machine *machine,
 
 		cache->offset = cache_offset;
 		cache->size   = ret;
-		dso_cache__insert(&dso->cache, cache);
+		dso_cache__insert(&dso->data.cache, cache);
 
 		ret = dso_cache__memcpy(cache, offset, data, size);
 
@@ -310,7 +310,7 @@ static ssize_t dso_cache_read(struct dso *dso, struct machine *machine,
 {
 	struct dso_cache *cache;
 
-	cache = dso_cache__find(&dso->cache, offset);
+	cache = dso_cache__find(&dso->data.cache, offset);
 	if (cache)
 		return dso_cache__memcpy(cache, offset, data, size);
 	else
@@ -473,7 +473,7 @@ struct dso *dso__new(const char *name)
 		dso__set_short_name(dso, dso->name, false);
 		for (i = 0; i < MAP__NR_TYPES; ++i)
 			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
-		dso->cache = RB_ROOT;
+		dso->data.cache = RB_ROOT;
 		dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
 		dso->binary_type = DSO_BINARY_TYPE__NOT_FOUND;
 		dso->loaded = 0;
@@ -506,7 +506,7 @@ void dso__delete(struct dso *dso)
 		dso->long_name_allocated = false;
 	}
 
-	dso_cache__free(&dso->cache);
+	dso_cache__free(&dso->data.cache);
 	dso__free_a2l(dso);
 	zfree(&dso->symsrc_filename);
 	free(dso);
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 38efe95..7637fdd 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -76,7 +76,6 @@ struct dso {
 	struct list_head node;
 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];
-	struct rb_root	 cache;
 	void		 *a2l;
 	char		 *symsrc_filename;
 	unsigned int	 a2l_fails;
@@ -99,6 +98,12 @@ struct dso {
 	const char	 *long_name;
 	u16		 long_name_len;
 	u16		 short_name_len;
+
+	/* dso data file */
+	struct {
+		struct rb_root	 cache;
+	} data;
+
 	char		 name[0];
 };
 
-- 
1.8.3.1

[PATCH 03/14] perf tools: Add data_fd into dso object

[Jiri Olsa]

Adding data_fd into dso object so we could handle caching
of opened dso file data descriptors coming int next patches.

Adding dso__data_close interface to keep the data_fd updated
when the descriptor is closed.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c              | 23 +++++++++++++++++++----
 tools/perf/util/dso.h              |  3 +++
 tools/perf/util/unwind-libunwind.c |  4 ++--
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 1c3cdaf..5acb4b8 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -159,6 +159,14 @@ static int open_dso(struct dso *dso, struct machine *machine)
 	return fd;
 }
 
+void dso__data_close(struct dso *dso)
+{
+	if (dso->data.fd >= 0) {
+		close(dso->data.fd);
+		dso->data.fd = -1;
+	}
+}
+
 int dso__data_fd(struct dso *dso, struct machine *machine)
 {
 	enum dso_binary_type binary_type_data[] = {
@@ -168,8 +176,13 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
 	};
 	int i = 0;
 
-	if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND)
-		return open_dso(dso, machine);
+	if (dso->data.fd >= 0)
+		return dso->data.fd;
+
+	if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND) {
+		dso->data.fd = open_dso(dso, machine);
+		return dso->data.fd;
+	}
 
 	do {
 		int fd;
@@ -178,7 +191,7 @@ int dso__data_fd(struct dso *dso, struct machine *machine)
 
 		fd = open_dso(dso, machine);
 		if (fd >= 0)
-			return fd;
+			return dso->data.fd = fd;
 
 	} while (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND);
 
@@ -301,7 +314,7 @@ dso_cache__read(struct dso *dso, struct machine *machine,
 	if (ret <= 0)
 		free(cache);
 
-	close(fd);
+	dso__data_close(dso);
 	return ret;
 }
 
@@ -474,6 +487,7 @@ struct dso *dso__new(const char *name)
 		for (i = 0; i < MAP__NR_TYPES; ++i)
 			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
 		dso->data.cache = RB_ROOT;
+		dso->data.fd = -1;
 		dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
 		dso->binary_type = DSO_BINARY_TYPE__NOT_FOUND;
 		dso->loaded = 0;
@@ -506,6 +520,7 @@ void dso__delete(struct dso *dso)
 		dso->long_name_allocated = false;
 	}
 
+	dso__data_close(dso);
 	dso_cache__free(&dso->data.cache);
 	dso__free_a2l(dso);
 	zfree(&dso->symsrc_filename);
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 7637fdd..e48dcf5 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -102,6 +102,7 @@ struct dso {
 	/* dso data file */
 	struct {
 		struct rb_root	 cache;
+		int		 fd;
 	} data;
 
 	char		 name[0];
@@ -147,6 +148,8 @@ int dso__read_binary_type_filename(const struct dso *dso, enum dso_binary_type t
 				   char *root_dir, char *filename, size_t size);
 
 int dso__data_fd(struct dso *dso, struct machine *machine);
+void dso__data_close(struct dso *dso);
+
 ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
 			      u64 offset, u8 *data, ssize_t size);
 ssize_t dso__data_read_addr(struct dso *dso, struct map *map,
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index bd5768d..4f8dd9e 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -250,7 +250,7 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
 
 	/* Check the .eh_frame section for unwinding info */
 	offset = elf_section_offset(fd, ".eh_frame_hdr");
-	close(fd);
+	dso__data_close(dso);
 
 	if (offset)
 		ret = unwind_spec_ehframe(dso, machine, offset,
@@ -271,7 +271,7 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
 
 	/* Check the .debug_frame section for unwinding info */
 	*offset = elf_section_offset(fd, ".debug_frame");
-	close(fd);
+	dso__data_close(dso);
 
 	if (*offset)
 		return 0;
-- 
1.8.3.1

[PATCH 04/14] perf tools: Add global list of opened dso objects

[Jiri Olsa]

Adding global list of opened dso objects, so we can
track them and use the list for caching dso data file
descriptors.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 41 +++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/dso.h |  1 +
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5acb4b8..5d7c7bc 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -136,7 +136,22 @@ int dso__read_binary_type_filename(const struct dso *dso,
 	return ret;
 }
 
-static int open_dso(struct dso *dso, struct machine *machine)
+/*
+ * Global list of open DSOs.
+ */
+static LIST_HEAD(dso__data_open);
+
+static void dso__list_add(struct dso *dso)
+{
+	list_add_tail(&dso->data.open_entry, &dso__data_open);
+}
+
+static void dso__list_del(struct dso *dso)
+{
+	list_del(&dso->data.open_entry);
+}
+
+static int __open_dso(struct dso *dso, struct machine *machine)
 {
 	int fd;
 	char *root_dir = (char *)"";
@@ -159,14 +174,35 @@ static int open_dso(struct dso *dso, struct machine *machine)
 	return fd;
 }
 
-void dso__data_close(struct dso *dso)
+static int open_dso(struct dso *dso, struct machine *machine)
+{
+	int fd = __open_dso(dso, machine);
+
+	if (fd > 0)
+		dso__list_add(dso);
+
+	return fd;
+}
+
+static void close_data_fd(struct dso *dso)
 {
 	if (dso->data.fd >= 0) {
 		close(dso->data.fd);
 		dso->data.fd = -1;
+		dso__list_del(dso);
 	}
 }
 
+static void close_dso(struct dso *dso)
+{
+	close_data_fd(dso);
+}
+
+void dso__data_close(struct dso *dso)
+{
+	close_dso(dso);
+}
+
 int dso__data_fd(struct dso *dso, struct machine *machine)
 {
 	enum dso_binary_type binary_type_data[] = {
@@ -499,6 +535,7 @@ struct dso *dso__new(const char *name)
 		dso->kernel = DSO_TYPE_USER;
 		dso->needs_swap = DSO_SWAP__UNSET;
 		INIT_LIST_HEAD(&dso->node);
+		INIT_LIST_HEAD(&dso->data.open_entry);
 	}
 
 	return dso;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index e48dcf5..90988bf 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -103,6 +103,7 @@ struct dso {
 	struct {
 		struct rb_root	 cache;
 		int		 fd;
+		struct list_head open_entry;
 	} data;
 
 	char		 name[0];
-- 
1.8.3.1

[PATCH 05/14] perf tools: Add global count of opened dso objects

[Jiri Olsa]

Adding global count of opened dso objects so we could
properly limit the number of opened dso data file
descriptors.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5d7c7bc..76e5c13 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1,3 +1,4 @@
+#include <asm/bug.h>
 #include "symbol.h"
 #include "dso.h"
 #include "machine.h"
@@ -137,18 +138,23 @@ int dso__read_binary_type_filename(const struct dso *dso,
 }
 
 /*
- * Global list of open DSOs.
+ * Global list of open DSOs and the counter.
  */
 static LIST_HEAD(dso__data_open);
+static long dso__data_open_cnt;
 
 static void dso__list_add(struct dso *dso)
 {
 	list_add_tail(&dso->data.open_entry, &dso__data_open);
+	dso__data_open_cnt++;
 }
 
 static void dso__list_del(struct dso *dso)
 {
 	list_del(&dso->data.open_entry);
+	WARN_ONCE(dso__data_open_cnt <= 0,
+		  "DSO data fd counter out of bounds.");
+	dso__data_open_cnt--;
 }
 
 static int __open_dso(struct dso *dso, struct machine *machine)
-- 
1.8.3.1

[PATCH 06/14] perf tools: Cache dso data file descriptor

[Jiri Olsa]

Changing the dso__data_close to keep the dso data file
descriptor open if possible.

We keep dsos data file descriptors open until their
count reaches the half of the current fd open limit
(RLIMIT_NOFILE). In this case we close file descriptor
of the first opened dso object.

We've got speed up in dso__data_fd function.

Output from report over 10 GB data with DWARF unwind stacks:
(TODO fix perf diff)

  current code:
   0.16%     perf  perf                       [.] dso__data_fd
  change:
   no record of dso__data_fd

And we got some noticable overall speed up:
(perf perf stat -e '{cycles,instructions}:u' ...)

  current code:
   291,315,329,878      cycles                     ( +-  0.22% )
   391,763,485,304      instructions               ( +-  0.03% )

     180.742249687 seconds time elapsed            ( +-  0.64% )

  change:
   228,948,088,958      cycles                     ( +-  0.20% )
   324,936,504,336      instructions               ( +-  0.02% )

     133.789677792 seconds time elapsed            ( +-  0.76% )

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 76e5c13..316e087 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1,4 +1,6 @@
 #include <asm/bug.h>
+#include <sys/time.h>
+#include <sys/resource.h>
 #include "symbol.h"
 #include "dso.h"
 #include "machine.h"
@@ -204,11 +206,60 @@ static void close_dso(struct dso *dso)
 	close_data_fd(dso);
 }
 
-void dso__data_close(struct dso *dso)
+static void close_first_dso(void)
 {
+	struct dso *dso;
+
+	dso = list_first_entry(&dso__data_open, struct dso, data.open_entry);
 	close_dso(dso);
 }
 
+static rlim_t get_fd_limit(void)
+{
+	struct rlimit l;
+	rlim_t limit = 0;
+
+	/* Allow half of the current open fd limit. */
+	if (getrlimit(RLIMIT_NOFILE, &l) == 0) {
+		if (l.rlim_cur == RLIM_INFINITY)
+			limit = l.rlim_cur;
+		else
+			limit = l.rlim_cur / 2;
+	} else {
+		pr_err("failed to get fd limit\n");
+		limit = 1;
+	}
+
+	return limit;
+}
+
+static bool may_cache_fd(void)
+{
+	static rlim_t limit;
+
+	if (!limit)
+		limit = get_fd_limit();
+
+	if (limit == RLIM_INFINITY)
+		return true;
+
+	return limit > (rlim_t) dso__data_open_cnt;
+}
+
+static void data_close(void)
+{
+	bool cache_fd = may_cache_fd();
+
+	if (!cache_fd)
+		close_first_dso();
+}
+
+void dso__data_close(struct dso *dso)
+{
+	if (dso->data.fd >= 0)
+		data_close();
+}
+
 int dso__data_fd(struct dso *dso, struct machine *machine)
 {
 	enum dso_binary_type binary_type_data[] = {
@@ -318,6 +369,7 @@ static ssize_t
 dso_cache__read(struct dso *dso, struct machine *machine,
 		 u64 offset, u8 *data, ssize_t size)
 {
+	bool new_fd = dso->data.fd == -1;
 	struct dso_cache *cache;
 	ssize_t ret;
 	int fd;
@@ -356,7 +408,10 @@ dso_cache__read(struct dso *dso, struct machine *machine,
 	if (ret <= 0)
 		free(cache);
 
-	dso__data_close(dso);
+	/* Check the cache only if we just added new fd. */
+	if (new_fd)
+		data_close();
+
 	return ret;
 }
 
@@ -563,7 +618,8 @@ void dso__delete(struct dso *dso)
 		dso->long_name_allocated = false;
 	}
 
-	dso__data_close(dso);
+	if (dso->data.fd != -1)
+		close_dso(dso);
 	dso_cache__free(&dso->data.cache);
 	dso__free_a2l(dso);
 	zfree(&dso->symsrc_filename);
-- 
1.8.3.1

[PATCH 07/14] perf tools: Add file size check and factor dso__data_read_offset

[Jiri Olsa]

Adding file size check, because the lseek will succeed
for any offset behind file size. Factoring the code
the check the offset earli in the flow.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
 tools/perf/util/dso.h |  1 +
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 316e087..ad0e2da 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -197,6 +197,7 @@ static void close_data_fd(struct dso *dso)
 	if (dso->data.fd >= 0) {
 		close(dso->data.fd);
 		dso->data.fd = -1;
+		dso->data.file_size = 0;
 		dso__list_del(dso);
 	}
 }
@@ -366,17 +367,10 @@ dso_cache__memcpy(struct dso_cache *cache, u64 offset,
 }
 
 static ssize_t
-dso_cache__read(struct dso *dso, struct machine *machine,
-		 u64 offset, u8 *data, ssize_t size)
+dso_cache__read(struct dso *dso, u64 offset, u8 *data, ssize_t size)
 {
-	bool new_fd = dso->data.fd == -1;
 	struct dso_cache *cache;
 	ssize_t ret;
-	int fd;
-
-	fd = dso__data_fd(dso, machine);
-	if (fd < 0)
-		return -1;
 
 	do {
 		u64 cache_offset;
@@ -390,10 +384,10 @@ dso_cache__read(struct dso *dso, struct machine *machine,
 		cache_offset = offset & DSO__DATA_CACHE_MASK;
 		ret = -EINVAL;
 
-		if (-1 == lseek(fd, cache_offset, SEEK_SET))
+		if (-1 == lseek(dso->data.fd, cache_offset, SEEK_SET))
 			break;
 
-		ret = read(fd, cache->data, DSO__DATA_CACHE_SIZE);
+		ret = read(dso->data.fd, cache->data, DSO__DATA_CACHE_SIZE);
 		if (ret <= 0)
 			break;
 
@@ -408,15 +402,11 @@ dso_cache__read(struct dso *dso, struct machine *machine,
 	if (ret <= 0)
 		free(cache);
 
-	/* Check the cache only if we just added new fd. */
-	if (new_fd)
-		data_close();
-
 	return ret;
 }
 
-static ssize_t dso_cache_read(struct dso *dso, struct machine *machine,
-			      u64 offset, u8 *data, ssize_t size)
+static ssize_t dso_cache_read(struct dso *dso, u64 offset,
+			      u8 *data, ssize_t size)
 {
 	struct dso_cache *cache;
 
@@ -424,11 +414,10 @@ static ssize_t dso_cache_read(struct dso *dso, struct machine *machine,
 	if (cache)
 		return dso_cache__memcpy(cache, offset, data, size);
 	else
-		return dso_cache__read(dso, machine, offset, data, size);
+		return dso_cache__read(dso, offset, data, size);
 }
 
-ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
-			      u64 offset, u8 *data, ssize_t size)
+static ssize_t cached_read(struct dso *dso, u64 offset, u8 *data, ssize_t size)
 {
 	ssize_t r = 0;
 	u8 *p = data;
@@ -436,7 +425,7 @@ ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
 	do {
 		ssize_t ret;
 
-		ret = dso_cache_read(dso, machine, offset, p, size);
+		ret = dso_cache_read(dso, offset, p, size);
 		if (ret < 0)
 			return ret;
 
@@ -456,6 +445,55 @@ ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
 	return r;
 }
 
+static int data_file_size(struct dso *dso)
+{
+	struct stat st;
+
+	if (!dso->data.file_size) {
+		if (fstat(dso->data.fd, &st)) {
+			pr_err("dso mmap failed, fstat: %s\n", strerror(errno));
+			return -1;
+		}
+		dso->data.file_size = st.st_size;
+	}
+
+	return 0;
+}
+
+static ssize_t data_read_offset(struct dso *dso, u64 offset,
+				u8 *data, ssize_t size)
+{
+	if (data_file_size(dso))
+		return -1;
+
+	/* Check the offset sanity. */
+	if (offset > dso->data.file_size)
+		return -1;
+
+	if (offset + size < offset)
+		return -1;
+
+	return cached_read(dso, offset, data, size);
+}
+
+ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
+			      u64 offset, u8 *data, ssize_t size)
+{
+	bool new_fd = dso->data.fd == -1;
+	ssize_t ret;
+
+	if (dso__data_fd(dso, machine) < 0)
+		return -1;
+
+	ret = data_read_offset(dso, offset, data, size);
+
+	/* Check the cache only if we just added new fd. */
+	if (new_fd)
+		data_close();
+
+	return ret;
+}
+
 ssize_t dso__data_read_addr(struct dso *dso, struct map *map,
 			    struct machine *machine, u64 addr,
 			    u8 *data, ssize_t size)
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 90988bf..da47b13 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -103,6 +103,7 @@ struct dso {
 	struct {
 		struct rb_root	 cache;
 		int		 fd;
+		size_t		 file_size;
 		struct list_head open_entry;
 	} data;
 
-- 
1.8.3.1

[PATCH 08/14] perf tools: Allow to close dso fd in case of open failure

[Jiri Olsa]

Adding do_open function that tries to close opened
dso objects in case we fail to open the dso due to
to crossing the allowed RLIMIT_NOFILE limit.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index ad0e2da..7404f67 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -159,6 +159,27 @@ static void dso__list_del(struct dso *dso)
 	dso__data_open_cnt--;
 }
 
+static void close_first_dso(void);
+
+static int do_open(char *name)
+{
+	int fd;
+
+	do {
+		fd = open(name, O_RDONLY);
+		if (fd >= 0)
+			return fd;
+
+		pr_debug("dso open failed, mmap: %s\n", strerror(errno));
+		if (!dso__data_open_cnt || errno != EMFILE)
+			break;
+
+		close_first_dso();
+	} while (1);
+
+	return -1;
+}
+
 static int __open_dso(struct dso *dso, struct machine *machine)
 {
 	int fd;
@@ -177,7 +198,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		return -EINVAL;
 	}
 
-	fd = open(name, O_RDONLY);
+	fd = do_open(name);
 	free(name);
 	return fd;
 }
-- 
1.8.3.1

[PATCH 09/14] perf tools: Add dso__data_* interface descriptons

[Jiri Olsa]

Adding descriptions/explanations for dso__data_*
interface functions.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/dso.h | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 7404f67..80b54e7 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -268,6 +268,11 @@ static bool may_cache_fd(void)
 	return limit > (rlim_t) dso__data_open_cnt;
 }
 
+/*
+ * Check and close LRU dso if we crossed allowed limit
+ * for opened dso file descriptors. The limit is half
+ * of the RLIMIT_NOFILE files opened.
+*/
 static void data_close(void)
 {
 	bool cache_fd = may_cache_fd();
@@ -276,12 +281,27 @@ static void data_close(void)
 		close_first_dso();
 }
 
+/**
+ * dso__data_close - close data file
+ * @dso: dso object
+ *
+ * Calls data_close (in case current dso is open) which takes care
+ * about caching dso objects file descriptors.
+ */
 void dso__data_close(struct dso *dso)
 {
 	if (dso->data.fd >= 0)
 		data_close();
 }
 
+/**
+ * dso__data_fd - Get dso's data file descriptor
+ * @dso: dso object
+ * @machine: machine object
+ *
+ * Find dso's file, open it and returns file descriptor,
+ * which needs to be closed later by dso__data_close.
+ */
 int dso__data_fd(struct dso *dso, struct machine *machine)
 {
 	enum dso_binary_type binary_type_data[] = {
@@ -438,6 +458,11 @@ static ssize_t dso_cache_read(struct dso *dso, u64 offset,
 		return dso_cache__read(dso, offset, data, size);
 }
 
+/*
+ * Reads and caches dso data DSO__DATA_CACHE_SIZE size chunks
+ * in the rb_tree. Any read to already cached data is served
+ * by cached data.
+ */
 static ssize_t cached_read(struct dso *dso, u64 offset, u8 *data, ssize_t size)
 {
 	ssize_t r = 0;
@@ -497,6 +522,17 @@ static ssize_t data_read_offset(struct dso *dso, u64 offset,
 	return cached_read(dso, offset, data, size);
 }
 
+/**
+ * dso__data_read_offset - Read data from dso file offset
+ * @dso: dso object
+ * @machine: machine object
+ * @offset: file offset
+ * @data: buffer to store data
+ * @size: size of the @data buffer
+ *
+ * Read data from dso file offset. Opens dso data file
+ * and use cached_read to get the data.
+ */
 ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
 			      u64 offset, u8 *data, ssize_t size)
 {
@@ -515,6 +551,16 @@ ssize_t dso__data_read_offset(struct dso *dso, struct machine *machine,
 	return ret;
 }
 
+/**
+ * dso__data_read_addr - Read data from dso address
+ * @dso: dso object
+ * @machine: machine object
+ * @offset: file offset
+ * @data: buffer to store data
+ * @size: size of the @data buffer
+ *
+ * Read data from dso address.
+ */
 ssize_t dso__data_read_addr(struct dso *dso, struct map *map,
 			    struct machine *machine, u64 addr,
 			    u8 *data, ssize_t size)
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index da47b13..d713184 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -149,6 +149,47 @@ char dso__symtab_origin(const struct dso *dso);
 int dso__read_binary_type_filename(const struct dso *dso, enum dso_binary_type type,
 				   char *root_dir, char *filename, size_t size);
 
+/*
+ * The dso__data_* interface provides following functions:
+ *   dso__data_fd
+ *   dso__data_close
+ *   dso__data_read_offset
+ *   dso__data_read_addr
+ *
+ * Please refer to the dso.c object code for each function
+ * and arguments documentation. Following text tries to
+ * explain the dso file descriptor caching.
+ *
+ * The dso__data* interface allows caching of opened file
+ * descriptors to speed up the dso data accesses. The idea
+ * is to leave the file descriptor opened/mmaped ideally
+ * for the whole life of the dso object.
+ *
+ * The current usage of the dso__data_* interface is as follows:
+ *
+ *   int fd = dso__data_fd(dso, machine);
+ *   USE 'fd' SOMEHOW
+ *   dso__data_close(dso);
+ *
+ * When the dso data file is opened/closed it's added/removed
+ * to/from the global list dso__data_open. The caching itself
+ * is done by dso__data_close(dso) and works as explained
+ * in pseudo code below:
+ *
+ * if dso is open:
+ *   if (number of opened dsos) > RLIMIT_NOFILE/2:
+ *      close/unmap first dso on dso__data_open list
+ *
+ * Both *read* functions opens dso data file and call
+ * dso__data_close(dso) before return:
+ *
+ *   n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
+ *   n = dso__data_read_addr(dso_0, &machine, 0, buf, BUFSIZE);
+ *
+ * The dso__delete function calls close_dso function to ensure the
+ * data file descriptor gets closed/unmapped before the dso object
+ * is freed.
+*/
 int dso__data_fd(struct dso *dso, struct machine *machine);
 void dso__data_close(struct dso *dso);
 
-- 
1.8.3.1

[PATCH 10/14] perf tests: Spawn child for each test

[Jiri Olsa]

In upcoming tests we will setup process limits, which
might affect other tests. Spawning child for each test
to prevent this.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/builtin-test.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 0d5afaf..fe46f3e 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -3,6 +3,8 @@
  *
  * Builtin regression testing command: ever growing number of sanity tests
  */
+#include <unistd.h>
+#include <string.h>
 #include "builtin.h"
 #include "intlist.h"
 #include "tests.h"
@@ -164,6 +166,34 @@ static bool perf_test__matches(int curr, int argc, const char *argv[])
 	return false;
 }
 
+static int run_test(struct test *test)
+{
+	int status, err = -1, child = fork();
+
+	if (child < 0) {
+		pr_err("failed to fork test: %s\n", strerror(errno));
+		return -1;
+	}
+
+	if (!child) {
+		pr_debug("test child forked, pid %d\n", getpid());
+		err = test->func();
+		exit(err);
+	}
+
+	wait(&status);
+
+	if (WIFEXITED(status)) {
+		err = WEXITSTATUS(status);
+		pr_debug("test child finished with %d\n", err);
+	} else if (WIFSIGNALED(status)) {
+		err = -1;
+		pr_debug("test child interrupted\n");
+	}
+
+	return err;
+}
+
 static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 {
 	int i = 0;
@@ -192,7 +222,7 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 		}
 
 		pr_debug("\n--- start ---\n");
-		err = tests[curr].func();
+		err = run_test(&tests[curr]);
 		pr_debug("---- end ----\n%s:", tests[curr].desc);
 
 		switch (err) {
-- 
1.8.3.1

[PATCH 11/14] perf tests: Allow reuse of test_file function

[Jiri Olsa]

Making the test_file function to be reusable for
new tests coming in following patches.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/dso-data.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 3e6cb17..f5c8743 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -12,11 +12,15 @@
 
 static char *test_file(int size)
 {
-	static char buf_templ[] = "/tmp/test-XXXXXX";
+#define TEMPL "/tmp/test-XXXXXX"
+	static char buf_templ[sizeof(TEMPL)];
 	char *templ = buf_templ;
 	int fd, i;
 	unsigned char *buf;
 
+	strcpy(buf_templ, TEMPL);
+#undef TEMPL
+
 	fd = mkstemp(templ);
 	if (fd < 0) {
 		perror("mkstemp failed");
-- 
1.8.3.1

[PATCH 12/14] perf tests: Add test interface for dso data fd limit

[Jiri Olsa]

Adding a way to setup test dso limit by global variable
test_dso_data__fd_limit. It'll be used in the dso data
cache tests.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dso.c | 5 +++++
 tools/perf/util/dso.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 80b54e7..9310369 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -7,6 +7,8 @@
 #include "util.h"
 #include "debug.h"
 
+rlim_t test_dso_data__fd_limit;
+
 char dso__symtab_origin(const struct dso *dso)
 {
 	static const char origin[] = {
@@ -262,6 +264,9 @@ static bool may_cache_fd(void)
 	if (!limit)
 		limit = get_fd_limit();
 
+	if (unlikely(test_dso_data__fd_limit))
+		limit = test_dso_data__fd_limit;
+
 	if (limit == RLIM_INFINITY)
 		return true;
 
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index d713184..3807014 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -1,6 +1,7 @@
 #ifndef __PERF_DSO
 #define __PERF_DSO
 
+#include <sys/resource.h>
 #include <linux/types.h>
 #include <linux/rbtree.h>
 #include <stdbool.h>
@@ -8,6 +9,8 @@
 #include "map.h"
 #include "build-id.h"
 
+extern rlim_t test_dso_data__fd_limit;
+
 enum dso_binary_type {
 	DSO_BINARY_TYPE__KALLSYMS = 0,
 	DSO_BINARY_TYPE__GUEST_KALLSYMS,
-- 
1.8.3.1

[PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Jiri Olsa]

Adding test that setup test_dso_data__fd_limit and test
dso data file descriptors are cached appropriately.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/builtin-test.c |   6 +-
 tools/perf/tests/dso-data.c     | 180 +++++++++++++++++++++++++++++++++++++++-
 tools/perf/tests/tests.h        |   1 +
 3 files changed, 183 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index fe46f3e..c4d581a 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -52,10 +52,14 @@ static struct test {
 		.func = test__pmu,
 	},
 	{
-		.desc = "Test dso data interface",
+		.desc = "Test dso data read",
 		.func = test__dso_data,
 	},
 	{
+		.desc = "Test dso data cache",
+		.func = test__dso_data_cache,
+	},
+	{
 		.desc = "roundtrip evsel->name check",
 		.func = test__perf_evsel__roundtrip_name_test,
 	},
diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index f5c8743..84ab939 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -1,11 +1,12 @@
-#include "util.h"
-
 #include <stdlib.h>
 #include <linux/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <string.h>
-
+#include <sys/time.h>
+#include <sys/resource.h>
+#include <api/fs/fs.h>
+#include "util.h"
 #include "machine.h"
 #include "symbol.h"
 #include "tests.h"
@@ -154,3 +155,176 @@ int test__dso_data(void)
 	unlink(file);
 	return 0;
 }
+
+static long open_files_cnt(void)
+{
+	char path[PATH_MAX];
+	struct dirent *dent;
+	DIR *dir;
+	long nr = 0;
+	int n;
+
+	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
+	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);
+
+	pr_debug("fd path: %s\n", path);
+
+	dir = opendir(path);
+	TEST_ASSERT_VAL("failed to open fd directory", dir);
+
+	while ((dent = readdir(dir)) != NULL) {
+		if (!strcmp(dent->d_name, ".") ||
+		    !strcmp(dent->d_name, ".."))
+			continue;
+
+		nr++;
+	}
+
+	closedir(dir);
+	return nr - 1;
+}
+
+static struct dso *dsos[3];
+#define dso_0 (dsos[0])
+#define dso_1 (dsos[1])
+#define dso_2 (dsos[2])
+
+static int dsos__create(int size)
+{
+	int i;
+
+	for (i = 0; i < 3; i++) {
+		char *file;
+
+		file = test_file(size);
+		TEST_ASSERT_VAL("failed to get dso file", file);
+
+		dsos[i] = dso__new((const char *)file);
+		TEST_ASSERT_VAL("failed to get dso", dsos[i]);
+	}
+
+	return 0;
+}
+
+static void dsos__delete(void)
+{
+	int i;
+
+	for (i = 0; i < 3; i++) {
+		struct dso *dso = dsos[i];
+		unlink(dso->name);
+		dso__delete(dso);
+	}
+}
+
+static int set_fd_limit(int n)
+{
+	struct rlimit rlim;
+
+	if (getrlimit(RLIMIT_NOFILE, &rlim))
+		return -1;
+
+	pr_debug("file limit %ld, new %d\n", (long) rlim.rlim_cur, n);
+
+	rlim.rlim_cur = n;
+	return setrlimit(RLIMIT_NOFILE, &rlim);
+}
+
+int test__dso_data_cache(void)
+{
+	struct machine machine;
+	long nr = open_files_cnt();
+#define BUFSIZE 10
+	u8 buf[BUFSIZE];
+	ssize_t n;
+	int fd;
+
+	memset(&machine, 0, sizeof(machine));
+
+	/* Make sure we are able to open 3 fds anyway */
+	TEST_ASSERT_VAL("failed to set file limit",
+			!set_fd_limit((nr + 6)));
+
+	/*
+	 * Test scenario:
+	 * - create 3 dso objects
+	 * - set the limit of opened data file descriptors to 2
+	 * - open/close dsos data fds and check for proper
+	 *   handling of the dso data cache
+	 */
+
+	test_dso_data__fd_limit = 3;
+
+	TEST_ASSERT_VAL("failed to create dsos\n", !dsos__create(TEST_FILE_SIZE));
+
+	/* open dso_0 */
+	fd = dso__data_fd(dso_0, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
+	TEST_ASSERT_VAL("failed to read dso", n == BUFSIZE);
+
+	/*
+	 * Close dso_0 data with cache = true,
+	 * dso_0 should remain open.
+	 */
+	dso__data_close(dso_0);
+	TEST_ASSERT_VAL("failed to not close dso", dso_0->data.fd != -1);
+
+	/* open dso_1 */
+	n = dso__data_read_offset(dso_1, &machine, 0, buf, BUFSIZE);
+	TEST_ASSERT_VAL("failed to read dso", n == BUFSIZE);
+
+	/*
+	 * Close dso_1 data with cache = true,
+	 * dso_0 and dso_1 should remain open.
+	 */
+	dso__data_close(dso_1);
+	TEST_ASSERT_VAL("failed to not close dso", dso_0->data.fd != -1);
+	TEST_ASSERT_VAL("failed to not close dso", dso_1->data.fd != -1);
+
+	/* open dso_2 */
+	fd = dso__data_fd(dso_2, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	/*
+	 * Close dso_1 data with cache = true,
+	 * dso_0 should get closed now
+	 */
+	dso__data_close(dso_2);
+	TEST_ASSERT_VAL("failed to close dso_0", dso_0->data.fd == -1);
+
+	/* reopen dso_0 */
+	fd = dso__data_fd(dso_0, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	/*
+	 * Close dso_0 data with cache = true,
+	 * dso_1 should get closed now.
+	 */
+	dso__data_close(dso_0);
+	TEST_ASSERT_VAL("failed to close dso_1", dso_1->data.fd == -1);
+
+	/* reopen dso_1 */
+	n = dso__data_read_offset(dso_1, &machine, 0, buf, BUFSIZE);
+	TEST_ASSERT_VAL("failed to read dso", n == BUFSIZE);
+
+	/*
+	 * Close dso_1 data with cache = true,
+	 * dso_2 should get closed now.
+	 */
+	dso__data_close(dso_1);
+	TEST_ASSERT_VAL("failed to close dso_2", dso_2->data.fd == -1);
+
+	/* dso_0 remains open */
+	TEST_ASSERT_VAL("failed to keep open dso_0", dso_0->data.fd >= 0);
+
+	/* cleanup everything */
+	dsos__delete();
+
+	pr_debug("nr start %ld, nr stop %ld\n", nr, open_files_cnt());
+
+	/* Make sure we did not leak any file descriptor. */
+	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
+	return 0;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index a9d7cb0..61e12b6 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -28,6 +28,7 @@ int test__syscall_open_tp_fields(void);
 int test__pmu(void);
 int test__attr(void);
 int test__dso_data(void);
+int test__dso_data_cache(void);
 int test__parse_events(void);
 int test__hists_link(void);
 int test__python_use(void);
-- 
1.8.3.1

[PATCH 14/14] perf tests: Add test for closing dso objects on EMFILE error

[Jiri Olsa]

Testing that perf properly closes opened dso objects
and tries to reopen in case we run out of allowed file
descriptors for dso data.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/builtin-test.c |  4 +++
 tools/perf/tests/dso-data.c     | 70 +++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |  1 +
 3 files changed, 75 insertions(+)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index c4d581a..a489cda 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -60,6 +60,10 @@ static struct test {
 		.func = test__dso_data_cache,
 	},
 	{
+		.desc = "Test dso data reopen",
+		.func = test__dso_data_reopen,
+	},
+	{
 		.desc = "roundtrip evsel->name check",
 		.func = test__perf_evsel__roundtrip_name_test,
 	},
diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 84ab939..ecc8acd 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -328,3 +328,73 @@ int test__dso_data_cache(void)
 	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
 	return 0;
 }
+
+int test__dso_data_reopen(void)
+{
+	struct machine machine;
+	long nr = open_files_cnt();
+#define BUFSIZE 10
+	int fd, fd_extra;
+
+	memset(&machine, 0, sizeof(machine));
+
+	/*
+	 * Test scenario:
+	 * - create 3 dso objects
+	 * - set process file descriptor limit to current
+	 *   files count + 3
+	 * - test that the first dso gets closed when we
+	 *   reach the files count limit
+	 */
+
+	/* Make sure we are able to open 3 fds anyway */
+	TEST_ASSERT_VAL("failed to set file limit",
+			!set_fd_limit((nr + 3)));
+
+	TEST_ASSERT_VAL("failed to create dsos\n", !dsos__create(TEST_FILE_SIZE));
+
+	/* open dso_0 */
+	fd = dso__data_fd(dso_0, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	/* open dso_1 */
+	fd = dso__data_fd(dso_1, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	/*
+	 * open extra file descriptor and we just
+	 * reached the files count limit
+	 */
+	fd_extra = open("/dev/null", O_RDONLY);
+	TEST_ASSERT_VAL("failed to open extra fd", fd_extra > 0);
+
+	/* open dso_2 */
+	fd = dso__data_fd(dso_2, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	/*
+	 * dso_0 should get closed, because we reached
+	 * the file descriptor limit
+	 */
+	TEST_ASSERT_VAL("failed to close dso_0", dso_0->data.fd == -1);
+
+	/* open dso_0 */
+	fd = dso__data_fd(dso_0, &machine);
+	TEST_ASSERT_VAL("failed to get fd", fd > 0);
+
+	/*
+	 * dso_1 should get closed, because we reached
+	 * the file descriptor limit
+	 */
+	TEST_ASSERT_VAL("failed to close dso_0", dso_1->data.fd == -1);
+
+	/* cleanup everything */
+	close(fd_extra);
+	dsos__delete();
+
+	pr_debug("nr start %ld, nr stop %ld\n", nr, open_files_cnt());
+
+	/* Make sure we did not leak any file descriptor. */
+	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
+	return 0;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 61e12b6..3247ca1 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -29,6 +29,7 @@ int test__pmu(void);
 int test__attr(void);
 int test__dso_data(void);
 int test__dso_data_cache(void);
+int test__dso_data_reopen(void);
 int test__parse_events(void);
 int test__hists_link(void);
 int test__python_use(void);
-- 
1.8.3.1

Re: [PATCHv2 00/14] perf tools: Speedup DWARF unwind

[Jiri Olsa]

ping ;-)

thanks,
jirka

On Thu, May 15, 2014 at 07:23:21PM +0200, Jiri Olsa wrote:
> hi,
> trying to speedup DWARF unwind report code by factoring
> related code:
>   - caching sample's registers access
>   - keep dso data file descriptor open for the
>     life of the dso object
>   - replace dso cache code by mapping dso data file
>     directly for the life of the dso object
> 
> The speedup is mainly for libunwind unwind. The libdw will benefit
> mainly from cached registers access, because it handles dso data
> accesses by itself.. and anyway it's still faster ;-).
> 
> v2 changes:
>   - adding limit for open dso objects with sort of LRU
>     mechanism to pick up and close dso objects if we
>     are over the limit
>   - file mmaping changes are omitted, because I couldn't prove
>     the improvement exactly, will resubmit later
>   - added dso close logic in case of no memory
>   - added tests
> 
> On 10GB perf data file with dwarf unwind data I've got
> around 30% speed up.
> 
> Also reachable in here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/core_unwind_speedup
> 
> thanks,
> jirka
> 
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> Jiri Olsa (14):
>       perf tools: Cache register accesses for unwind processing
>       perf tools: Separate dso data related variables
>       perf tools: Add data_fd into dso object
>       perf tools: Add global list of opened dso objects
>       perf tools: Add global count of opened dso objects
>       perf tools: Cache dso data file descriptor
>       perf tools: Add file size check and factor dso__data_read_offset
>       perf tools: Allow to close dso fd in case of open failure
>       perf tools: Add dso__data_* interface descriptons
>       perf tests: Spawn child for each test
>       perf tests: Allow reuse of test_file function
>       perf tests: Add test interface for dso data fd limit
>       perf tests: Add test for caching dso file descriptors
>       perf tests: Add test for closing dso objects on EMFILE error
> 
>  tools/perf/tests/builtin-test.c    |  42 ++++++++++++++-
>  tools/perf/tests/dso-data.c        | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  tools/perf/tests/tests.h           |   2 +
>  tools/perf/util/dso.c              | 274 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  tools/perf/util/dso.h              |  56 +++++++++++++++++++-
>  tools/perf/util/event.h            |   5 ++
>  tools/perf/util/perf_regs.c        |  10 +++-
>  tools/perf/util/perf_regs.h        |   4 +-
>  tools/perf/util/unwind-libunwind.c |   4 +-
>  9 files changed, 617 insertions(+), 36 deletions(-)

Re: [PATCHv2 00/14] perf tools: Speedup DWARF unwind

[Jean Pihet]

Jiri,


On 23 May 2014 10:13, Jiri Olsa <jolsa@redhat.com> wrote:
> ping ;-)
This looks a ping to yourself ;-)

Here are the results on ARMv7:
- libunwind: between -29% in execution time for light load (i.e. using
not-so-deep backtraces from the stress app.) and -49% for deep
backtrace (the 'stress_bt' app.),
- libdw: no significant improvement (0-2% improvement).

Cf. https://wiki.linaro.org/LEG/Engineering/TOOLS/perf-callstack-unwinding#Speed_improvement
for more details.

FWIW:
Acked-by: Jean Pihet <jean.pihet@linaro.org>

Regards,
Jean


>
> thanks,
> jirka
>
> On Thu, May 15, 2014 at 07:23:21PM +0200, Jiri Olsa wrote:
>> hi,
>> trying to speedup DWARF unwind report code by factoring
>> related code:
>>   - caching sample's registers access
>>   - keep dso data file descriptor open for the
>>     life of the dso object
>>   - replace dso cache code by mapping dso data file
>>     directly for the life of the dso object
>>
>> The speedup is mainly for libunwind unwind. The libdw will benefit
>> mainly from cached registers access, because it handles dso data
>> accesses by itself.. and anyway it's still faster ;-).
>>
>> v2 changes:
>>   - adding limit for open dso objects with sort of LRU
>>     mechanism to pick up and close dso objects if we
>>     are over the limit
>>   - file mmaping changes are omitted, because I couldn't prove
>>     the improvement exactly, will resubmit later
>>   - added dso close logic in case of no memory
>>   - added tests
>>
>> On 10GB perf data file with dwarf unwind data I've got
>> around 30% speed up.
>>
>> Also reachable in here:
>>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>>   perf/core_unwind_speedup
>>
>> thanks,
>> jirka
>>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Jean Pihet <jean.pihet@linaro.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>> Jiri Olsa (14):
>>       perf tools: Cache register accesses for unwind processing
>>       perf tools: Separate dso data related variables
>>       perf tools: Add data_fd into dso object
>>       perf tools: Add global list of opened dso objects
>>       perf tools: Add global count of opened dso objects
>>       perf tools: Cache dso data file descriptor
>>       perf tools: Add file size check and factor dso__data_read_offset
>>       perf tools: Allow to close dso fd in case of open failure
>>       perf tools: Add dso__data_* interface descriptons
>>       perf tests: Spawn child for each test
>>       perf tests: Allow reuse of test_file function
>>       perf tests: Add test interface for dso data fd limit
>>       perf tests: Add test for caching dso file descriptors
>>       perf tests: Add test for closing dso objects on EMFILE error
>>
>>  tools/perf/tests/builtin-test.c    |  42 ++++++++++++++-
>>  tools/perf/tests/dso-data.c        | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  tools/perf/tests/tests.h           |   2 +
>>  tools/perf/util/dso.c              | 274 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>>  tools/perf/util/dso.h              |  56 +++++++++++++++++++-
>>  tools/perf/util/event.h            |   5 ++
>>  tools/perf/util/perf_regs.c        |  10 +++-
>>  tools/perf/util/perf_regs.h        |   4 +-
>>  tools/perf/util/unwind-libunwind.c |   4 +-
>>  9 files changed, 617 insertions(+), 36 deletions(-)

Re: [PATCHv2 00/14] perf tools: Speedup DWARF unwind

[Jiri Olsa]

On Fri, May 23, 2014 at 03:26:27PM +0200, Jean Pihet wrote:
> Jiri,
> 
> 
> On 23 May 2014 10:13, Jiri Olsa <jolsa@redhat.com> wrote:
> > ping ;-)
> This looks a ping to yourself ;-)

:-)

> 
> Here are the results on ARMv7:
> - libunwind: between -29% in execution time for light load (i.e. using
> not-so-deep backtraces from the stress app.) and -49% for deep
> backtrace (the 'stress_bt' app.),
> - libdw: no significant improvement (0-2% improvement).

nice, thanks for testing!

> 
> Cf. https://wiki.linaro.org/LEG/Engineering/TOOLS/perf-callstack-unwinding#Speed_improvement
> for more details.
> 
> FWIW:
> Acked-by: Jean Pihet <jean.pihet@linaro.org>

thanks,
jirka

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

[Namhyung Kim]

Hi Jiri,

On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:

[SNIP]
> +static void data_close(void)
> +{
> +	bool cache_fd = may_cache_fd();
> +
> +	if (!cache_fd)
> +		close_first_dso();
> +}

Why do you do this at close()?  As long as there's no attempt to open a
new file, we can keep existing fd, no?

> +
> +void dso__data_close(struct dso *dso)
> +{
> +	if (dso->data.fd >= 0)
> +		data_close();
> +}

Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
than the given dso.  And this dso__data_close() is not paired with any
_open() also these close calls make me confusing which one to use. ;-p


Thanks
Namhyung

Re: [PATCH 09/14] perf tools: Add dso__data_* interface descriptons

[Namhyung Kim]

On Thu, 15 May 2014 19:23:30 +0200, Jiri Olsa wrote:

[SNIP]
> +/**
> + * dso__data_read_addr - Read data from dso address
> + * @dso: dso object
> + * @machine: machine object
> + * @offset: file offset

s/offset/addr/

Thanks
Namhyung


> + * @data: buffer to store data
> + * @size: size of the @data buffer
> + *
> + * Read data from dso address.
> + */
>  ssize_t dso__data_read_addr(struct dso *dso, struct map *map,
>  			    struct machine *machine, u64 addr,
>  			    u8 *data, ssize_t size)

Re: [PATCH 10/14] perf tests: Spawn child for each test

[Namhyung Kim]

On Thu, 15 May 2014 19:23:31 +0200, Jiri Olsa wrote:
> In upcoming tests we will setup process limits, which
> might affect other tests. Spawning child for each test
> to prevent this.

But you can restore original limits after the test using soft limits?
But I think it's better to run the tests in a child anyway. :)

Thanks,
Namhyung

Re: [PATCH 12/14] perf tests: Add test interface for dso data fd limit

[Namhyung Kim]

On Thu, 15 May 2014 19:23:33 +0200, Jiri Olsa wrote:
> Adding a way to setup test dso limit by global variable
> test_dso_data__fd_limit. It'll be used in the dso data
> cache tests.

Why is this needed?  Why not justing setting RLIMIT_NOFILE in the test
cases?

Thanks,
Namhyung

>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/dso.c | 5 +++++
>  tools/perf/util/dso.h | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 80b54e7..9310369 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -7,6 +7,8 @@
>  #include "util.h"
>  #include "debug.h"
>  
> +rlim_t test_dso_data__fd_limit;
> +
>  char dso__symtab_origin(const struct dso *dso)
>  {
>  	static const char origin[] = {
> @@ -262,6 +264,9 @@ static bool may_cache_fd(void)
>  	if (!limit)
>  		limit = get_fd_limit();
>  
> +	if (unlikely(test_dso_data__fd_limit))
> +		limit = test_dso_data__fd_limit;
> +
>  	if (limit == RLIM_INFINITY)
>  		return true;
>  
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index d713184..3807014 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -1,6 +1,7 @@
>  #ifndef __PERF_DSO
>  #define __PERF_DSO
>  
> +#include <sys/resource.h>
>  #include <linux/types.h>
>  #include <linux/rbtree.h>
>  #include <stdbool.h>
> @@ -8,6 +9,8 @@
>  #include "map.h"
>  #include "build-id.h"
>  
> +extern rlim_t test_dso_data__fd_limit;
> +
>  enum dso_binary_type {
>  	DSO_BINARY_TYPE__KALLSYMS = 0,
>  	DSO_BINARY_TYPE__GUEST_KALLSYMS,

Re: [PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Namhyung Kim]

On Thu, 15 May 2014 19:23:34 +0200, Jiri Olsa wrote:
> Adding test that setup test_dso_data__fd_limit and test
> dso data file descriptors are cached appropriately.

[SNIP]
> +static long open_files_cnt(void)
> +{
> +	char path[PATH_MAX];
> +	struct dirent *dent;
> +	DIR *dir;
> +	long nr = 0;
> +	int n;
> +
> +	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
> +	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);

Looks like an unnecessary check since the scnprintf() cannot return more
than (or equal to) PATH_MAX.

> +
> +	pr_debug("fd path: %s\n", path);
> +
> +	dir = opendir(path);
> +	TEST_ASSERT_VAL("failed to open fd directory", dir);
> +
> +	while ((dent = readdir(dir)) != NULL) {
> +		if (!strcmp(dent->d_name, ".") ||
> +		    !strcmp(dent->d_name, ".."))
> +			continue;
> +
> +		nr++;
> +	}
> +
> +	closedir(dir);
> +	return nr - 1;
> +}

[SNIP]
> +static int set_fd_limit(int n)
> +{
> +	struct rlimit rlim;
> +
> +	if (getrlimit(RLIMIT_NOFILE, &rlim))
> +		return -1;
> +
> +	pr_debug("file limit %ld, new %d\n", (long) rlim.rlim_cur, n);
> +
> +	rlim.rlim_cur = n;
> +	return setrlimit(RLIMIT_NOFILE, &rlim);
> +}
> +
> +int test__dso_data_cache(void)
> +{
> +	struct machine machine;
> +	long nr = open_files_cnt();
> +#define BUFSIZE 10
> +	u8 buf[BUFSIZE];
> +	ssize_t n;
> +	int fd;
> +
> +	memset(&machine, 0, sizeof(machine));
> +
> +	/* Make sure we are able to open 3 fds anyway */
> +	TEST_ASSERT_VAL("failed to set file limit",
> +			!set_fd_limit((nr + 6)));

3 or 6?

> +
> +	/*
> +	 * Test scenario:
> +	 * - create 3 dso objects
> +	 * - set the limit of opened data file descriptors to 2
> +	 * - open/close dsos data fds and check for proper
> +	 *   handling of the dso data cache
> +	 */
> +
> +	test_dso_data__fd_limit = 3;

2 or 3?

> +
> +	TEST_ASSERT_VAL("failed to create dsos\n", !dsos__create(TEST_FILE_SIZE));
> +
> +	/* open dso_0 */
> +	fd = dso__data_fd(dso_0, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
> +	TEST_ASSERT_VAL("failed to read dso", n == BUFSIZE);
> +
> +	/*
> +	 * Close dso_0 data with cache = true,

What does it mean by 'cache = true'?


> +	 * dso_0 should remain open.
> +	 */
> +	dso__data_close(dso_0);
> +	TEST_ASSERT_VAL("failed to not close dso", dso_0->data.fd != -1);
> +
> +	/* open dso_1 */
> +	n = dso__data_read_offset(dso_1, &machine, 0, buf, BUFSIZE);
> +	TEST_ASSERT_VAL("failed to read dso", n == BUFSIZE);
> +
> +	/*
> +	 * Close dso_1 data with cache = true,
> +	 * dso_0 and dso_1 should remain open.
> +	 */
> +	dso__data_close(dso_1);
> +	TEST_ASSERT_VAL("failed to not close dso", dso_0->data.fd != -1);
> +	TEST_ASSERT_VAL("failed to not close dso", dso_1->data.fd != -1);
> +
> +	/* open dso_2 */
> +	fd = dso__data_fd(dso_2, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	/*
> +	 * Close dso_1 data with cache = true,

You meant dso_2 right? :)

Thanks,
Namhyung


> +	 * dso_0 should get closed now
> +	 */
> +	dso__data_close(dso_2);
> +	TEST_ASSERT_VAL("failed to close dso_0", dso_0->data.fd == -1);
> +
> +	/* reopen dso_0 */
> +	fd = dso__data_fd(dso_0, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	/*
> +	 * Close dso_0 data with cache = true,
> +	 * dso_1 should get closed now.
> +	 */
> +	dso__data_close(dso_0);
> +	TEST_ASSERT_VAL("failed to close dso_1", dso_1->data.fd == -1);
> +
> +	/* reopen dso_1 */
> +	n = dso__data_read_offset(dso_1, &machine, 0, buf, BUFSIZE);
> +	TEST_ASSERT_VAL("failed to read dso", n == BUFSIZE);
> +
> +	/*
> +	 * Close dso_1 data with cache = true,
> +	 * dso_2 should get closed now.
> +	 */
> +	dso__data_close(dso_1);
> +	TEST_ASSERT_VAL("failed to close dso_2", dso_2->data.fd == -1);
> +
> +	/* dso_0 remains open */
> +	TEST_ASSERT_VAL("failed to keep open dso_0", dso_0->data.fd >= 0);
> +
> +	/* cleanup everything */
> +	dsos__delete();
> +
> +	pr_debug("nr start %ld, nr stop %ld\n", nr, open_files_cnt());
> +
> +	/* Make sure we did not leak any file descriptor. */
> +	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
> +	return 0;
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index a9d7cb0..61e12b6 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -28,6 +28,7 @@ int test__syscall_open_tp_fields(void);
>  int test__pmu(void);
>  int test__attr(void);
>  int test__dso_data(void);
> +int test__dso_data_cache(void);
>  int test__parse_events(void);
>  int test__hists_link(void);
>  int test__python_use(void);

Re: [PATCH 14/14] perf tests: Add test for closing dso objects on EMFILE error

[Namhyung Kim]

On Thu, 15 May 2014 19:23:35 +0200, Jiri Olsa wrote:
> Testing that perf properly closes opened dso objects
> and tries to reopen in case we run out of allowed file
> descriptors for dso data.
>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/tests/builtin-test.c |  4 +++
>  tools/perf/tests/dso-data.c     | 70 +++++++++++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |  1 +
>  3 files changed, 75 insertions(+)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index c4d581a..a489cda 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -60,6 +60,10 @@ static struct test {
>  		.func = test__dso_data_cache,
>  	},
>  	{
> +		.desc = "Test dso data reopen",
> +		.func = test__dso_data_reopen,
> +	},
> +	{
>  		.desc = "roundtrip evsel->name check",
>  		.func = test__perf_evsel__roundtrip_name_test,
>  	},
> diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
> index 84ab939..ecc8acd 100644
> --- a/tools/perf/tests/dso-data.c
> +++ b/tools/perf/tests/dso-data.c
> @@ -328,3 +328,73 @@ int test__dso_data_cache(void)
>  	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
>  	return 0;
>  }
> +
> +int test__dso_data_reopen(void)
> +{
> +	struct machine machine;
> +	long nr = open_files_cnt();
> +#define BUFSIZE 10

Looks like a copy-n-paste error.. :)


> +	int fd, fd_extra;
> +
> +	memset(&machine, 0, sizeof(machine));
> +
> +	/*
> +	 * Test scenario:
> +	 * - create 3 dso objects
> +	 * - set process file descriptor limit to current
> +	 *   files count + 3
> +	 * - test that the first dso gets closed when we
> +	 *   reach the files count limit
> +	 */
> +
> +	/* Make sure we are able to open 3 fds anyway */
> +	TEST_ASSERT_VAL("failed to set file limit",
> +			!set_fd_limit((nr + 3)));
> +
> +	TEST_ASSERT_VAL("failed to create dsos\n", !dsos__create(TEST_FILE_SIZE));
> +
> +	/* open dso_0 */
> +	fd = dso__data_fd(dso_0, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	/* open dso_1 */
> +	fd = dso__data_fd(dso_1, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	/*
> +	 * open extra file descriptor and we just
> +	 * reached the files count limit
> +	 */
> +	fd_extra = open("/dev/null", O_RDONLY);
> +	TEST_ASSERT_VAL("failed to open extra fd", fd_extra > 0);
> +
> +	/* open dso_2 */
> +	fd = dso__data_fd(dso_2, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	/*
> +	 * dso_0 should get closed, because we reached
> +	 * the file descriptor limit
> +	 */
> +	TEST_ASSERT_VAL("failed to close dso_0", dso_0->data.fd == -1);
> +
> +	/* open dso_0 */
> +	fd = dso__data_fd(dso_0, &machine);
> +	TEST_ASSERT_VAL("failed to get fd", fd > 0);
> +
> +	/*
> +	 * dso_1 should get closed, because we reached
> +	 * the file descriptor limit
> +	 */
> +	TEST_ASSERT_VAL("failed to close dso_0", dso_1->data.fd == -1);

s/dso_0/dso_1/

Btw, I don't see a big difference between this and previous testcase.
Any chance to merge them into one?

Thanks,
Namhyung

> +
> +	/* cleanup everything */
> +	close(fd_extra);
> +	dsos__delete();
> +
> +	pr_debug("nr start %ld, nr stop %ld\n", nr, open_files_cnt());
> +
> +	/* Make sure we did not leak any file descriptor. */
> +	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
> +	return 0;
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 61e12b6..3247ca1 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -29,6 +29,7 @@ int test__pmu(void);
>  int test__attr(void);
>  int test__dso_data(void);
>  int test__dso_data_cache(void);
> +int test__dso_data_reopen(void);
>  int test__parse_events(void);
>  int test__hists_link(void);
>  int test__python_use(void);

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

[Jiri Olsa]

On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
> 
> [SNIP]
> > +static void data_close(void)
> > +{
> > +	bool cache_fd = may_cache_fd();
> > +
> > +	if (!cache_fd)
> > +		close_first_dso();
> > +}
> 
> Why do you do this at close()?  As long as there's no attempt to open a
> new file, we can keep existing fd, no?

so the way it works now is:

 - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
 - if we try to open dso and it fails, because we are out of
   file descriptors, we close dso objects and try to reopen
   (check do_open function)
 - when we close the dso object we check if number of opened
   dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
   the dso opened, if not we close first dso in the list

util/dso.h tries to describe that

> 
> > +
> > +void dso__data_close(struct dso *dso)
> > +{
> > +	if (dso->data.fd >= 0)
> > +		data_close();
> > +}
> 
> Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
> than the given dso.  And this dso__data_close() is not paired with any
> _open() also these close calls make me confusing which one to use. ;-p

thats due to the caching.. as explained above

About the pairing.. originally the interface was only dso__data_fd
that opened and returned fd, which the caller needed to close.

I added dso__data_close so we could keep track of file descriptors.

I could add dso__data_open I guess, but it is dso__data_fd which is
needed for elf interface anyway.

thanks,
jirka

Re: [PATCH 09/14] perf tools: Add dso__data_* interface descriptons

[Jiri Olsa]

On Tue, May 27, 2014 at 10:06:48AM +0900, Namhyung Kim wrote:
> On Thu, 15 May 2014 19:23:30 +0200, Jiri Olsa wrote:
> 
> [SNIP]
> > +/**
> > + * dso__data_read_addr - Read data from dso address
> > + * @dso: dso object
> > + * @machine: machine object
> > + * @offset: file offset
> 
> s/offset/addr/

oops.. copy/paste stuff ;-\

thanks,
jirka

Re: [PATCH 10/14] perf tests: Spawn child for each test

[Jiri Olsa]

On Tue, May 27, 2014 at 10:08:25AM +0900, Namhyung Kim wrote:
> On Thu, 15 May 2014 19:23:31 +0200, Jiri Olsa wrote:
> > In upcoming tests we will setup process limits, which
> > might affect other tests. Spawning child for each test
> > to prevent this.
> 
> But you can restore original limits after the test using soft limits?
> But I think it's better to run the tests in a child anyway. :)

right.. I also recalled that Ingo recommended this before,
I'll queue this one separately..

thanks,
jirka

> 
> Thanks,
> Namhyung

Re: [PATCH 12/14] perf tests: Add test interface for dso data fd limit

[Jiri Olsa]

On Tue, May 27, 2014 at 10:10:40AM +0900, Namhyung Kim wrote:
> On Thu, 15 May 2014 19:23:33 +0200, Jiri Olsa wrote:
> > Adding a way to setup test dso limit by global variable
> > test_dso_data__fd_limit. It'll be used in the dso data
> > cache tests.
> 
> Why is this needed?  Why not justing setting RLIMIT_NOFILE in the test
> cases?

well, I wanted to avoid to customizing the test to the current
open files count state.. the way with artifical limit is easier
and more clean IMO ;-)

I'll see if I can make the setup without this one

thanks,
jirka

Re: [PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Jiri Olsa]

On Tue, May 27, 2014 at 10:36:44AM +0900, Namhyung Kim wrote:
> On Thu, 15 May 2014 19:23:34 +0200, Jiri Olsa wrote:
> > Adding test that setup test_dso_data__fd_limit and test
> > dso data file descriptors are cached appropriately.
> 
> [SNIP]
> > +static long open_files_cnt(void)
> > +{
> > +	char path[PATH_MAX];
> > +	struct dirent *dent;
> > +	DIR *dir;
> > +	long nr = 0;
> > +	int n;
> > +
> > +	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
> > +	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);
> 
> Looks like an unnecessary check since the scnprintf() cannot return more
> than (or equal to) PATH_MAX.

once it's equal it's bad.. as the man says:
"return value of size or more means that the output was  truncated"

I'll fix the rest you found

thanks,
jirka

Re: [PATCH 14/14] perf tests: Add test for closing dso objects on EMFILE error

[Jiri Olsa]

On Tue, May 27, 2014 at 10:43:57AM +0900, Namhyung Kim wrote:
> On Thu, 15 May 2014 19:23:35 +0200, Jiri Olsa wrote:
> > Testing that perf properly closes opened dso objects
> > and tries to reopen in case we run out of allowed file
> > descriptors for dso data.
> >
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Jean Pihet <jean.pihet@linaro.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/tests/builtin-test.c |  4 +++
> >  tools/perf/tests/dso-data.c     | 70 +++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/tests/tests.h        |  1 +
> >  3 files changed, 75 insertions(+)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index c4d581a..a489cda 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -60,6 +60,10 @@ static struct test {
> >  		.func = test__dso_data_cache,
> >  	},
> >  	{
> > +		.desc = "Test dso data reopen",
> > +		.func = test__dso_data_reopen,
> > +	},
> > +	{
> >  		.desc = "roundtrip evsel->name check",
> >  		.func = test__perf_evsel__roundtrip_name_test,
> >  	},
> > diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
> > index 84ab939..ecc8acd 100644
> > --- a/tools/perf/tests/dso-data.c
> > +++ b/tools/perf/tests/dso-data.c
> > @@ -328,3 +328,73 @@ int test__dso_data_cache(void)
> >  	TEST_ASSERT_VAL("failed leadking files", nr == open_files_cnt());
> >  	return 0;
> >  }
> > +
> > +int test__dso_data_reopen(void)
> > +{
> > +	struct machine machine;
> > +	long nr = open_files_cnt();
> > +#define BUFSIZE 10
> 
> Looks like a copy-n-paste error.. :)

yea.. ;) I wonder why gcc did not scream about that

SNIP

> > +
> > +	/*
> > +	 * dso_1 should get closed, because we reached
> > +	 * the file descriptor limit
> > +	 */
> > +	TEST_ASSERT_VAL("failed to close dso_0", dso_1->data.fd == -1);
> 
> s/dso_0/dso_1/
> 
> Btw, I don't see a big difference between this and previous testcase.
> Any chance to merge them into one?

- the first one tests the caching or closing of file descriptors
  after dso__data_close is called
  
- the second one tests that we actually try to close dso objects
  in case we cannot open new dso

thanks,
jirka

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

[Namhyung Kim]

On Tue, 27 May 2014 09:37:38 +0200, Jiri Olsa wrote:
> On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
>> Hi Jiri,
>> 
>> On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
>> 
>> [SNIP]
>> > +static void data_close(void)
>> > +{
>> > +	bool cache_fd = may_cache_fd();
>> > +
>> > +	if (!cache_fd)
>> > +		close_first_dso();
>> > +}
>> 
>> Why do you do this at close()?  As long as there's no attempt to open a
>> new file, we can keep existing fd, no?
>
> so the way it works now is:
>
>  - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
>  - if we try to open dso and it fails, because we are out of
>    file descriptors, we close dso objects and try to reopen
>    (check do_open function)
>  - when we close the dso object we check if number of opened
>    dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
>    the dso opened, if not we close first dso in the list
>
> util/dso.h tries to describe that

Yes, I know.  But my question is why do this at close()?  Isn't it
sufficient to check the file limit at open() and close previous one if
necessary?

>
>> 
>> > +
>> > +void dso__data_close(struct dso *dso)
>> > +{
>> > +	if (dso->data.fd >= 0)
>> > +		data_close();
>> > +}
>> 
>> Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
>> than the given dso.  And this dso__data_close() is not paired with any
>> _open() also these close calls make me confusing which one to use. ;-p
>
> thats due to the caching.. as explained above
>
> About the pairing.. originally the interface was only dso__data_fd
> that opened and returned fd, which the caller needed to close.
>
> I added dso__data_close so we could keep track of file descriptors.
>
> I could add dso__data_open I guess, but it is dso__data_fd which is
> needed for elf interface anyway.

I'd rather suggest dropping the open/close idiom for this case since
it's confusing.  What about get/put or get_fd/put_fd?

Thanks,
Namhyung

Re: [PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Namhyung Kim]

On Tue, 27 May 2014 09:54:36 +0200, Jiri Olsa wrote:
> On Tue, May 27, 2014 at 10:36:44AM +0900, Namhyung Kim wrote:
>> On Thu, 15 May 2014 19:23:34 +0200, Jiri Olsa wrote:
>> > Adding test that setup test_dso_data__fd_limit and test
>> > dso data file descriptors are cached appropriately.
>> 
>> [SNIP]
>> > +static long open_files_cnt(void)
>> > +{
>> > +	char path[PATH_MAX];
>> > +	struct dirent *dent;
>> > +	DIR *dir;
>> > +	long nr = 0;
>> > +	int n;
>> > +
>> > +	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
>> > +	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);
>> 
>> Looks like an unnecessary check since the scnprintf() cannot return more
>> than (or equal to) PATH_MAX.
>
> once it's equal it's bad.. as the man says:
> "return value of size or more means that the output was  truncated"

Did you see "sn"printf?

Thanks,
Namhyung

Re: [PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Jiri Olsa]

On Thu, May 29, 2014 at 09:06:05AM +0900, Namhyung Kim wrote:
> On Tue, 27 May 2014 09:54:36 +0200, Jiri Olsa wrote:
> > On Tue, May 27, 2014 at 10:36:44AM +0900, Namhyung Kim wrote:
> >> On Thu, 15 May 2014 19:23:34 +0200, Jiri Olsa wrote:
> >> > Adding test that setup test_dso_data__fd_limit and test
> >> > dso data file descriptors are cached appropriately.
> >> 
> >> [SNIP]
> >> > +static long open_files_cnt(void)
> >> > +{
> >> > +	char path[PATH_MAX];
> >> > +	struct dirent *dent;
> >> > +	DIR *dir;
> >> > +	long nr = 0;
> >> > +	int n;
> >> > +
> >> > +	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
> >> > +	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);
> >> 
> >> Looks like an unnecessary check since the scnprintf() cannot return more
> >> than (or equal to) PATH_MAX.
> >
> > once it's equal it's bad.. as the man says:
> > "return value of size or more means that the output was  truncated"
> 
> Did you see "sn"printf?

yes, I just double checked.. jirka

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

[Jiri Olsa]

On Thu, May 29, 2014 at 09:02:36AM +0900, Namhyung Kim wrote:
> On Tue, 27 May 2014 09:37:38 +0200, Jiri Olsa wrote:
> > On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
> >> Hi Jiri,
> >> 
> >> On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
> >> 
> >> [SNIP]
> >> > +static void data_close(void)
> >> > +{
> >> > +	bool cache_fd = may_cache_fd();
> >> > +
> >> > +	if (!cache_fd)
> >> > +		close_first_dso();
> >> > +}
> >> 
> >> Why do you do this at close()?  As long as there's no attempt to open a
> >> new file, we can keep existing fd, no?
> >
> > so the way it works now is:
> >
> >  - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
> >  - if we try to open dso and it fails, because we are out of
> >    file descriptors, we close dso objects and try to reopen
> >    (check do_open function)
> >  - when we close the dso object we check if number of opened
> >    dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
> >    the dso opened, if not we close first dso in the list
> >
> > util/dso.h tries to describe that
> 
> Yes, I know.  But my question is why do this at close()?  Isn't it
> sufficient to check the file limit at open() and close previous one if
> necessary?

hm, it's still the same operation to be done either in open
or in close.. but we would not need dso__data_close then..
ok, I'll make the change ;-)

thanks,
jirka

Re: [PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Namhyung Kim]

Hi Jiri,

On Thu, 29 May 2014 10:37:32 +0200, Jiri Olsa wrote:
> On Thu, May 29, 2014 at 09:06:05AM +0900, Namhyung Kim wrote:
>> On Tue, 27 May 2014 09:54:36 +0200, Jiri Olsa wrote:
>> > On Tue, May 27, 2014 at 10:36:44AM +0900, Namhyung Kim wrote:
>> >> On Thu, 15 May 2014 19:23:34 +0200, Jiri Olsa wrote:
>> >> > Adding test that setup test_dso_data__fd_limit and test
>> >> > dso data file descriptors are cached appropriately.
>> >> 
>> >> [SNIP]
>> >> > +static long open_files_cnt(void)
>> >> > +{
>> >> > +	char path[PATH_MAX];
>> >> > +	struct dirent *dent;
>> >> > +	DIR *dir;
>> >> > +	long nr = 0;
>> >> > +	int n;
>> >> > +
>> >> > +	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
>> >> > +	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);
>> >> 
>> >> Looks like an unnecessary check since the scnprintf() cannot return more
>> >> than (or equal to) PATH_MAX.
>> >
>> > once it's equal it's bad.. as the man says:
>> > "return value of size or more means that the output was  truncated"
>> 
>> Did you see "sn"printf?
>
> yes, I just double checked.. jirka

Please see "scn"printf then. :)  It does something like below..

  i = snprintf(buf, size, ...);
  return (i >= size) ? : size - 1 : i;


Thanks,
Namhyung

Re: [PATCH 13/14] perf tests: Add test for caching dso file descriptors

[Jiri Olsa]

On Fri, May 30, 2014 at 11:11:20AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, 29 May 2014 10:37:32 +0200, Jiri Olsa wrote:
> > On Thu, May 29, 2014 at 09:06:05AM +0900, Namhyung Kim wrote:
> >> On Tue, 27 May 2014 09:54:36 +0200, Jiri Olsa wrote:
> >> > On Tue, May 27, 2014 at 10:36:44AM +0900, Namhyung Kim wrote:
> >> >> On Thu, 15 May 2014 19:23:34 +0200, Jiri Olsa wrote:
> >> >> > Adding test that setup test_dso_data__fd_limit and test
> >> >> > dso data file descriptors are cached appropriately.
> >> >> 
> >> >> [SNIP]
> >> >> > +static long open_files_cnt(void)
> >> >> > +{
> >> >> > +	char path[PATH_MAX];
> >> >> > +	struct dirent *dent;
> >> >> > +	DIR *dir;
> >> >> > +	long nr = 0;
> >> >> > +	int n;
> >> >> > +
> >> >> > +	n = scnprintf(path, PATH_MAX, "%s/self/fd", procfs__mountpoint());
> >> >> > +	TEST_ASSERT_VAL("couldn't get fd path", n < PATH_MAX);
> >> >> 
> >> >> Looks like an unnecessary check since the scnprintf() cannot return more
> >> >> than (or equal to) PATH_MAX.
> >> >
> >> > once it's equal it's bad.. as the man says:
> >> > "return value of size or more means that the output was  truncated"
> >> 
> >> Did you see "sn"printf?
> >
> > yes, I just double checked.. jirka
> 
> Please see "scn"printf then. :)  It does something like below..
> 
>   i = snprintf(buf, size, ...);
>   return (i >= size) ? : size - 1 : i;

aaargh... ok ;-)

thanks,
jirka