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