perf/powerpc: Cache the DWARF debug info

From 773a3608a0cd2daf02e244cb9ffbf5bb6a0e724e Mon Sep 17 00:00:00 2001

Quoted 1 line

From 773a3608a0cd2daf02e244cb9ffbf5bb6a0e724e Mon Sep 17 00:00:00 2001

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 21 Oct 2014 13:20:22 -0500
Subject: [PATCH 1/1] perf/powerpc: Cache DWARF debug info

Cache the DWARF debug info for DSO so we don't have to rebuild it for each address in the DSO (duh!).

	$ time /tmp/perf.orig report -g > /tmp/report.1

	real	0m1.845s
	user	0m0.963s
	sys	0m0.879s

	$ time /tmp/perf report -g > /tmp/report.2

	real	0m0.089s
	user	0m0.082s
	sys	0m0.006s

	$ diff /tmp/report.1 /tmp/report.2
	$

Reported-by Anton Blanchard <[email protected]>

Signed-off-by Sukadev Bhattiprolu <[email protected]>

---
 tools/perf/arch/powerpc/util/skip-callchain-idx.c | 91 ++++++++++++++++++++---
 1 file changed, 82 insertions(+), 9 deletions(-)

tools/perf/arch/powerpc/util/skip-callchain-idx.c +82 -9

index d73ef8b..bfe254d 100644
@@ -38,6 +38,63 @@ static const Dwfl_Callbacks offline_callbacks = {
 	.section_address = dwfl_offline_section_address,
 };
 
+struct list_head perf_dwfl_files;
+
+struct perf_dwfl_node {
+	struct list_head node;
+	char *exec_file;
+	Dwfl *dwfl;
+};
+
+static void perf_init_dwfl(void)
+{
+	static int dwfl_inited;
+
+	if (!dwfl_inited) {
+		dwfl_inited = 1;
+		INIT_LIST_HEAD(&perf_dwfl_files);
+	}
+}
+
+static Dwfl *perf_dwfl_find(const char *file_name)
+{
+	struct list_head *pos;
+	struct perf_dwfl_node *node;
+
+	perf_init_dwfl();
+
+	list_for_each(pos, &perf_dwfl_files) {
+		node = list_entry(pos, struct perf_dwfl_node, node);
+		if (!strcmp(node->exec_file, file_name))
+			return node->dwfl;
+	}
+
+	return NULL;
+}
+
+
+/*
+ * Return 1 if were able to cache the DWARF debug info. 0 otherwise.
+ */
+static int perf_dwfl_cache(const char *file_name, Dwfl *dwfl)
+{
+	struct perf_dwfl_node *dwfl_node;
+
+	dwfl_node = malloc(sizeof(struct perf_dwfl_node));
+
+	if (!dwfl_node) {
+		pr_debug("%s(): Unable to alloc memory\n", __func__);
+		return 0;
+	}
+	INIT_LIST_HEAD(&dwfl_node->node);
+	dwfl_node->dwfl = dwfl;
+	dwfl_node->exec_file = strdup(file_name);
+
+	list_add(&dwfl_node->node, &perf_dwfl_files);
+
+	return 1;
+}
+
 
 /*
  * Use the DWARF expression for the Call-frame-address and determine

@@ -155,16 +212,26 @@ static int check_return_addr(const char *exec_file, Dwarf_Addr pc)
 	Dwarf_Addr	start = pc;
 	Dwarf_Addr	end = pc;
 	bool		signalp;
+	int		cached;
 
-	dwfl = dwfl_begin(&offline_callbacks);
+	cached = 1;
+	dwfl = perf_dwfl_find(exec_file);
 	if (!dwfl) {
-		pr_debug("dwfl_begin() failed: %s\n", dwarf_errmsg(-1));
-		return -1;
-	}
-
-	if (dwfl_report_offline(dwfl, "",  exec_file, -1) == NULL) {
-		pr_debug("dwfl_report_offline() failed %s\n", dwarf_errmsg(-1));
-		goto out;
+		dwfl = dwfl_begin(&offline_callbacks);
+		if (!dwfl) {
+			pr_debug("dwfl_begin() failed: %s\n", dwarf_errmsg(-1));
+			return -1;
+		}
+
+		if (dwfl_report_offline(dwfl, "",  exec_file, -1) == NULL) {
+			pr_debug("dwfl_report_offline() failed %s\n",
+							dwarf_errmsg(-1));
+			goto out;
+		}
+		/*
+		 * If we fail to alloc memory, we lose the benefit of caching
+		 */
+		cached = perf_dwfl_cache(exec_file, dwfl);
 	}
 
 	mod = dwfl_addrmodule(dwfl, pc);

@@ -194,7 +261,13 @@ static int check_return_addr(const char *exec_file, Dwarf_Addr pc)
 	rc = check_return_reg(ra_regno, frame);
 
 out:
