Re: [PATCH 2/4] perf symbols: /proc/kallsyms does not sort module symbols

Re: [PATCH 2/4] perf symbols: /proc/kallsyms does not sort module symbols

[Stephane Eranian]

Hi,

What's the status of this patch?

It is not in 3.1.0-rc7. It is needed!

I ran into serious issues (bogus symbols) with a profile where
kernel modules were stressed and it boiled down to kallsyms__parse()
being broken because /proc/kallsyms is assumed sorted when it actually
is NOT.

Re: [PATCH 2/4] perf symbols: /proc/kallsyms does not sort module symbols

[Arnaldo Carvalho de Melo]

Em Fri, Sep 23, 2011 at 03:59:33PM +0200, Stephane Eranian escreveu:
> Hi,
> 
> What's the status of this patch?
> 
> It is not in 3.1.0-rc7. It is needed!
> 
> I ran into serious issues (bogus symbols) with a profile where
> kernel modules were stressed and it boiled down to kallsyms__parse()
> being broken because /proc/kallsyms is assumed sorted when it actually
> is NOT.

Its all here:

https://github.com/acmel/linux/commits/perf/urgent

I was waiting for Ingo, Peter, can you take a look and see if you agree
with the patches there then I can ask Linus to pull it, ok?

- Arnaldo

Re: [PATCH 2/4] perf symbols: /proc/kallsyms does not sort module symbols

[Peter Zijlstra]

On Fri, 2011-09-23 at 11:11 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 23, 2011 at 03:59:33PM +0200, Stephane Eranian escreveu:
> > Hi,
> > 
> > What's the status of this patch?
> > 
> > It is not in 3.1.0-rc7. It is needed!
> > 
> > I ran into serious issues (bogus symbols) with a profile where
> > kernel modules were stressed and it boiled down to kallsyms__parse()
> > being broken because /proc/kallsyms is assumed sorted when it actually
> > is NOT.
> 
> Its all here:
> 
> https://github.com/acmel/linux/commits/perf/urgent
> 
> I was waiting for Ingo, Peter, can you take a look and see if you agree
> with the patches there then I can ask Linus to pull it, ok?

Looks all-right, ship it! :-)

[PATCH 0/4] perf symbol fixes

[Anton Blanchard]

The following patches fix some bugs in our symbol code and adds
heuristics to select which duplicate symbol to use.

In the process this fixes the issue on ppc where we need to handle both
the old style and new style 64bit ABI.

Anton

[PATCH 2/4] perf symbols: /proc/kallsyms does not sort module symbols

[Anton Blanchard]

[-- Attachment #1: kallsyms_1 --]
[-- Type: text/plain, Size: 2266 bytes --]

kallsyms__parse assumes that /proc/kallsyms is sorted and sets the
end of the previous symbol to the start of the current one.

Unfortunately module symbols are not sorted, eg:

ffffffffa0081f30 t e1000_clean_rx_irq   [e1000e]
ffffffffa00817a0 t e1000_alloc_rx_buffers       [e1000e]

Some symbols end up with a negative length and others have a length
larger than they should. This results in confusing perf output.

We already have a function to fixup the end of zero length symbols
so use that instead.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6-tip/tools/perf/util/symbol.c
===================================================================
--- linux-2.6-tip.orig/tools/perf/util/symbol.c	2011-08-19 12:33:21.501373684 +1000
+++ linux-2.6-tip/tools/perf/util/symbol.c	2011-08-19 12:33:24.711429312 +1000
@@ -440,18 +440,11 @@ int kallsyms__parse(const char *filename
 	char *line = NULL;
 	size_t n;
 	int err = -1;
-	u64 prev_start = 0;
-	char prev_symbol_type = 0;
-	char *prev_symbol_name;
 	FILE *file = fopen(filename, "r");
 
 	if (file == NULL)
 		goto out_failure;
 
-	prev_symbol_name = malloc(KSYM_NAME_LEN);
-	if (prev_symbol_name == NULL)
-		goto out_close;
-
 	err = 0;
 
 	while (!feof(file)) {
@@ -482,24 +475,18 @@ int kallsyms__parse(const char *filename
 			break;
 		}
 
-		if (prev_symbol_type) {
-			u64 end = start;
-			if (end != prev_start)
-				--end;
-			err = process_symbol(arg, prev_symbol_name,
-					     prev_symbol_type, prev_start, end);
-			if (err)
-				break;
-		}
-
-		memcpy(prev_symbol_name, symbol_name, len + 1);
-		prev_symbol_type = symbol_type;
-		prev_start = start;
+		/*
+		 * module symbols are not sorted so we add all
+		 * symbols with zero length and rely on
+		 * symbols__fixup_end() to fix it up.
+		 */
+		err = process_symbol(arg, symbol_name,
+				     symbol_type, start, start);
+		if (err)
+			break;
 	}
 
-	free(prev_symbol_name);
 	free(line);
-out_close:
 	fclose(file);
 	return err;
 
@@ -705,6 +692,8 @@ int dso__load_kallsyms(struct dso *dso,
 	if (dso__load_all_kallsyms(dso, filename, map) < 0)
 		return -1;
 
+	symbols__fixup_end(&dso->symbols[map->type]);
+
 	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)
 		dso->symtab_type = SYMTAB__GUEST_KALLSYMS;
 	else