[PATCH] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[PATCH] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Paul Mackerras]

The glib dependency in kerneltop.c is only for a little bit of list
manipulation, and I find it inconvenient.  This adds a 'next' field to
struct source_line, which lets us link them together into a list.  The
code to do the linking ourselves turns out to be no longer or more
difficult than using glib.

This also fixes a few other problems:

- We need to #include <limits.h> to get PATH_MAX on powerpc.

- We need to #include <linux/types.h> rather than have our own
  definitions of __u64 and __s64; on powerpc the installed headers
  define them to be unsigned long and long respectively, and if we
  have our own, different definition here that causes a compile error.

- This takes out the x86 setting of errno from -ret in
  sys_perf_counter_open.  My experiments on x86 indicate that the
  glibc syscall() does this for us already.

- We had two CPU migration counters in the default set, which seems
  unnecessary; I changed one of them to a context switch counter.

- In perfstat mode we were printing CPU cycles and instructions as
  milliseconds, and the cpu clock and task clock counters as events.
  This fixes that.

- In perfstat mode we were still printing a blank line after the first
  counter, which was a holdover from when a task clock counter was
  automatically included as the first counter.  This removes the blank
  line.

- On a test machine here, parse_symbols() and parse_vmlinux() were
  taking long enough (almost 0.5 seconds) for the mmap buffer to
  overflow before we got to the first mmap_read() call, so this moves
  them before we open all the counters.

- The error message if sys_perf_counter_open fails needs to use errno,
  not -fd[i][counter].

Signed-off-by: Paul Mackerras <paulus@samba.org>

---
 Documentation/perf_counter/Makefile    |    2 +-
 Documentation/perf_counter/kerneltop.c |  112 +++++++++++++-------------------
 2 files changed, 46 insertions(+), 68 deletions(-)

diff --git a/Documentation/perf_counter/Makefile b/Documentation/perf_counter/Makefile
index 666da95..194b662 100644
--- a/Documentation/perf_counter/Makefile
+++ b/Documentation/perf_counter/Makefile
@@ -3,7 +3,7 @@ BINS = kerneltop perfstat
 all: $(BINS)
 
 kerneltop: kerneltop.c ../../include/linux/perf_counter.h
-	cc -O6 -Wall -lrt `pkg-config --cflags --libs glib-2.0` -o $@ $<
+	cc -O6 -Wall -lrt -o $@ $<
 
 perfstat: kerneltop
 	ln -sf kerneltop perfstat
diff --git a/Documentation/perf_counter/kerneltop.c b/Documentation/perf_counter/kerneltop.c
index 2ab29b5..ea13e4e 100644
--- a/Documentation/perf_counter/kerneltop.c
+++ b/Documentation/perf_counter/kerneltop.c
@@ -3,7 +3,7 @@
 
    Build with:
 
-     cc -O6 -Wall -lrt `pkg-config --cflags --libs glib-2.0` -o kerneltop kerneltop.c
+     cc -O6 -Wall -c -o kerneltop.o kerneltop.c -lrt
 
    Sample output:
 
@@ -56,6 +56,7 @@
   *   Yanmin Zhang <yanmin.zhang@intel.com>
   *   Wu Fengguang <fengguang.wu@intel.com>
   *   Mike Galbraith <efault@gmx.de>
+  *   Paul Mackerras <paulus@samba.org>
   *
   * Released under the GPL v2. (and only v2, not any later version)
   */
@@ -68,6 +69,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
+#include <limits.h>
 #include <getopt.h>
 #include <assert.h>
 #include <fcntl.h>
@@ -76,8 +78,6 @@
 #include <ctype.h>
 #include <time.h>
 
-#include <glib.h>
-
 #include <sys/syscall.h>
 #include <sys/ioctl.h>
 #include <sys/poll.h>
@@ -87,6 +87,7 @@
 #include <sys/mman.h>
 
 #include <linux/unistd.h>
+#include <linux/types.h>
 
 #include "../../include/linux/perf_counter.h"
 
@@ -114,11 +115,6 @@
 #define __user
 #define asmlinkage
 
-typedef unsigned int            __u32;
-typedef unsigned long long      __u64;
-typedef long long               __s64;
-
-
 #ifdef __x86_64__
 #define __NR_perf_counter_open 295
 #define rmb()		asm volatile("lfence" ::: "memory")
@@ -146,17 +142,8 @@ asmlinkage int sys_perf_counter_open(
         int                             group_fd,
         unsigned long                   flags)
 {
-        int ret;
-
-        ret = syscall(
+        return syscall(
                 __NR_perf_counter_open, hw_event_uptr, pid, cpu, group_fd, flags);
-#if defined(__x86_64__) || defined(__i386__)
-        if (ret < 0 && ret > -4096) {
-                errno = -ret;
-                ret = -1;
-        }
-#endif
-        return ret;
 }
 
 #define MAX_COUNTERS			64
@@ -170,7 +157,7 @@ static int			system_wide			=  0;
 static int			nr_counters			=  0;
 static __u64			event_id[MAX_COUNTERS]		= {
 	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_TASK_CLOCK),
-	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CPU_MIGRATIONS),
+	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CONTEXT_SWITCHES),
 	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CPU_MIGRATIONS),
 	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_PAGE_FAULTS),
 