-	dwfl_end(dwfl);
+	/*
+	 * In the unlikely event that we were unable to cache the debug
+	 * info, release it so we don't leak fds.
+	 */
+	if (!cached)
+		dwfl_end(dwfl);
+
 	return rc;
 }
 

Signature · 1 line

1.8.4.2

Arnaldo Carvalho de Melo [[email protected]] wrote:

| Em Tue, Oct 21, 2014 at 11:56:10AM -0700, Sukadev Bhattiprolu escreveu:
| > >From 773a3608a0cd2daf02e244cb9ffbf5bb6a0e724e Mon Sep 17 00:00:00 2001
| > From: Sukadev Bhattiprolu <[email protected]>
| > Date: Tue, 21 Oct 2014 13:20:22 -0500
| > Subject: [PATCH 1/1] perf/powerpc: Cache DWARF debug info
| 
| Jiri, isn't his related to that dsos caching thing you created?

| 
| Anyway, there is a per machine struct (struct machine) where things like
| this should be put, please do not add globals or static variables in
| functions.

Good point. How about something like this:

tools/perf/arch/powerpc/util/skip-callchain-idx.c +11 -9

@@ -145,7 +145,7 @@ static Dwarf_Frame *get_dwarf_frame(Dwfl_Module *mod, Dwarf_Addr pc)
  *		yet used)
  *	-1 in case of errors
  */
-static int check_return_addr(const char *exec_file, Dwarf_Addr pc)
+static int check_return_addr(struct dso *dso, Dwarf_Addr pc)
 {
 	int		rc = -1;
 	Dwfl		*dwfl;

@@ -156,15 +156,27 @@ static int check_return_addr(const char *exec_file, Dwarf_Addr pc)
 	Dwarf_Addr	end = pc;
 	bool		signalp;
 
-	dwfl = dwfl_begin(&offline_callbacks);
-	if (!dwfl) {
-		pr_debug("dwfl_begin() failed: %s\n", dwarf_errmsg(-1));
-		return -1;
-	}
+	dwfl = dso->dwfl;
 
-	if (dwfl_report_offline(dwfl, "",  exec_file, -1) == NULL) {
-		pr_debug("dwfl_report_offline() failed %s\n", dwarf_errmsg(-1));
-		goto out;
+	if (!dwfl) {
+		dwfl = dwfl_begin(&offline_callbacks);
+		if (!dwfl) {
+			pr_debug("dwfl_begin() failed: %s\n", dwarf_errmsg(-1));
+			return -1;
+		}
+
+		if (dwfl_report_offline(dwfl, "", dso->long_name, -1) == NULL) {
+			pr_debug("dwfl_report_offline() failed %s\n",

Jiri Olsa [[email protected]] wrote:

| > +			goto out;
| > +		}
| > +		dso->dwfl = dwfl;
| 
| so by this we get powerpc arch code sharing dw handle via dso object,
| but we have lot of generic code too ;-)

Well, this applies to powerpc...

| 
| could you make this happen for unwind__get_entries.. probably
| both sharing same generic code I guess

and unwind_get_entries() applies only to x86 and arm right ? ;-) Or at least thats what the config/Makefile says.

I can take a look at unwind_get_entries(), but can you please merge this fix for now, since the current performance is bad?

Em Wed, Oct 22, 2014 at 10:46:59AM -0700, Sukadev Bhattiprolu escreveu:

Quoted 14 lines

Jiri Olsa [[email protected]] wrote:

| > +			goto out;
| > +		}
| > +		dso->dwfl = dwfl;
| 
| so by this we get powerpc arch code sharing dw handle via dso object,
| but we have lot of generic code too ;-)

Well, this applies to powerpc...

| 
| could you make this happen for unwind__get_entries.. probably
| both sharing same generic code I guess

Quoted 5 lines

and unwind_get_entries() applies only to x86 and arm right ? ;-) Or at least thats what the config/Makefile says.

I can take a look at unwind_get_entries(), but can you please merge this fix for now, since the current performance is bad?

Right, I think the way it is now is a good compromise, i.e. you seem to be using the right place to cache this, this is restricted to powerpc, i.e. if leaks or excessive memory usage happens in workloads with lots of DSOs having dwfl handlers open at the same times happens, it doesn't affect users in other arches.

Jiri: do you agree?

Quoted 20 lines

Em Wed, Oct 22, 2014 at 10:46:59AM -0700, Sukadev Bhattiprolu escreveu:

Quoted 10 lines

Jiri Olsa [[email protected]] wrote:

| > +			goto out;
| > +		}
| > +		dso->dwfl = dwfl;

| so by this we get powerpc arch code sharing dw handle via dso object, | but we have lot of generic code too ;-)

