Skip to contentSkip to conversation

perf/urgent fixes

GIT PULL New Arnaldo Carvalho de Melo 2 participants 9 messages Apr 27, 2015 – May 1, 2015

Subsystems (2)
lore
Share & tools
lore

Arnaldo Carvalho de Melo

author maintainer
#0 lore raw

Hi Ingo,

Please consider pulling, this is on top of my previous 'perf-urgent-for-mingo'
pull request.

- Arnaldo

The following changes since commit de28c15daf60e9625bece22f13a091fac8d05f1d:

tools lib api: Undefine _FORTIFY_SOURCE before setting it (2015-04-23 17:08:23 -0300)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo-2

for you to fetch changes up to 1d90a685eb75a56648d7dd22c704a1a6da516de9:

perf bench numa: Fix immediate meeting of convergence condition (2015-04-27 13:57:50 -0300)

----------------------------------------------------------------
perf/urgent fixes:

User visible:

. Fix a segfault in 'perf top' when kernel map is restricted (Wang Nan)

. Fix hung wakeup tasks after requeueing in 'perf bench futex' (Davidlohr Bueso)

. Fix bug in perf probe global variables handling, missing curly braces on
an if body (He Kuang)

. 'perf bench numa' fixes (command line help/handling, etc) (Petr Holasek)

Build fixes:

. 'perf kmem' on RHEL6/OL6 (David Ahern)

. libtraceevent on 32-bit arch (Namhyung Kim)

----------------------------------------------------------------
David Ahern (1):
perf kmem: Fix compiles on RHEL6/OL6

Davidlohr Bueso (1):
perf bench futex: Fix hung wakeup tasks after requeueing

He Kuang (1):
perf probe: Fix bug with global variables handling

Namhyung Kim (1):
tools lib traceevent: Fix build failure on 32-bit arch

Petr Holasek (2):
perf bench numa: Fixes of --quiet argument
perf bench numa: Fix immediate meeting of convergence condition

Wang Nan (1):
perf top: Fix a segfault when kernel map is restricted.

 tools/lib/traceevent/event-parse.c |  2 +-
 tools/perf/bench/futex-requeue.c   | 15 ++++++-----
 tools/perf/bench/numa.c            | 12 +++++++--
 tools/perf/builtin-kmem.c          | 54 +++++++++++++++++++-------------------
 tools/perf/builtin-top.c           |  2 +-
 tools/perf/util/probe-finder.c     |  4 ++-
 6 files changed, 50 insertions(+), 39 deletions(-)
author maintainer
#1 lore raw

PATCH 1/7 perf kmem: Fix compiles on RHEL6/OL6

From: David Ahern <[email protected]>

0d68bc92c48 breaks compiles on RHEL6/OL6:
cc1: warnings being treated as errors
builtin-kmem.c: In function ‘search_page_alloc_stat’:
builtin-kmem.c:322: error: declaration of ‘stat’ shadows a global declaration
node = &parent->rb_left;
/usr/include/sys/stat.h:455: error: shadowed declaration is here builtin-kmem.c: In function ‘perf_evsel__process_page_alloc_event’:
builtin-kmem.c:378: error: declaration of ‘stat’ shadows a global declaration /usr/include/sys/stat.h:455: error: shadowed declaration is here builtin-kmem.c: In function ‘perf_evsel__process_page_free_event’:
builtin-kmem.c:431: error: declaration of ‘stat’ shadows a global declaration /usr/include/sys/stat.h:455: error: shadowed declaration is here

Rename local variable to pstat to avoid the name conflict.

---
 tools/perf/builtin-kmem.c | 54 +++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)
tools/perf/builtin-kmem.c +27 -27
index a1915b430044..1634186d537c 100644
@@ -319,7 +319,7 @@ static int page_stat_cmp(struct page_stat *a, struct page_stat *b)
 	return 0;
 }
 