@@ -202,14 +189,15 @@ static int			delay_secs			=  2;
 static int			zero;
 static int			dump_symtab;
 
-static GList			*lines;
-
 struct source_line {
 	uint64_t		EIP;
 	unsigned long		count;
 	char			*line;
+	struct source_line	*next;
 };
 
+static struct source_line	*lines;
+static struct source_line	**lines_tail;
 
 const unsigned int default_count[] = {
 	1000000,
@@ -519,9 +507,8 @@ int do_perfstat(int argc, char *argv[])
 			count += single_count;
 		}
 
-		if (!PERF_COUNTER_RAW(event_id[counter]) &&
-		    (event_id[counter] == PERF_COUNT_CPU_CLOCK ||
-		     event_id[counter] == PERF_COUNT_TASK_CLOCK)) {
+		if (event_id[counter] == EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CPU_CLOCK) ||
+		    event_id[counter] == EID(PERF_TYPE_SOFTWARE, PERF_COUNT_TASK_CLOCK)) {
 
 			double msecs = (double)count / 1000000;
 
@@ -531,8 +518,6 @@ int do_perfstat(int argc, char *argv[])
 			fprintf(stderr, " %14Ld  %-20s (events)\n",
 				count, event_name(counter));
 		}
-		if (!counter)
-			fprintf(stderr, "\n");
 	}
 	fprintf(stderr, "\n");
 	fprintf(stderr, " Wall-clock time elapsed: %12.6f msecs\n",
@@ -554,7 +539,7 @@ struct sym_entry {
 	char			*sym;
 	unsigned long		count[MAX_COUNTERS];
 	int			skip;
-	GList			*source;
+	struct source_line	*source;
 };
 
 #define MAX_SYMS		100000
@@ -855,6 +840,7 @@ static void parse_vmlinux(char *filename)
 	if (!file)
 		return;
 
+	lines_tail = &lines;
 	while (!feof(file)) {
 		struct source_line *src;
 		size_t dummy = 0;
@@ -873,7 +859,9 @@ static void parse_vmlinux(char *filename)
 		if (c)
 			*c = 0;
 
-		lines = g_list_prepend(lines, src);
+		src->next = NULL;
+		*lines_tail = src;
+		lines_tail = &src->next;
 
 		if (strlen(src->line)>8 && src->line[8] == ':')
 			src->EIP = strtoull(src->line, NULL, 16);
@@ -881,52 +869,43 @@ static void parse_vmlinux(char *filename)
 			src->EIP = strtoull(src->line, NULL, 16);
 	}
 	pclose(file);
-	lines = g_list_reverse(lines);
 }
 
 static void record_precise_ip(uint64_t ip)
 {
 	struct source_line *line;
-	GList *item;
 
-	item = g_list_first(lines);
-	while (item) {
-		line = item->data;
+	for (line = lines; line; line = line->next) {
 		if (line->EIP == ip)
 			line->count++;
 		if (line->EIP > ip)
 			break;
-		item = g_list_next(item);
 	}
 }
 
 static void lookup_sym_in_vmlinux(struct sym_entry *sym)
 {
 	struct source_line *line;
-	GList *item;
 	char pattern[PATH_MAX];
 	sprintf(pattern, "<%s>:", sym->sym);
 
-	item = g_list_first(lines);
-	while (item) {
-		line = item->data;
+	for (line = lines; line; line = line->next) {
 		if (strstr(line->line, pattern)) {
-			sym->source = item;
+			sym->source = line;
 			break;
 		}
-		item = g_list_next(item);
 	}
 }
 
-void show_lines(GList *item_queue, int item_queue_count)
+static void show_lines(struct source_line *line_queue, int line_queue_count)
 {
 	int i;
 	struct source_line *line;
 
-	for (i = 0; i < item_queue_count; i++) {
-		line = item_queue->data;
+	line = line_queue;
+	for (i = 0; i < line_queue_count; i++) {
 		printf("%8li\t%s\n", line->count, line->line);
-		item_queue = g_list_next(item_queue);
+		line = line->next;
 	}
 }
 
@@ -935,10 +914,9 @@ void show_lines(GList *item_queue, int item_queue_count)
 static void show_details(struct sym_entry *sym)
 {
 	struct source_line *line;
-	GList *item;
+	struct source_line *line_queue = NULL;
 	int displayed = 0;
-	GList *item_queue = NULL;
-	int item_queue_count = 0;
+	int line_queue_count = 0;
 
 	if (!sym->source)
 		lookup_sym_in_vmlinux(sym);
@@ -947,30 +925,29 @@ static void show_details(struct sym_entry *sym)
 
 	printf("Showing details for %s\n", sym->sym);
 
-	item = sym->source;
-	while (item) {
-		line = item->data;
+	line = sym->source;
+	while (line) {
 		if (displayed && strstr(line->line, ">:"))
 			break;
 
-		if (!item_queue_count)
-			item_queue = item;
-		item_queue_count ++;
+		if (!line_queue_count)
+			line_queue = line;
+		line_queue_count ++;
 
 		if (line->count >= count_filter) {
-			show_lines(item_queue, item_queue_count);
-			item_queue_count = 0;
-			item_queue = NULL;
-		} else if (item_queue_count > TRACE_COUNT) {
-			item_queue = g_list_next(item_queue);
-			item_queue_count --;
+			show_lines(line_queue, line_queue_count);
+			line_queue_count = 0;
+			line_queue = NULL;
+		} else if (line_queue_count > TRACE_COUNT) {
+			line_queue = line_queue->next;
+			line_queue_count --;
 		}
 
 		line->count = 0;
 		displayed++;
 		if (displayed > 300)
 			break;
-		item = g_list_next(item);
+		line = line->next;
 	}
 }
 
@@ -1201,6 +1178,10 @@ int main(int argc, char *argv[])
 	if (tid != -1 || profile_cpu != -1)
 		nr_cpus = 1;
 
+	parse_symbols();
+	if (vmlinux && sym_filter_entry)
+		parse_vmlinux(vmlinux);
+
 	for (i = 0; i < nr_cpus; i++) {
 		group_fd = -1;
 		for (counter = 0; counter < nr_counters; counter++) {
@@ -1216,15 +1197,16 @@ int main(int argc, char *argv[])
 			hw_event.nmi		= nmi;
 
 			fd[i][counter] = sys_perf_counter_open(&hw_event, tid, cpu, group_fd, 0);
-			fcntl(fd[i][counter], F_SETFL, O_NONBLOCK);
 			if (fd[i][counter] < 0) {
+				int err = errno;
 				printf("kerneltop error: syscall returned with %d (%s)\n",
-					fd[i][counter], strerror(-fd[i][counter]));
-				if (fd[i][counter] == -1)
+					fd[i][counter], strerror(err));
+				if (err == EPERM)
 					printf("Are you root?\n");
 				exit(-1);
 			}
 			assert(fd[i][counter] >= 0);
+			fcntl(fd[i][counter], F_SETFL, O_NONBLOCK);
 
 			/*
 			 * First counter acts as the group leader:
@@ -1248,10 +1230,6 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	parse_symbols();
-	if (vmlinux && sym_filter_entry)
-		parse_vmlinux(vmlinux);
-
 	printf("KernelTop refresh period: %d seconds\n", delay_secs);
 	last_refresh = time(NULL);
 

Re: [PATCH] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Peter Zijlstra]

On Tue, 2009-03-24 at 16:52 +1100, Paul Mackerras wrote:
> The glib dependency in kerneltop.c is only for a little bit of list
> manipulation, and I find it inconvenient.  This adds a 'next' field to
> struct source_line, which lets us link them together into a list.  The
> code to do the linking ourselves turns out to be no longer or more
> difficult than using glib.

Awesome!! now I can compile on my test machine as well (yum got stuck,
it doesn't want to go fwd nor backwards, and I can't find time to
reinstall the thing).

> This also fixes a few other problems:

> - We had two CPU migration counters in the default set, which seems
>   unnecessary; I changed one of them to a context switch counter.

Ooh, my bad, copy/paste trouble I imagine.

> - On a test machine here, parse_symbols() and parse_vmlinux() were
>   taking long enough (almost 0.5 seconds) for the mmap buffer to
>   overflow before we got to the first mmap_read() call, so this moves
>   them before we open all the counters.

Ah, thanks for looking into that!

Re: [PATCH] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Ingo Molnar]

* Paul Mackerras <paulus@samba.org> wrote:

> The glib dependency in kerneltop.c is only for a little bit of list
> manipulation, and I find it inconvenient.  This adds a 'next' field to
> struct source_line, which lets us link them together into a list.  The
> code to do the linking ourselves turns out to be no longer or more
> difficult than using glib.
> 
> This also fixes a few other problems:
> 
> - We need to #include <limits.h> to get PATH_MAX on powerpc.
> 
> - We need to #include <linux/types.h> rather than have our own
>   definitions of __u64 and __s64; on powerpc the installed headers
>   define them to be unsigned long and long respectively, and if we
>   have our own, different definition here that causes a compile error.
> 
> - This takes out the x86 setting of errno from -ret in
>   sys_perf_counter_open.  My experiments on x86 indicate that the
>   glibc syscall() does this for us already.
> 
> - We had two CPU migration counters in the default set, which seems
>   unnecessary; I changed one of them to a context switch counter.
> 
> - In perfstat mode we were printing CPU cycles and instructions as
>   milliseconds, and the cpu clock and task clock counters as events.
>   This fixes that.
> 
> - In perfstat mode we were still printing a blank line after the first
>   counter, which was a holdover from when a task clock counter was
>   automatically included as the first counter.  This removes the blank
>   line.
> 
> - On a test machine here, parse_symbols() and parse_vmlinux() were
>   taking long enough (almost 0.5 seconds) for the mmap buffer to
>   overflow before we got to the first mmap_read() call, so this moves
>   them before we open all the counters.
> 
> - The error message if sys_perf_counter_open fails needs to use errno,
>   not -fd[i][counter].
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> 
> ---
>  Documentation/perf_counter/Makefile    |    2 +-
>  Documentation/perf_counter/kerneltop.c |  112 +++++++++++++-------------------
>  2 files changed, 46 insertions(+), 68 deletions(-)

Very nice, thanks Paul!

	Ingo

[tip:perfcounters/core] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Paul Mackerras]

Commit-ID:  6357003d6c8bc0056d79768692955da6d151880f
Gitweb:     http://git.kernel.org/tip/6357003d6c8bc0056d79768692955da6d151880f
Author:     Paul Mackerras <paulus@samba.org>
AuthorDate: Tue, 24 Mar 2009 16:52:34 +1100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Mar 2009 10:20:07 +0100

perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

The glib dependency in kerneltop.c is only for a little bit of list
manipulation, and I find it inconvenient.  This adds a 'next' field to
struct source_line, which lets us link them together into a list.  The
code to do the linking ourselves turns out to be no longer or more
difficult than using glib.

This also fixes a few other problems:

- We need to #include <limits.h> to get PATH_MAX on powerpc.

- We need to #include <linux/types.h> rather than have our own
  definitions of __u64 and __s64; on powerpc the installed headers
  define them to be unsigned long and long respectively, and if we
  have our own, different definition here that causes a compile error.

- This takes out the x86 setting of errno from -ret in
  sys_perf_counter_open.  My experiments on x86 indicate that the
  glibc syscall() does this for us already.

- We had two CPU migration counters in the default set, which seems
  unnecessary; I changed one of them to a context switch counter.

- In perfstat mode we were printing CPU cycles and instructions as
  milliseconds, and the cpu clock and task clock counters as events.
  This fixes that.

- In perfstat mode we were still printing a blank line after the first
  counter, which was a holdover from when a task clock counter was
  automatically included as the first counter.  This removes the blank
  line.

- On a test machine here, parse_symbols() and parse_vmlinux() were
  taking long enough (almost 0.5 seconds) for the mmap buffer to
  overflow before we got to the first mmap_read() call, so this moves
  them before we open all the counters.

- The error message if sys_perf_counter_open fails needs to use errno,
  not -fd[i][counter].

Signed-off-by: Paul Mackerras <paulus@samba.org>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Mike Galbraith <efault@gmx.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
LKML-Reference: <18888.29986.340328.540512@cargo.ozlabs.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 Documentation/perf_counter/Makefile    |    2 +-
 Documentation/perf_counter/kerneltop.c |  112 +++++++++++++-------------------
 2 files changed, 46 insertions(+), 68 deletions(-)

diff --git a/Documentation/perf_counter/Makefile b/Documentation/perf_counter/Makefile
index 666da95..194b662 100644
--- a/Documentation/perf_counter/Makefile
+++ b/Documentation/perf_counter/Makefile
@@ -3,7 +3,7 @@ BINS = kerneltop perfstat
 all: $(BINS)
 
 kerneltop: kerneltop.c ../../include/linux/perf_counter.h
-	cc -O6 -Wall -lrt `pkg-config --cflags --libs glib-2.0` -o $@ $<
+	cc -O6 -Wall -lrt -o $@ $<
 
 perfstat: kerneltop
 	ln -sf kerneltop perfstat
diff --git a/Documentation/perf_counter/kerneltop.c b/Documentation/perf_counter/kerneltop.c
index 2ab29b5..ea13e4e 100644
--- a/Documentation/perf_counter/kerneltop.c
+++ b/Documentation/perf_counter/kerneltop.c
@@ -3,7 +3,7 @@
 
    Build with:
 
-     cc -O6 -Wall -lrt `pkg-config --cflags --libs glib-2.0` -o kerneltop kerneltop.c
+     cc -O6 -Wall -c -o kerneltop.o kerneltop.c -lrt
 
    Sample output:
 
@@ -56,6 +56,7 @@
   *   Yanmin Zhang <yanmin.zhang@intel.com>
   *   Wu Fengguang <fengguang.wu@intel.com>
   *   Mike Galbraith <efault@gmx.de>
+  *   Paul Mackerras <paulus@samba.org>
   *
   * Released under the GPL v2. (and only v2, not any later version)
   */
@@ -68,6 +69,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
+#include <limits.h>
 #include <getopt.h>
 #include <assert.h>
 #include <fcntl.h>
@@ -76,8 +78,6 @@
 #include <ctype.h>
 #include <time.h>
 
-#include <glib.h>
-
 #include <sys/syscall.h>
 #include <sys/ioctl.h>
 #include <sys/poll.h>
@@ -87,6 +87,7 @@
 #include <sys/mman.h>
 
 #include <linux/unistd.h>
+#include <linux/types.h>
 
 #include "../../include/linux/perf_counter.h"
 
@@ -114,11 +115,6 @@
 #define __user
 #define asmlinkage
 
-typedef unsigned int            __u32;
-typedef unsigned long long      __u64;
-typedef long long               __s64;
-
-
 #ifdef __x86_64__
 #define __NR_perf_counter_open 295
 #define rmb()		asm volatile("lfence" ::: "memory")
@@ -146,17 +142,8 @@ asmlinkage int sys_perf_counter_open(
         int                             group_fd,
         unsigned long                   flags)
 {
-        int ret;
-
-        ret = syscall(
+        return syscall(
                 __NR_perf_counter_open, hw_event_uptr, pid, cpu, group_fd, flags);
-#if defined(__x86_64__) || defined(__i386__)
-        if (ret < 0 && ret > -4096) {
-                errno = -ret;
-                ret = -1;
-        }
-#endif
-        return ret;
 }
 
 #define MAX_COUNTERS			64
@@ -170,7 +157,7 @@ static int			system_wide			=  0;
 static int			nr_counters			=  0;
 static __u64			event_id[MAX_COUNTERS]		= {
 	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_TASK_CLOCK),
-	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CPU_MIGRATIONS),
+	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CONTEXT_SWITCHES),
 	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CPU_MIGRATIONS),
 	EID(PERF_TYPE_SOFTWARE, PERF_COUNT_PAGE_FAULTS),
 
@@ -202,14 +189,15 @@ static int			delay_secs			=  2;
 static int			zero;
 static int			dump_symtab;
 
-static GList			*lines;
-
 struct source_line {
 	uint64_t		EIP;
 	unsigned long		count;
 	char			*line;
+	struct source_line	*next;
 };
 
+static struct source_line	*lines;
+static struct source_line	**lines_tail;
 
 const unsigned int default_count[] = {
 	1000000,
@@ -519,9 +507,8 @@ int do_perfstat(int argc, char *argv[])
 			count += single_count;
 		}
 
-		if (!PERF_COUNTER_RAW(event_id[counter]) &&
-		    (event_id[counter] == PERF_COUNT_CPU_CLOCK ||
-		     event_id[counter] == PERF_COUNT_TASK_CLOCK)) {
+		if (event_id[counter] == EID(PERF_TYPE_SOFTWARE, PERF_COUNT_CPU_CLOCK) ||
+		    event_id[counter] == EID(PERF_TYPE_SOFTWARE, PERF_COUNT_TASK_CLOCK)) {
 
 			double msecs = (double)count / 1000000;
 
@@ -531,8 +518,6 @@ int do_perfstat(int argc, char *argv[])
 			fprintf(stderr, " %14Ld  %-20s (events)\n",
 				count, event_name(counter));
 		}
-		if (!counter)
-			fprintf(stderr, "\n");
 	}
 	fprintf(stderr, "\n");
 	fprintf(stderr, " Wall-clock time elapsed: %12.6f msecs\n",
@@ -554,7 +539,7 @@ struct sym_entry {
 	char			*sym;
 	unsigned long		count[MAX_COUNTERS];
 	int			skip;
-	GList			*source;
+	struct source_line	*source;
 };
 
 #define MAX_SYMS		100000
@@ -855,6 +840,7 @@ static void parse_vmlinux(char *filename)
 	if (!file)
 		return;
 
+	lines_tail = &lines;
 	while (!feof(file)) {
 		struct source_line *src;
 		size_t dummy = 0;
@@ -873,7 +859,9 @@ static void parse_vmlinux(char *filename)
 		if (c)
 			*c = 0;
 
-		lines = g_list_prepend(lines, src);
+		src->next = NULL;
+		*lines_tail = src;
+		lines_tail = &src->next;
 
 		if (strlen(src->line)>8 && src->line[8] == ':')
 			src->EIP = strtoull(src->line, NULL, 16);
@@ -881,52 +869,43 @@ static void parse_vmlinux(char *filename)
 			src->EIP = strtoull(src->line, NULL, 16);
 	}
 	pclose(file);
-	lines = g_list_reverse(lines);
 }
 
 static void record_precise_ip(uint64_t ip)
 {
 	struct source_line *line;
-	GList *item;
 
-	item = g_list_first(lines);
-	while (item) {
-		line = item->data;
+	for (line = lines; line; line = line->next) {
 		if (line->EIP == ip)
 			line->count++;
 		if (line->EIP > ip)
 			break;
-		item = g_list_next(item);
 	}
 }
 
 static void lookup_sym_in_vmlinux(struct sym_entry *sym)
 {
 	struct source_line *line;
-	GList *item;
 	char pattern[PATH_MAX];
 	sprintf(pattern, "<%s>:", sym->sym);
 
-	item = g_list_first(lines);
-	while (item) {
-		line = item->data;
+	for (line = lines; line; line = line->next) {
 		if (strstr(line->line, pattern)) {
-			sym->source = item;
+			sym->source = line;
 			break;
 		}
-		item = g_list_next(item);
 	}
 }
 
-void show_lines(GList *item_queue, int item_queue_count)
+static void show_lines(struct source_line *line_queue, int line_queue_count)
 {
 	int i;
 	struct source_line *line;
 
-	for (i = 0; i < item_queue_count; i++) {
-		line = item_queue->data;
+	line = line_queue;
+	for (i = 0; i < line_queue_count; i++) {
 		printf("%8li\t%s\n", line->count, line->line);
-		item_queue = g_list_next(item_queue);
+		line = line->next;
 	}
 }
 
@@ -935,10 +914,9 @@ void show_lines(GList *item_queue, int item_queue_count)
 static void show_details(struct sym_entry *sym)
 {
 	struct source_line *line;
-	GList *item;
+	struct source_line *line_queue = NULL;
 	int displayed = 0;
-	GList *item_queue = NULL;
-	int item_queue_count = 0;
+	int line_queue_count = 0;
 
 	if (!sym->source)
 		lookup_sym_in_vmlinux(sym);
@@ -947,30 +925,29 @@ static void show_details(struct sym_entry *sym)
 
 	printf("Showing details for %s\n", sym->sym);
 
-	item = sym->source;
-	while (item) {
-		line = item->data;
+	line = sym->source;
+	while (line) {
 		if (displayed && strstr(line->line, ">:"))
 			break;
 
-		if (!item_queue_count)
-			item_queue = item;
-		item_queue_count ++;
+		if (!line_queue_count)
+			line_queue = line;
+		line_queue_count ++;
 
 		if (line->count >= count_filter) {
-			show_lines(item_queue, item_queue_count);
-			item_queue_count = 0;
-			item_queue = NULL;
-		} else if (item_queue_count > TRACE_COUNT) {
-			item_queue = g_list_next(item_queue);
-			item_queue_count --;
+			show_lines(line_queue, line_queue_count);
+			line_queue_count = 0;
+			line_queue = NULL;
+		} else if (line_queue_count > TRACE_COUNT) {
+			line_queue = line_queue->next;
+			line_queue_count --;
 		}
 
 		line->count = 0;
 		displayed++;
 		if (displayed > 300)
 			break;
-		item = g_list_next(item);
+		line = line->next;
 	}
 }
 
@@ -1201,6 +1178,10 @@ int main(int argc, char *argv[])
 	if (tid != -1 || profile_cpu != -1)
 		nr_cpus = 1;
 
+	parse_symbols();
+	if (vmlinux && sym_filter_entry)
+		parse_vmlinux(vmlinux);
+
 	for (i = 0; i < nr_cpus; i++) {
 		group_fd = -1;
 		for (counter = 0; counter < nr_counters; counter++) {
@@ -1216,15 +1197,16 @@ int main(int argc, char *argv[])
 			hw_event.nmi		= nmi;
 
 			fd[i][counter] = sys_perf_counter_open(&hw_event, tid, cpu, group_fd, 0);
-			fcntl(fd[i][counter], F_SETFL, O_NONBLOCK);
 			if (fd[i][counter] < 0) {
+				int err = errno;
 				printf("kerneltop error: syscall returned with %d (%s)\n",
-					fd[i][counter], strerror(-fd[i][counter]));
-				if (fd[i][counter] == -1)
+					fd[i][counter], strerror(err));
+				if (err == EPERM)
 					printf("Are you root?\n");
 				exit(-1);
 			}
 			assert(fd[i][counter] >= 0);
+			fcntl(fd[i][counter], F_SETFL, O_NONBLOCK);
 
 			/*
 			 * First counter acts as the group leader:
@@ -1248,10 +1230,6 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	parse_symbols();
-	if (vmlinux && sym_filter_entry)
-		parse_vmlinux(vmlinux);
-
 	printf("KernelTop refresh period: %d seconds\n", delay_secs);
 	last_refresh = time(NULL);
 

Re: [tip:perfcounters/core] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Ingo Molnar]

btw.,:

| Performance counter stats for 'ls':
|
|        1915281  task clock ticks     (events)
|
|              6  CPU migrations       (events)
|              6  CPU migrations       (events)
|            284  pagefaults           (events)
|       4.333761  CPU cycles           (msecs)
|       1.544153  instructions         (msecs)
|         620030  cache references     (events)
|          17632  cache misses         (events)
|
| Wall-clock time elapsed:     1.868138 msecs

the 'instructions' field is weird - it should not be millisecs but 
'events'.

	Ingo

[tip:perfcounters/core] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c, fix poll()

[Peter Zijlstra]

Commit-ID:  afc2a3523fc6664f3713b594daa1857f37ad87df
Gitweb:     http://git.kernel.org/tip/afc2a3523fc6664f3713b594daa1857f37ad87df
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Tue, 24 Mar 2009 10:50:24 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Mar 2009 10:56:18 +0100

perf_counter tools: remove glib dependency and fix bugs in kerneltop.c, fix poll()

Paul Mackerras wrote:

> I noticed the poll stuff is bogus - we have a 2D array of struct
> pollfds (MAX_NR_CPUS x MAX_COUNTERS), we fill in a sub-array (with the
> rest being uninitialized, since the array is on the stack) and then
> pass the first nr_cpus elements to poll.  Not what we really meant, I
> suspect. :)  Not even if we only have one counter, since it's the
> counter dimension that varies fastest.

This should fix the most obvious poll fubar.. not enough to fix the
full problem though..

Reported-by: Paul Mackerras <paulus@samba.org>
Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arjan van de Ven <arjan@linux.intel.com>
LKML-Reference: <18888.29986.340328.540512@cargo.ozlabs.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 Documentation/perf_counter/kerneltop.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/perf_counter/kerneltop.c b/Documentation/perf_counter/kerneltop.c
index ea13e4e..7ebde7a 100644
--- a/Documentation/perf_counter/kerneltop.c
+++ b/Documentation/perf_counter/kerneltop.c
@@ -1157,10 +1157,10 @@ static void mmap_read(struct mmap_data *md)
 
 int main(int argc, char *argv[])
 {
-	struct pollfd event_array[MAX_NR_CPUS][MAX_COUNTERS];
+	struct pollfd event_array[MAX_NR_CPUS * MAX_COUNTERS];
 	struct mmap_data mmap_array[MAX_NR_CPUS][MAX_COUNTERS];
 	struct perf_counter_hw_event hw_event;
-	int i, counter, group_fd;
+	int i, counter, group_fd, nr_poll = 0;
 	unsigned int cpu;
 	int ret;
 
@@ -1214,8 +1214,9 @@ int main(int argc, char *argv[])
 			if (group && group_fd == -1)
 				group_fd = fd[i][counter];
 
-			event_array[i][counter].fd = fd[i][counter];
-			event_array[i][counter].events = POLLIN;
+			event_array[nr_poll].fd = fd[i][counter];
+			event_array[nr_poll].events = POLLIN;
+			nr_poll++;
 
 			mmap_array[i][counter].counter = counter;
 			mmap_array[i][counter].prev = 0;
@@ -1247,7 +1248,7 @@ int main(int argc, char *argv[])
 		}
 
 		if (hits == events)
-			ret = poll(event_array[0], nr_cpus, 1000);
+			ret = poll(event_array, nr_poll, 1000);
 		hits = events;
 	}
 

Re: [tip:perfcounters/core] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Paul Mackerras]

Ingo Molnar writes:

> btw.,:
> 
> | Performance counter stats for 'ls':
> |
> |        1915281  task clock ticks     (events)
> |
> |              6  CPU migrations       (events)
> |              6  CPU migrations       (events)
> |            284  pagefaults           (events)
> |       4.333761  CPU cycles           (msecs)
> |       1.544153  instructions         (msecs)
> |         620030  cache references     (events)
> |          17632  cache misses         (events)
> |
> | Wall-clock time elapsed:     1.868138 msecs
> 
> the 'instructions' field is weird - it should not be millisecs but 
> 'events'.

Is that with my patch applied?  That was one of the things I fixed, or
at least intended to fix :).

Paul.

Re: [tip:perfcounters/core] perf_counter tools: remove glib dependency and fix bugs in kerneltop.c

[Ingo Molnar]

* Paul Mackerras <paulus@samba.org> wrote:

> Ingo Molnar writes:
> 
> > btw.,:
> > 
> > | Performance counter stats for 'ls':
> > |
> > |        1915281  task clock ticks     (events)
> > |
> > |              6  CPU migrations       (events)
> > |              6  CPU migrations       (events)
> > |            284  pagefaults           (events)
> > |       4.333761  CPU cycles           (msecs)
> > |       1.544153  instructions         (msecs)
> > |         620030  cache references     (events)
> > |          17632  cache misses         (events)
> > |
> > | Wall-clock time elapsed:     1.868138 msecs
> > 
> > the 'instructions' field is weird - it should not be millisecs but 
> > 'events'.
> 
> Is that with my patch applied?  That was one of the things I fixed, or
> at least intended to fix :).

ah. no. It was indeed with an ancient kernel 6 hours old or so :)

	Ingo

[PATCH] perf_counter: fix perf_poll()

[Peter Zijlstra]

The below does seem to fix things (along with the previous kerneltop.c
patch),.. however now kerneltop is going splat like:

# ./kerneltop -e 1:2 -c 10
KernelTop refresh period: 2 seconds
ret: 1
 left: 0000000080200000
   ip: 000000008038d4ad
right: 00000000ffffffff
kerneltop: kerneltop.c:985: record_ip: Assertion `left <= ip && ip <= right' failed.
Aborted

Which isn't making much sense..

---
Only return a poll event when there's actually been one, poll_wait()
doesn't actually wait for the waitq you pass it, it only enqueues you on
it.

Only once all FDs have been iterated and non returned a poll-event will
it schedule().

Also make it return POLL_HUP when there's not mmap() area to read from.

Further, fix a silly bug in the write code.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/perf_counter.h |    1 +
 kernel/perf_counter.c        |   14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_counter.h b/include/linux/perf_counter.h
index 2b5e66d..48212c1 100644
--- a/include/linux/perf_counter.h
+++ b/include/linux/perf_counter.h
@@ -246,6 +246,7 @@ struct file;
 struct perf_mmap_data {
 	struct rcu_head			rcu_head;
 	int				nr_pages;
+	atomic_t			wakeup;
 	atomic_t			head;
 	struct perf_counter_mmap_page   *user_page;
 	void 				*data_pages[0];
diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index 0dfe910..affe227 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -1161,7 +1161,16 @@ perf_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
 static unsigned int perf_poll(struct file *file, poll_table *wait)
 {
 	struct perf_counter *counter = file->private_data;
-	unsigned int events = POLLIN;
+	struct perf_mmap_data *data;
+	unsigned int events;
+
+	rcu_read_lock();
+	data = rcu_dereference(counter->data);
+	if (data)
+		events = atomic_xchg(&data->wakeup, 0);
+	else
+		events = POLL_HUP;
+	rcu_read_unlock();
 
 	poll_wait(file, &counter->waitq, wait);
 
@@ -1425,7 +1434,7 @@ static int perf_output_write(struct perf_counter
*counter, int nmi,
 
 	do {
 		offset = head = atomic_read(&data->head);
-		head += sizeof(u64);
+		head += size;
 	} while (atomic_cmpxchg(&data->head, offset, head) != offset);
 
 	wakeup = (offset >> PAGE_SHIFT) != (head >> PAGE_SHIFT);
@@ -1446,6 +1455,7 @@ static int perf_output_write(struct perf_counter
*counter, int nmi,
 	 * generate a poll() wakeup for every page boundary crossed
 	 */
 	if (wakeup) {
+		atomic_xchg(&data->wakeup, POLL_IN);
 		__perf_counter_update_userpage(counter, data);
 		if (nmi) {
 			counter->wakeup_pending = 1;

Re: [PATCH] perf_counter: fix perf_poll()

[Peter Zijlstra]

On Tue, 2009-03-24 at 13:18 +0100, Peter Zijlstra wrote:
> The below does seem to fix things (along with the previous kerneltop.c
> patch),.. however now kerneltop is going splat like:
> 
> # ./kerneltop -e 1:2 -c 10
> KernelTop refresh period: 2 seconds
> ret: 1
>  left: 0000000080200000
>    ip: 000000008038d4ad
> right: 00000000ffffffff
> kerneltop: kerneltop.c:985: record_ip: Assertion `left <= ip && ip <= right' failed.
> Aborted
> 
> Which isn't making much sense..

Seems to be an artefact of running a 32bit kerneltop on a 64bit kernel..
freshyl compiled 64bit kerneltop works just fine.

Re: [PATCH] perf_counter: fix perf_poll()

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, 2009-03-24 at 13:18 +0100, Peter Zijlstra wrote:
> > The below does seem to fix things (along with the previous kerneltop.c
> > patch),.. however now kerneltop is going splat like:
> > 
> > # ./kerneltop -e 1:2 -c 10
> > KernelTop refresh period: 2 seconds
> > ret: 1
> >  left: 0000000080200000
> >    ip: 000000008038d4ad
> > right: 00000000ffffffff
> > kerneltop: kerneltop.c:985: record_ip: Assertion `left <= ip && ip <= right' failed.
> > Aborted
> > 
> > Which isn't making much sense..
> 
> Seems to be an artefact of running a 32bit kerneltop on a 64bit 
> kernel.. freshyl compiled 64bit kerneltop works just fine.

Looks like you discovered a real bug by accident :)

	Ingo

[tip:perfcounters/core] perf_counter: fix perf_poll()

[Peter Zijlstra]

Commit-ID:  47a6aae483f6b98509a4565ad2d8916de77ee078
Gitweb:     http://git.kernel.org/tip/47a6aae483f6b98509a4565ad2d8916de77ee078
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 24 Mar 2009 13:18:16 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Mar 2009 13:28:56 +0100

perf_counter: fix perf_poll()

Impact: fix kerneltop 100% CPU usage

Only return a poll event when there's actually been one, poll_wait()
doesn't actually wait for the waitq you pass it, it only enqueues
you on it.

Only once all FDs have been iterated and none of thm returned a
poll-event will it schedule().

Also make it return POLL_HUP when there's not mmap() area to read from.

Further, fix a silly bug in the write code.

Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arjan van de Ven <arjan@infradead.org>
LKML-Reference: <1237897096.24918.181.camel@twins>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 include/linux/perf_counter.h |    1 +
 kernel/perf_counter.c        |   14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_counter.h b/include/linux/perf_counter.h
index 2b5e66d..48212c1 100644
--- a/include/linux/perf_counter.h
+++ b/include/linux/perf_counter.h
@@ -246,6 +246,7 @@ struct file;
 struct perf_mmap_data {
 	struct rcu_head			rcu_head;
 	int				nr_pages;
+	atomic_t			wakeup;
 	atomic_t			head;
 	struct perf_counter_mmap_page   *user_page;
 	void 				*data_pages[0];
diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index 0dfe910..affe227 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -1161,7 +1161,16 @@ perf_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 static unsigned int perf_poll(struct file *file, poll_table *wait)
 {
 	struct perf_counter *counter = file->private_data;
-	unsigned int events = POLLIN;
+	struct perf_mmap_data *data;
+	unsigned int events;
+
+	rcu_read_lock();
+	data = rcu_dereference(counter->data);
+	if (data)
+		events = atomic_xchg(&data->wakeup, 0);
+	else
+		events = POLL_HUP;
+	rcu_read_unlock();
 
 	poll_wait(file, &counter->waitq, wait);
 
@@ -1425,7 +1434,7 @@ static int perf_output_write(struct perf_counter *counter, int nmi,
 
 	do {
 		offset = head = atomic_read(&data->head);
-		head += sizeof(u64);
+		head += size;
 	} while (atomic_cmpxchg(&data->head, offset, head) != offset);
 
 	wakeup = (offset >> PAGE_SHIFT) != (head >> PAGE_SHIFT);
@@ -1446,6 +1455,7 @@ static int perf_output_write(struct perf_counter *counter, int nmi,
 	 * generate a poll() wakeup for every page boundary crossed
 	 */
 	if (wakeup) {
+		atomic_xchg(&data->wakeup, POLL_IN);
 		__perf_counter_update_userpage(counter, data);
 		if (nmi) {
 			counter->wakeup_pending = 1;