Well, this applies to powerpc...

Quoted 8 lines

| could you make this happen for unwind__get_entries.. probably | both sharing same generic code I guess

and unwind_get_entries() applies only to x86 and arm right ? ;-) Or at least thats what the config/Makefile says.

I can take a look at unwind_get_entries(), but can you please merge this fix for now, since the current performance is bad?

Quoted 7 lines

Right, I think the way it is now is a good compromise, i.e. you seem to be using the right place to cache this, this is restricted to powerpc, i.e. if leaks or excessive memory usage happens in workloads with lots of DSOs having dwfl handlers open at the same times happens, it doesn't affect users in other arches.

Jiri: do you agree?

well it's powerpc specific now.. anyway the code in the patch to open the dwfl is generic and should be in in generic place.. like in some extern function that the x86 would call to get the dwfl handle

also the current patch leaks the dso->dwfl, which is never freed -> dwfl_end-ed, dwfl_end should be called of in dso__delete I think

Em Thu, Oct 23, 2014 at 04:12:13PM +0200, Jiri Olsa escreveu:

Quoted 29 lines · Arnaldo Carvalho de Melo

Quoted 20 lines

Em Wed, Oct 22, 2014 at 10:46:59AM -0700, Sukadev Bhattiprolu escreveu:

Quoted 10 lines

Jiri Olsa [[email protected]] wrote:

| > +			goto out;
| > +		}
| > +		dso->dwfl = dwfl;

| so by this we get powerpc arch code sharing dw handle via dso object, | but we have lot of generic code too ;-)

Well, this applies to powerpc...

Quoted 8 lines

| could you make this happen for unwind__get_entries.. probably | both sharing same generic code I guess

and unwind_get_entries() applies only to x86 and arm right ? ;-) Or at least thats what the config/Makefile says.

I can take a look at unwind_get_entries(), but can you please merge this fix for now, since the current performance is bad?

Quoted 7 lines

Right, I think the way it is now is a good compromise, i.e. you seem to be using the right place to cache this, this is restricted to powerpc, i.e. if leaks or excessive memory usage happens in workloads with lots of DSOs having dwfl handlers open at the same times happens, it doesn't affect users in other arches.

Jiri: do you agree?

Quoted 7 lines

well it's powerpc specific now.. anyway the code in the patch to open the dwfl is generic and should be in in generic place.. like in some extern function that the x86 would call to get the dwfl handle

also the current patch leaks the dso->dwfl, which is never freed -> dwfl_end-ed, dwfl_end should be called of in dso__delete I think

Yeah, as my comment implies, I guess those are all valid concerns, i.e. the patch needs more work, I was willing to accept it as-is because it would hurt just Sukadev (i.e. powerpc), as he seems to be in a hurry to get the performance improved :-)

I will remove it from my tree for now, as in the end what I'm doing doesn't touch those specific functions.

But I think this will go on dragging extra work, i.e.: how to limit the number of dwfl handlers used? Should we have just a front end cache like what is done for machine__findnew_thread() (with just the last hit) and perhaps then have a few slots for keeping N dwfl open and when that number is up we check the one with less queries and close it?

Jiri, are you doing that on that cache stuff you did? I mean how do you keep this stuff:

/*
* Global list of open DSOs and the counter.
*/
static LIST_HEAD(dso__data_open);
static long dso__data_open_cnt;

Also this should not be global at all, this should be on struct machine, since a DSO that is present on a machine may have the same name as the dso on another machine (two guests, hosts, etc) and thus should not be kept on the same list, etc.

So reading a bit more you seem to check rlimit, do LRUing when hitting the limit, etc, that is why I thought about that stuff when Sukadev first posted this patch...

Sukadev, all this is in tools/perf/util/dso.c

That is why I thought it would be a compromise to put what he did, it would not make the existing situation that much worse, work needs to be done in this area :-\

Quoted 3 lines

That is why I thought it would be a compromise to put what he did, it would not make the existing situation that much worse, work needs to be done in this area :-\

I think we just need to put that libdw handle into dso object as I suggested above

Em Thu, Oct 23, 2014 at 05:13:06PM +0200, Jiri Olsa escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo

Quoted 3 lines

That is why I thought it would be a compromise to put what he did, it would not make the existing situation that much worse, work needs to be done in this area :-\

I think we just need to put that libdw handle into dso object as I suggested above

Isn't it there already?

The patch does:

+++ b/tools/perf/util/dso.h

(quoted diff) +1 -1

@@ -127,6 +127,7 @@ struct dso {
        const char       *long_name;
        u16              long_name_len;
        u16              short_name_len;
+       void            *dwfl;                  /* DWARF debug info */
 
 
----------

What you want, on top of that, at a minimum, we somehow limit the number of simultaneously dwfl_begin()'ed DSOs, right?