-static struct page_stat *search_page_alloc_stat(struct page_stat *stat, bool create)
+static struct page_stat *search_page_alloc_stat(struct page_stat *pstat, bool create)
 {
 	struct rb_node **node = &page_alloc_tree.rb_node;
 	struct rb_node *parent = NULL;
@@ -331,7 +331,7 @@ static struct page_stat *search_page_alloc_stat(struct page_stat *stat, bool cre
 		parent = *node;
 		data = rb_entry(*node, struct page_stat, node);
 
-		cmp = page_stat_cmp(data, stat);
+		cmp = page_stat_cmp(data, pstat);
 		if (cmp < 0)
 			node = &parent->rb_left;
 		else if (cmp > 0)
@@ -345,10 +345,10 @@ static struct page_stat *search_page_alloc_stat(struct page_stat *stat, bool cre
 
 	data = zalloc(sizeof(*data));
 	if (data != NULL) {
-		data->page = stat->page;
-		data->order = stat->order;
-		data->gfp_flags = stat->gfp_flags;
-		data->migrate_type = stat->migrate_type;
+		data->page = pstat->page;
+		data->order = pstat->order;
+		data->gfp_flags = pstat->gfp_flags;
+		data->migrate_type = pstat->migrate_type;
 
 		rb_link_node(&data->node, parent, node);
 		rb_insert_color(&data->node, &page_alloc_tree);
@@ -375,7 +375,7 @@ static int perf_evsel__process_page_alloc_event(struct perf_evsel *evsel,
 	unsigned int migrate_type = perf_evsel__intval(evsel, sample,
 						       "migratetype");
 	u64 bytes = kmem_page_size << order;
-	struct page_stat *stat;
+	struct page_stat *pstat;
 	struct page_stat this = {
 		.order = order,
 		.gfp_flags = gfp_flags,
@@ -401,21 +401,21 @@ static int perf_evsel__process_page_alloc_event(struct perf_evsel *evsel,
 	 * This is to find the current page (with correct gfp flags and
 	 * migrate type) at free event.
 	 */
-	stat = search_page(page, true);
-	if (stat == NULL)
+	pstat = search_page(page, true);
+	if (pstat == NULL)
 		return -ENOMEM;
 
-	stat->order = order;
-	stat->gfp_flags = gfp_flags;
-	stat->migrate_type = migrate_type;
+	pstat->order = order;
+	pstat->gfp_flags = gfp_flags;
+	pstat->migrate_type = migrate_type;
 
 	this.page = page;
-	stat = search_page_alloc_stat(&this, true);
-	if (stat == NULL)
+	pstat = search_page_alloc_stat(&this, true);
+	if (pstat == NULL)
 		return -ENOMEM;
 
-	stat->nr_alloc++;
-	stat->alloc_bytes += bytes;
+	pstat->nr_alloc++;
+	pstat->alloc_bytes += bytes;
 
 	order_stats[order][migrate_type]++;
 
@@ -428,7 +428,7 @@ static int perf_evsel__process_page_free_event(struct perf_evsel *evsel,
 	u64 page;
 	unsigned int order = perf_evsel__intval(evsel, sample, "order");
 	u64 bytes = kmem_page_size << order;
-	struct page_stat *stat;
+	struct page_stat *pstat;
 	struct page_stat this = {
 		.order = order,
 	};
@@ -441,8 +441,8 @@ static int perf_evsel__process_page_free_event(struct perf_evsel *evsel,
 	nr_page_frees++;
 	total_page_free_bytes += bytes;
 
-	stat = search_page(page, false);
-	if (stat == NULL) {
+	pstat = search_page(page, false);
+	if (pstat == NULL) {
 		pr_debug2("missing free at page %"PRIx64" (order: %d)\n",
 			  page, order);
 
@@ -453,18 +453,18 @@ static int perf_evsel__process_page_free_event(struct perf_evsel *evsel,
 	}
 
 	this.page = page;
-	this.gfp_flags = stat->gfp_flags;
-	this.migrate_type = stat->migrate_type;
+	this.gfp_flags = pstat->gfp_flags;
+	this.migrate_type = pstat->migrate_type;
 
-	rb_erase(&stat->node, &page_tree);
-	free(stat);
+	rb_erase(&pstat->node, &page_tree);
+	free(pstat);
 
-	stat = search_page_alloc_stat(&this, false);
-	if (stat == NULL)
+	pstat = search_page_alloc_stat(&this, false);
+	if (pstat == NULL)
 		return -ENOENT;
 
-	stat->nr_free++;
-	stat->free_bytes += bytes;
+	pstat->nr_free++;
+	pstat->free_bytes += bytes;
 
 	return 0;
 }
Signature · 1 line
1.9.3
author maintainer
#2 lore raw

PATCH 2/7 tools lib traceevent: Fix build failure on 32-bit arch

In reply to Arnaldo Carvalho de Melo

From: Namhyung Kim <[email protected]>

In my i386 build, it failed like this:

CC event-parse.o
event-parse.c: In function 'print_str_arg':
event-parse.c:3868:5: warning: format '%lu' expects argument of type 'long unsigned int',
but argument 3 has type 'uint64_t' [-Wformat]

---
 tools/lib/traceevent/event-parse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/lib/traceevent/event-parse.c +1 -1
index 12a7e2a40c89..aa21bd55bd8a 100644
@@ -3865,7 +3865,7 @@ static void print_str_arg(struct trace_seq *s, void *data, int size,
 			} else if (el_size == 4) {
 				trace_seq_printf(s, "%u", *(uint32_t *)num);
 			} else if (el_size == 8) {
-				trace_seq_printf(s, "%lu", *(uint64_t *)num);
+				trace_seq_printf(s, "%"PRIu64, *(uint64_t *)num);
 			} else {
 				trace_seq_printf(s, "BAD SIZE:%d 0x%x",
 						 el_size, *(uint8_t *)num);
Signature · 1 line
1.9.3
author maintainer
#3 lore raw

PATCH 3/7 perf top: Fix a segfault when kernel map is restricted.

In reply to Arnaldo Carvalho de Melo

From: Wang Nan <[email protected]>

Perf top raise a warning if a kernel sample is collected but kernel map is restricted. The warning message needs to dereference al.map->dso...

However, previous perf_event__preprocess_sample() doesn't always guarantee al.map != NULL, for example, when kernel map is restricted.

This patch validates al.map before dereferencing, avoid the segfault.

Before this patch:

 $ cat /proc/sys/kernel/kptr_restrict
 1
 $ perf top -p  120183
 perf: Segmentation fault
 -------- backtrace --------
 /path/to/perf[0x509868]
 /lib64/libc.so.6(+0x3545f)[0x7f9a1540045f]
 /path/to/perf[0x448820]
 /path/to/perf(cmd_top+0xe3c)[0x44a5dc]
 /path/to/perf[0x4766a2]
 /path/to/perf(main+0x5f5)[0x42e545]
 /lib64/libc.so.6(__libc_start_main+0xf4)[0x7f9a153ecbd4]
 /path/to/perf[0x42e674]

And gdb call trace:

Program received signal SIGSEGV, Segmentation fault.
perf_event__process_sample (machine=0xa44030, sample=0x7fffffffa4c0, evsel=0xa43b00, event=0x7ffff41c3000, tool=0x7fffffffa8a0)

    at builtin-top.c:736
 736				  !RB_EMPTY_ROOT(&al.map->dso->symbols[MAP__FUNCTION]) ?
 (gdb) bt
 #0  perf_event__process_sample (machine=0xa44030, sample=0x7fffffffa4c0, evsel=0xa43b00, event=0x7ffff41c3000, tool=0x7fffffffa8a0)
     at builtin-top.c:736
 #1  perf_top__mmap_read_idx (top=top@entry=0x7fffffffa8a0, idx=idx@entry=0) at builtin-top.c:855
 #2  0x000000000044a5dd in perf_top__mmap_read (top=0x7fffffffa8a0) at builtin-top.c:872
 #3  __cmd_top (top=0x7fffffffa8a0) at builtin-top.c:997
 #4  cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1267
 #5  0x00000000004766a3 in run_builtin (p=p@entry=0x8a6ce8 <commands+264>, argc=argc@entry=3, argv=argv@entry=0x7fffffffdf70)
      at perf.c:371
 #6  0x000000000042e546 in handle_internal_command (argv=0x7fffffffdf70, argc=3) at perf.c:430
 #7  run_argv (argv=0x7fffffffdcf0, argcp=0x7fffffffdcfc) at perf.c:474
 #8  main (argc=3, argv=0x7fffffffdf70) at perf.c:589

(gdb)

---
 tools/perf/builtin-top.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/builtin-top.c +1 -1
index 1cb3436276d1..6a4d5d41c671 100644
@@ -733,7 +733,7 @@ static void perf_event__process_sample(struct perf_tool *tool,
 "Kernel address maps (/proc/{kallsyms,modules}) are restricted.\n\n"
 "Check /proc/sys/kernel/kptr_restrict.\n\n"
 "Kernel%s samples will not be resolved.\n",
-			  !RB_EMPTY_ROOT(&al.map->dso->symbols[MAP__FUNCTION]) ?
+			  al.map && !RB_EMPTY_ROOT(&al.map->dso->symbols[MAP__FUNCTION]) ?
 			  " modules" : "");
 		if (use_browser <= 0)
 			sleep(5);
Signature · 1 line
1.9.3
author maintainer
#4 lore raw

PATCH 4/7 perf probe: Fix bug with global variables handling

In reply to Arnaldo Carvalho de Melo

From: He Kuang <[email protected]>

There are missing curly braces which causes find_variable() return wrong value when probing with global variables.

This problem can be reproduced as following:

  $ perf probe -v --add='generic_perform_write global_variable_for_test'
  ...
  Try to find probe point from debuginfo.
  Probe point found: generic_perform_write+0
  Searching 'global_variable_for_test' variable in context.
  An error occurred in debuginfo analysis (-2).
    Error: Failed to add events. Reason: No such file or directory (Code: -2)

After this patch:

  $ perf probe -v --add='generic_perform_write global_variable_for_test'
  ...
  Converting variable global_variable_for_test into trace event.
  global_variable_for_test type is int.
  Found 1 probe_trace_events.
  Opening /sys/kernel/debug/tracing/kprobe_events write=1
  Added new event:
  Writing event: p:probe/generic_perform_write _stext+1237464
  global_variable_for_test=@global_variable_for_test+0:s32
    probe:generic_perform_write (on generic_perform_write with
    global_variable_for_test)

You can now use it in all perf tools, such as:

perf record -e probe:generic_perform_write -aR sleep 1

---
 tools/perf/util/probe-finder.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
tools/perf/util/probe-finder.c +3 -1
index 44554c3c2220..1c3cc07937d5 100644
@@ -578,10 +578,12 @@ static int find_variable(Dwarf_Die *sc_die, struct probe_finder *pf)
 	/* Search child die for local variables and parameters. */
 	if (!die_find_variable_at(sc_die, pf->pvar->var, pf->addr, &vr_die)) {
 		/* Search again in global variables */
-		if (!die_find_variable_at(&pf->cu_die, pf->pvar->var, 0, &vr_die))
+		if (!die_find_variable_at(&pf->cu_die, pf->pvar->var,
+						0, &vr_die)) {
 			pr_warning("Failed to find '%s' in this function.\n",
 				   pf->pvar->var);
 			ret = -ENOENT;
+		}
 	}
 	if (ret >= 0)
 		ret = convert_variable(&vr_die, pf);
Signature · 1 line
1.9.3
author maintainer
#5 lore raw

PATCH 5/7 perf bench futex: Fix hung wakeup tasks after requeueing

In reply to Arnaldo Carvalho de Melo

From: Davidlohr Bueso <[email protected]>

The futex-requeue benchmark can hang because of missing wakeups once the benchmark is done, ie:

[Run 1]: Requeued 1024 of 1024 threads in 0.3290 ms
perf: couldn't wakeup all tasks (135/1024)

This bug, while perhaps suggesting missing wakeups in kernel futex code, is merely a consequence of the crappy FUTEX_CMP_REQUEUE man page, incorrectly mentioning that the number of requeued tasks is in fact returned, not the wakeups.

This patch acknowledges this and updates the corresponding futex_wake code around it.

---
 tools/perf/bench/futex-requeue.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
tools/perf/bench/futex-requeue.c +8 -7
index bedff6b5b3cf..ad0d9b5342fb 100644
@@ -132,6 +132,9 @@ int bench_futex_requeue(int argc, const char **argv,
 	if (!fshared)
 		futex_flag = FUTEX_PRIVATE_FLAG;
 
+	if (nrequeue > nthreads)
+		nrequeue = nthreads;
+
 	printf("Run summary [PID %d]: Requeuing %d threads (from [%s] %p to %p), "
 	       "%d at a time.\n\n",  getpid(), nthreads,
 	       fshared ? "shared":"private", &futex1, &futex2, nrequeue);
@@ -161,20 +164,18 @@ int bench_futex_requeue(int argc, const char **argv,
 
 		/* Ok, all threads are patiently blocked, start requeueing */
 		gettimeofday(&start, NULL);
-		for (nrequeued = 0; nrequeued < nthreads; nrequeued += nrequeue) {
+		while (nrequeued < nthreads) {
 			/*
 			 * Do not wakeup any tasks blocked on futex1, allowing
 			 * us to really measure futex_wait functionality.
 			 */
-			futex_cmp_requeue(&futex1, 0, &futex2, 0,
-					  nrequeue, futex_flag);
+			nrequeued += futex_cmp_requeue(&futex1, 0, &futex2, 0,
+						       nrequeue, futex_flag);
 		}
+
 		gettimeofday(&end, NULL);
 		timersub(&end, &start, &runtime);
 
-		if (nrequeued > nthreads)
-			nrequeued = nthreads;
-
 		update_stats(&requeued_stats, nrequeued);
 		update_stats(&requeuetime_stats, runtime.tv_usec);
 
@@ -184,7 +185,7 @@ int bench_futex_requeue(int argc, const char **argv,
 		}
 
 		/* everybody should be blocked on futex2, wake'em up */
-		nrequeued = futex_wake(&futex2, nthreads, futex_flag);
+		nrequeued = futex_wake(&futex2, nrequeued, futex_flag);
 		if (nthreads != nrequeued)
 			warnx("couldn't wakeup all tasks (%d/%d)", nrequeued, nthreads);
 
Signature · 1 line
1.9.3
author maintainer
#6 lore raw

PATCH 6/7 perf bench numa: Fixes of --quiet argument

In reply to Arnaldo Carvalho de Melo

From: Petr Holasek <[email protected]>

Corrected description and fixed function of --quiet argument.

---
 tools/perf/bench/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
tools/perf/bench/numa.c +2 -2
index ebfa163b80b5..cd872e9c3a9c 100644
@@ -180,7 +180,7 @@ static const struct option options[] = {
 	OPT_INTEGER('H', "thp"		, &p0.thp,		"MADV_NOHUGEPAGE < 0 < MADV_HUGEPAGE"),
 	OPT_BOOLEAN('c', "show_convergence", &p0.show_convergence, "show convergence details"),
 	OPT_BOOLEAN('m', "measure_convergence",	&p0.measure_convergence, "measure convergence latency"),
-	OPT_BOOLEAN('q', "quiet"	, &p0.show_quiet,	"bzero the initial allocations"),
+	OPT_BOOLEAN('q', "quiet"	, &p0.show_quiet,	"quiet mode"),
 	OPT_BOOLEAN('S', "serialize-startup", &p0.serialize_startup,"serialize thread startup"),
 
 	/* Special option string parsing callbacks: */
@@ -1395,7 +1395,7 @@ static void print_res(const char *name, double val,
 	if (!name)
 		name = "main,";
 
-	if (g->p.show_quiet)
+	if (!g->p.show_quiet)
 		printf(" %-30s %15.3f, %-15s %s\n", name, val, txt_unit, txt_short);
 	else
 		printf(" %14.3f %s\n", val, txt_long);
Signature · 1 line
1.9.3
author maintainer
#7 lore raw

PATCH 7/7 perf bench numa: Fix immediate meeting of convergence condition

In reply to Arnaldo Carvalho de Melo

From: Petr Holasek <[email protected]>

This patch fixes the race in the beginning of benchmark run when some threads hasn't got assigned curr_cpu yet so they don't occur in nodes-of-process stats and benchmark concludes that all remaining threads are converged already.

The race can be reproduced with small amount of threads and some bigger amount of shared process memory, e.g. one process, two threads and 5GB of process memory.

---
 tools/perf/bench/numa.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/perf/bench/numa.c +8 -0
index cd872e9c3a9c..ba5efa4710b5 100644
@@ -828,6 +828,9 @@ static int count_process_nodes(int process_nr)
 		td = g->threads + task_nr;
 
 		node = numa_node_of_cpu(td->curr_cpu);
+		if (node < 0) /* curr_cpu was likely still -1 */
+			return 0;
+
 		node_present[node] = 1;
 	}
 
@@ -882,6 +885,11 @@ static void calc_convergence_compression(int *strong)
 	for (p = 0; p < g->p.nr_proc; p++) {
 		unsigned int nodes = count_process_nodes(p);
 
+		if (!nodes) {
+			*strong = 0;
+			return;
+		}
+
 		nodes_min = min(nodes, nodes_min);
 		nodes_max = max(nodes, nodes_max);
 	}
Signature · 1 line
1.9.3
#8 lore raw
In reply to Arnaldo Carvalho de Melo
Quoted 61 lines · Arnaldo Carvalho de Melo

Hi Ingo,

Please consider pulling, this is on top of my previous 'perf-urgent-for-mingo'
pull request.

- Arnaldo

The following changes since commit de28c15daf60e9625bece22f13a091fac8d05f1d:

tools lib api: Undefine _FORTIFY_SOURCE before setting it (2015-04-23 17:08:23 -0300)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo-2

for you to fetch changes up to 1d90a685eb75a56648d7dd22c704a1a6da516de9:

perf bench numa: Fix immediate meeting of convergence condition (2015-04-27 13:57:50 -0300)

----------------------------------------------------------------
perf/urgent fixes:

User visible:

. Fix a segfault in 'perf top' when kernel map is restricted (Wang Nan)

. Fix hung wakeup tasks after requeueing in 'perf bench futex' (Davidlohr Bueso)

. Fix bug in perf probe global variables handling, missing curly braces on
an if body (He Kuang)

. 'perf bench numa' fixes (command line help/handling, etc) (Petr Holasek)

Build fixes:

. 'perf kmem' on RHEL6/OL6 (David Ahern)

. libtraceevent on 32-bit arch (Namhyung Kim)

----------------------------------------------------------------
David Ahern (1):
perf kmem: Fix compiles on RHEL6/OL6

Davidlohr Bueso (1):
perf bench futex: Fix hung wakeup tasks after requeueing

He Kuang (1):
perf probe: Fix bug with global variables handling

Namhyung Kim (1):
tools lib traceevent: Fix build failure on 32-bit arch

Petr Holasek (2):
perf bench numa: Fixes of --quiet argument
perf bench numa: Fix immediate meeting of convergence condition

Wang Nan (1):
perf top: Fix a segfault when kernel map is restricted.

Quoted 7 lines
 tools/lib/traceevent/event-parse.c |  2 +-
 tools/perf/bench/futex-requeue.c   | 15 ++++++-----
 tools/perf/bench/numa.c            | 12 +++++++--
 tools/perf/builtin-kmem.c          | 54 +++++++++++++++++++-------------------
 tools/perf/builtin-top.c           |  2 +-
 tools/perf/util/probe-finder.c     |  4 ++-
 6 files changed, 50 insertions(+), 39 deletions(-)

Pulled, thanks a lot Arnaldo!

Ingo