[PATCHv2 0/4] perf tools: Fix top crashes

[PATCHv2 0/4] perf tools: Fix top crashes

[Jiri Olsa]

hi,
perf top is occasionaly crashes on hitting assert, because of
the synthesize function that runs on multiple threads now.

I found some issues with list/tree accessing and this patchset
is trying to fix them.

I was runing 'perf top' in a loop with attached change below
and haven't hit any other new crash. We're either clear now
or it's really hard to hit the rest ;-)

v2 changes:
  - remove the 'struct comm_str' only in the comm_str__put
    when the refcnt drops to 0 (Namhyung)

thanks,
jirka

---
Jiri Olsa (4):
      perf tools: Add threads__get_last_match function
      perf tools: Add threads__set_last_match function
      perf tools: Use last_match threads cache only in single thread mode
      perf tools: Fix struct comm_str removal crash

 tools/perf/util/comm.c    | 15 +++++++++------
 tools/perf/util/machine.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 63 insertions(+), 22 deletions(-)
---
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index ffdc2769ff9f..1b57a66ef779 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1102,6 +1102,8 @@ static int __cmd_top(struct perf_top *top)
 	if (top->nr_threads_synthesize > 1)
 		perf_set_singlethreaded();
 
+	return 0;
+
 	if (perf_hpp_list.socket) {
 		ret = perf_env__read_cpu_topology_map(&perf_env);
 		if (ret < 0)

[PATCH 1/4] perf tools: Add threads__get_last_match function

[Jiri Olsa]

Separating threads::last_match cache read/check into
separate threads__get_last_match function. This will
be useful in following patch.

Link: http://lkml.kernel.org/n/tip-z4zzlpp3vusjued0gzp5uycq@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e7b4a8b513f2..7a5236a4ced7 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -408,23 +408,16 @@ static void machine__update_thread_pid(struct machine *machine,
 }
 
 /*
- * Caller must eventually drop thread->refcnt returned with a successful
- * lookup/new thread inserted.
+ * Front-end cache - TID lookups come in blocks,
+ * so most of the time we dont have to look up
+ * the full rbtree:
  */
-static struct thread *____machine__findnew_thread(struct machine *machine,
-						  struct threads *threads,
-						  pid_t pid, pid_t tid,
-						  bool create)
+static struct thread*
+threads__get_last_match(struct threads *threads, struct machine *machine,
+			int pid, int tid)
 {
-	struct rb_node **p = &threads->entries.rb_node;
-	struct rb_node *parent = NULL;
 	struct thread *th;
 
-	/*
-	 * Front-end cache - TID lookups come in blocks,
-	 * so most of the time we dont have to look up
-	 * the full rbtree:
-	 */
 	th = threads->last_match;
 	if (th != NULL) {
 		if (th->tid == tid) {
@@ -435,6 +428,26 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 		threads->last_match = NULL;
 	}
 
+	return NULL;
+}
+
+/*
+ * Caller must eventually drop thread->refcnt returned with a successful
+ * lookup/new thread inserted.
+ */
+static struct thread *____machine__findnew_thread(struct machine *machine,
+						  struct threads *threads,
+						  pid_t pid, pid_t tid,
+						  bool create)
+{
+	struct rb_node **p = &threads->entries.rb_node;
+	struct rb_node *parent = NULL;
+	struct thread *th;
+
+	th = threads__get_last_match(threads, machine, pid, tid);
+	if (th)
+		return th;
+
 	while (*p != NULL) {
 		parent = *p;
 		th = rb_entry(parent, struct thread, rb_node);
-- 
2.17.1

[tip:perf/core] perf machine: Add threads__get_last_match function

[tip-bot for Jiri Olsa]

Commit-ID:  f8b2ebb532e0553b60ae5ad1b84d1d4f0c285752
Gitweb:     https://git.kernel.org/tip/f8b2ebb532e0553b60ae5ad1b84d1d4f0c285752
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 19 Jul 2018 16:33:42 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Jul 2018 14:53:31 -0300

perf machine: Add threads__get_last_match function

Separating threads::last_match cache read/check into separate
threads__get_last_match function. This will be useful in following
patch.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Lukasz Odzioba <lukasz.odzioba@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20180719143345.12963-2-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 22dbb6612b41..df41aa1a4cf9 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -408,23 +408,16 @@ out_err:
 }
 
 /*
- * Caller must eventually drop thread->refcnt returned with a successful
- * lookup/new thread inserted.
+ * Front-end cache - TID lookups come in blocks,
+ * so most of the time we dont have to look up
+ * the full rbtree:
  */
-static struct thread *____machine__findnew_thread(struct machine *machine,
-						  struct threads *threads,
-						  pid_t pid, pid_t tid,
-						  bool create)
+static struct thread*
+threads__get_last_match(struct threads *threads, struct machine *machine,
+			int pid, int tid)
 {
-	struct rb_node **p = &threads->entries.rb_node;
-	struct rb_node *parent = NULL;
 	struct thread *th;
 
-	/*
-	 * Front-end cache - TID lookups come in blocks,
-	 * so most of the time we dont have to look up
-	 * the full rbtree:
-	 */
 	th = threads->last_match;
 	if (th != NULL) {
 		if (th->tid == tid) {
@@ -435,6 +428,26 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 		threads->last_match = NULL;
 	}
 
+	return NULL;
+}
+
+/*
+ * Caller must eventually drop thread->refcnt returned with a successful
+ * lookup/new thread inserted.
+ */
+static struct thread *____machine__findnew_thread(struct machine *machine,
+						  struct threads *threads,
+						  pid_t pid, pid_t tid,
+						  bool create)
+{
+	struct rb_node **p = &threads->entries.rb_node;
+	struct rb_node *parent = NULL;
+	struct thread *th;
+
+	th = threads__get_last_match(threads, machine, pid, tid);
+	if (th)
+		return th;
+
 	while (*p != NULL) {
 		parent = *p;
 		th = rb_entry(parent, struct thread, rb_node);

[PATCH 2/4] perf tools: Add threads__set_last_match function

[Jiri Olsa]

Separating threads::last_match cache set into
separate threads__set_last_match function.
This will be useful in following patch.

Link: http://lkml.kernel.org/n/tip-r2phhluimtb4747rug66mpra@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7a5236a4ced7..090ff42615bb 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -431,6 +431,12 @@ threads__get_last_match(struct threads *threads, struct machine *machine,
 	return NULL;
 }
 
+static void
+threads__set_last_match(struct threads *threads, struct thread *th)
+{
+	threads->last_match = th;
+}
+
 /*
  * Caller must eventually drop thread->refcnt returned with a successful
  * lookup/new thread inserted.
@@ -453,7 +459,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 		th = rb_entry(parent, struct thread, rb_node);
 
 		if (th->tid == tid) {
-			threads->last_match = th;
+			threads__set_last_match(threads, th);
 			machine__update_thread_pid(machine, th, pid);
 			return thread__get(th);
 		}
@@ -490,7 +496,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 		 * It is now in the rbtree, get a ref
 		 */
 		thread__get(th);
-		threads->last_match = th;
+		threads__set_last_match(threads, th);
 		++threads->nr;
 	}
 
@@ -1648,7 +1654,7 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
 	struct threads *threads = machine__threads(machine, th->tid);
 
 	if (threads->last_match == th)
-		threads->last_match = NULL;
+		threads__set_last_match(threads, NULL);
 
 	BUG_ON(refcount_read(&th->refcnt) == 0);
 	if (lock)
-- 
2.17.1

[tip:perf/core] perf machine: Add threads__set_last_match function

[tip-bot for Jiri Olsa]

Commit-ID:  67fda0f32cd9428cb9a3166796162097d7fcbcbf
Gitweb:     https://git.kernel.org/tip/67fda0f32cd9428cb9a3166796162097d7fcbcbf
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 19 Jul 2018 16:33:43 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Jul 2018 14:53:42 -0300

perf machine: Add threads__set_last_match function

Separating threads::last_match cache set into separate
threads__set_last_match function.  This will be useful in following
patch.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Lukasz Odzioba <lukasz.odzioba@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20180719143345.12963-3-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index df41aa1a4cf9..8992fcf42257 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -431,6 +431,12 @@ threads__get_last_match(struct threads *threads, struct machine *machine,
 	return NULL;
 }
 
+static void
+threads__set_last_match(struct threads *threads, struct thread *th)
+{
+	threads->last_match = th;
+}
+
 /*
  * Caller must eventually drop thread->refcnt returned with a successful
  * lookup/new thread inserted.
@@ -453,7 +459,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 		th = rb_entry(parent, struct thread, rb_node);
 
 		if (th->tid == tid) {
-			threads->last_match = th;
+			threads__set_last_match(threads, th);
 			machine__update_thread_pid(machine, th, pid);
 			return thread__get(th);
 		}
@@ -490,7 +496,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
 		 * It is now in the rbtree, get a ref
 		 */
 		thread__get(th);
-		threads->last_match = th;
+		threads__set_last_match(threads, th);
 		++threads->nr;
 	}
 
@@ -1648,7 +1654,7 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
 	struct threads *threads = machine__threads(machine, th->tid);
 
 	if (threads->last_match == th)
-		threads->last_match = NULL;
+		threads__set_last_match(threads, NULL);
 
 	BUG_ON(refcount_read(&th->refcnt) == 0);
 	if (lock)

[PATCH 3/4] perf tools: Use last_match threads cache only in single thread mode

[Jiri Olsa]

There's an issue with using threads::last_match in multithread
mode which is enabled during the perf top synthesize. It might
crash with following assertion:

  perf: ...include/linux/refcount.h:109: refcount_inc:
        Assertion `!(!refcount_inc_not_zero(r))' failed.

The gdb backtrace looks like this:

  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  (gdb)
  #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
  #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
  #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
  #4  0x0000000000535ff9 in refcount_inc (r=0x7fffe8009a70)
      at ...include/linux/refcount.h:109
  #5  0x0000000000536771 in thread__get (thread=0x7fffe8009a40)
      at util/thread.c:115
  #6  0x0000000000523cd0 in ____machine__findnew_thread (machine=0xbfde38,
      threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:432
  #7  0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
      pid=2, tid=2) at util/machine.c:489
  #8  0x0000000000523f24 in machine__findnew_thread (machine=0xbfde38,
      pid=2, tid=2) at util/machine.c:499
  #9  0x0000000000526fbe in machine__process_fork_event (machine=0xbfde38,
  ...

The failing assertion is this one:

  REFCOUNT_WARN(!refcount_inc_not_zero(r), ...

the problem is that we don't serialize access to threads::last_match.
We serialize the access to the threads tree, but we don't care how's
threads::last_match being accessed. Both locked/unlocked paths use
that data and can set it. In multithreaded mode we can end up with
invalid object in thread__get call, like in following paths race:

  thread 1
    ...
    machine__findnew_thread
      down_write(&threads->lock);
      __machine__findnew_thread
        ____machine__findnew_thread
          th = threads->last_match;
          if (th->tid == tid) {
            thread__get

  thread 2
    ...
    machine__find_thread
      down_read(&threads->lock);
      __machine__findnew_thread
        ____machine__findnew_thread
          th = threads->last_match;
          if (th->tid == tid) {
            thread__get

  thread 3
    ...
    machine__process_fork_event
      machine__remove_thread
        __machine__remove_thread
          threads->last_match = NULL
          thread__put
      thread__put

Thread 1 and 2 might got stale last_match, before thread 3 clears
it. Thread 1 and 2 then race with thread 3's thread__put and they
might trigger the refcnt == 0 assertion above.

The patch is disabling the last_match cache for multiple thread
mode. It was originally meant for single thread scenarios, where
it's common to have multiple sequential searches of the same
thread.

In multithread mode this does not make sense, because top's threads
processes different /proc entries and so the 'struct threads' object
is queried for various threads. Moreover we'd need to add more locks
to make it work.

Link: http://lkml.kernel.org/n/tip-rq60pig3u8kx9kbzdckbyby0@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 090ff42615bb..175f963f5729 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -413,8 +413,8 @@ static void machine__update_thread_pid(struct machine *machine,
  * the full rbtree:
  */
 static struct thread*
-threads__get_last_match(struct threads *threads, struct machine *machine,
-			int pid, int tid)
+__threads__get_last_match(struct threads *threads, struct machine *machine,
+			  int pid, int tid)
 {
 	struct thread *th;
 
@@ -431,12 +431,31 @@ threads__get_last_match(struct threads *threads, struct machine *machine,
 	return NULL;
 }
 
+static struct thread*
+threads__get_last_match(struct threads *threads, struct machine *machine,
+			int pid, int tid)
+{
+	struct thread *th = NULL;
+
+	if (perf_singlethreaded)
+		th = __threads__get_last_match(threads, machine, pid, tid);
+
+	return th;
+}
+
 static void
-threads__set_last_match(struct threads *threads, struct thread *th)
+__threads__set_last_match(struct threads *threads, struct thread *th)
 {
 	threads->last_match = th;
 }
 
+static void
+threads__set_last_match(struct threads *threads, struct thread *th)
+{
+	if (perf_singlethreaded)
+		__threads__set_last_match(threads, th);
+}
+
 /*
  * Caller must eventually drop thread->refcnt returned with a successful
  * lookup/new thread inserted.
-- 
2.17.1

[tip:perf/core] perf machine: Use last_match threads cache only in single thread mode

[tip-bot for Jiri Olsa]

Commit-ID:  b57334b9453949bf81281321d14d86d60aee6fde
Gitweb:     https://git.kernel.org/tip/b57334b9453949bf81281321d14d86d60aee6fde
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 19 Jul 2018 16:33:44 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Jul 2018 14:53:52 -0300

perf machine: Use last_match threads cache only in single thread mode

There's an issue with using threads::last_match in multithread mode
which is enabled during the perf top synthesize. It might crash with
following assertion:

  perf: ...include/linux/refcount.h:109: refcount_inc:
        Assertion `!(!refcount_inc_not_zero(r))' failed.

The gdb backtrace looks like this:

  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  (gdb)
  #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
  #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
  #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
  #4  0x0000000000535ff9 in refcount_inc (r=0x7fffe8009a70)
      at ...include/linux/refcount.h:109
  #5  0x0000000000536771 in thread__get (thread=0x7fffe8009a40)
      at util/thread.c:115
  #6  0x0000000000523cd0 in ____machine__findnew_thread (machine=0xbfde38,
      threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:432
  #7  0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
      pid=2, tid=2) at util/machine.c:489
  #8  0x0000000000523f24 in machine__findnew_thread (machine=0xbfde38,
      pid=2, tid=2) at util/machine.c:499
  #9  0x0000000000526fbe in machine__process_fork_event (machine=0xbfde38,
  ...

The failing assertion is this one:

  REFCOUNT_WARN(!refcount_inc_not_zero(r), ...

the problem is that we don't serialize access to threads::last_match.
We serialize the access to the threads tree, but we don't care how's
threads::last_match being accessed. Both locked/unlocked paths use
that data and can set it. In multithreaded mode we can end up with
invalid object in thread__get call, like in following paths race:

  thread 1
    ...
    machine__findnew_thread
      down_write(&threads->lock);
      __machine__findnew_thread
        ____machine__findnew_thread
          th = threads->last_match;
          if (th->tid == tid) {
            thread__get

  thread 2
    ...
    machine__find_thread
      down_read(&threads->lock);
      __machine__findnew_thread
        ____machine__findnew_thread
          th = threads->last_match;
          if (th->tid == tid) {
            thread__get

  thread 3
    ...
    machine__process_fork_event
      machine__remove_thread
        __machine__remove_thread
          threads->last_match = NULL
          thread__put
      thread__put

Thread 1 and 2 might got stale last_match, before thread 3 clears
it. Thread 1 and 2 then race with thread 3's thread__put and they
might trigger the refcnt == 0 assertion above.

The patch is disabling the last_match cache for multiple thread
mode. It was originally meant for single thread scenarios, where
it's common to have multiple sequential searches of the same
thread.

In multithread mode this does not make sense, because top's threads
processes different /proc entries and so the 'struct threads' object
is queried for various threads. Moreover we'd need to add more locks
to make it work.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Lukasz Odzioba <lukasz.odzioba@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20180719143345.12963-4-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 8992fcf42257..b300a3973448 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -413,8 +413,8 @@ out_err:
  * the full rbtree:
  */
 static struct thread*
-threads__get_last_match(struct threads *threads, struct machine *machine,
-			int pid, int tid)
+__threads__get_last_match(struct threads *threads, struct machine *machine,
+			  int pid, int tid)
 {
 	struct thread *th;
 
@@ -431,12 +431,31 @@ threads__get_last_match(struct threads *threads, struct machine *machine,
 	return NULL;
 }
 
+static struct thread*
+threads__get_last_match(struct threads *threads, struct machine *machine,
+			int pid, int tid)
+{
+	struct thread *th = NULL;
+
+	if (perf_singlethreaded)
+		th = __threads__get_last_match(threads, machine, pid, tid);
+
+	return th;
+}
+
 static void
-threads__set_last_match(struct threads *threads, struct thread *th)
+__threads__set_last_match(struct threads *threads, struct thread *th)
 {
 	threads->last_match = th;
 }
 
+static void
+threads__set_last_match(struct threads *threads, struct thread *th)
+{
+	if (perf_singlethreaded)
+		__threads__set_last_match(threads, th);
+}
+
 /*
  * Caller must eventually drop thread->refcnt returned with a successful
  * lookup/new thread inserted.

[PATCH 4/4] perf tools: Fix struct comm_str removal crash

[Jiri Olsa]

We occasionaly hit following assert failure in perf top,
when processing the /proc info in multiple threads.

  perf: ...include/linux/refcount.h:109: refcount_inc:
        Assertion `!(!refcount_inc_not_zero(r))' failed.

The gdb backtrace looks like this:

  [Switching to Thread 0x7ffff11ba700 (LWP 13749)]
  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  (gdb)
  #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
  #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
  #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
  #4  0x0000000000535373 in refcount_inc (r=0x7fffdc009be0)
      at ...include/linux/refcount.h:109
  #5  0x00000000005354f1 in comm_str__get (cs=0x7fffdc009bc0)
      at util/comm.c:24
  #6  0x00000000005356bd in __comm_str__findnew (str=0x7fffd000b260 ":2",
      root=0xbed5c0 <comm_str_root>) at util/comm.c:72
  #7  0x000000000053579e in comm_str__findnew (str=0x7fffd000b260 ":2",
      root=0xbed5c0 <comm_str_root>) at util/comm.c:95
  #8  0x000000000053582e in comm__new (str=0x7fffd000b260 ":2",
      timestamp=0, exec=false) at util/comm.c:111
  #9  0x00000000005363bc in thread__new (pid=2, tid=2) at util/thread.c:57
  #10 0x0000000000523da0 in ____machine__findnew_thread (machine=0xbfde38,
      threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:457
  #11 0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
  ...

The failing assertion is this one:

  REFCOUNT_WARN(!refcount_inc_not_zero(r), ...

The problem is that we keep global comm_str_root list, which
is accessed by multiple threads during the perf top startup
and following 2 paths can race:

  thread 1:
    ...
    thread__new
      comm__new
        comm_str__findnew
          down_write(&comm_str_lock);
          __comm_str__findnew
            comm_str__get

  thread 2:
    ...
    comm__override or comm__free
      comm_str__put
        refcount_dec_and_test
          down_write(&comm_str_lock);
          rb_erase(&cs->rb_node, &comm_str_root);

Because thread 2 first decrements the refcnt and only after then it
removes the struct comm_str from the list, the thread 1 can find this
object on the list with refcnt equls to 0 and hit the assert.

This patch fixes the thread 1 __comm_str__findnew path, by ignoring
objects that already dropped the refcnt to 0. For the rest of the
objects we take the refcnt before comparing its name and release
it afterwards with comm_str__put, which can also release the object
completely.

Link: http://lkml.kernel.org/n/tip-vrizt6sw1lu1ybsrl9l0wwln@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/comm.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 7798a2cc8a86..9c4a18991e41 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -18,11 +18,9 @@ struct comm_str {
 static struct rb_root comm_str_root;
 static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
 
-static struct comm_str *comm_str__get(struct comm_str *cs)
+static bool comm_str__get(struct comm_str *cs)
 {
-	if (cs)
-		refcount_inc(&cs->refcnt);
-	return cs;
+	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
 }
 
 static void comm_str__put(struct comm_str *cs)
@@ -67,9 +65,14 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
 		parent = *p;
 		iter = rb_entry(parent, struct comm_str, rb_node);
 
+		/*
+		 * If we race with comm_str__put, iter->refcnt is 0
+		 * and it will be removed within comm_str__put call
+		 * shortly, ignore it in this search.
+		 */
 		cmp = strcmp(str, iter->str);
-		if (!cmp)
-			return comm_str__get(iter);
+		if (!cmp && comm_str__get(iter))
+			return iter;
 
 		if (cmp < 0)
 			p = &(*p)->rb_left;
-- 
2.17.1

Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash

[Arnaldo Carvalho de Melo]

Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> We occasionaly hit following assert failure in perf top,
> when processing the /proc info in multiple threads.

Namhyung, are you ok with this one?

- Arnaldo
 
>   perf: ...include/linux/refcount.h:109: refcount_inc:
>         Assertion `!(!refcount_inc_not_zero(r))' failed.
> 
> The gdb backtrace looks like this:
> 
>   [Switching to Thread 0x7ffff11ba700 (LWP 13749)]
>   0x00007ffff50839fb in raise () from /lib64/libc.so.6
>   (gdb)
>   #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
>   #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
>   #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
>   #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
>   #4  0x0000000000535373 in refcount_inc (r=0x7fffdc009be0)
>       at ...include/linux/refcount.h:109
>   #5  0x00000000005354f1 in comm_str__get (cs=0x7fffdc009bc0)
>       at util/comm.c:24
>   #6  0x00000000005356bd in __comm_str__findnew (str=0x7fffd000b260 ":2",
>       root=0xbed5c0 <comm_str_root>) at util/comm.c:72
>   #7  0x000000000053579e in comm_str__findnew (str=0x7fffd000b260 ":2",
>       root=0xbed5c0 <comm_str_root>) at util/comm.c:95
>   #8  0x000000000053582e in comm__new (str=0x7fffd000b260 ":2",
>       timestamp=0, exec=false) at util/comm.c:111
>   #9  0x00000000005363bc in thread__new (pid=2, tid=2) at util/thread.c:57
>   #10 0x0000000000523da0 in ____machine__findnew_thread (machine=0xbfde38,
>       threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:457
>   #11 0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
>   ...
> 
> The failing assertion is this one:
> 
>   REFCOUNT_WARN(!refcount_inc_not_zero(r), ...
> 
> The problem is that we keep global comm_str_root list, which
> is accessed by multiple threads during the perf top startup
> and following 2 paths can race:
> 
>   thread 1:
>     ...
>     thread__new
>       comm__new
>         comm_str__findnew
>           down_write(&comm_str_lock);
>           __comm_str__findnew
>             comm_str__get
> 
>   thread 2:
>     ...
>     comm__override or comm__free
>       comm_str__put
>         refcount_dec_and_test
>           down_write(&comm_str_lock);
>           rb_erase(&cs->rb_node, &comm_str_root);
> 
> Because thread 2 first decrements the refcnt and only after then it
> removes the struct comm_str from the list, the thread 1 can find this
> object on the list with refcnt equls to 0 and hit the assert.
> 
> This patch fixes the thread 1 __comm_str__findnew path, by ignoring
> objects that already dropped the refcnt to 0. For the rest of the
> objects we take the refcnt before comparing its name and release
> it afterwards with comm_str__put, which can also release the object
> completely.
> 
> Link: http://lkml.kernel.org/n/tip-vrizt6sw1lu1ybsrl9l0wwln@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/comm.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> index 7798a2cc8a86..9c4a18991e41 100644
> --- a/tools/perf/util/comm.c
> +++ b/tools/perf/util/comm.c
> @@ -18,11 +18,9 @@ struct comm_str {
>  static struct rb_root comm_str_root;
>  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
>  
> -static struct comm_str *comm_str__get(struct comm_str *cs)
> +static bool comm_str__get(struct comm_str *cs)
>  {
> -	if (cs)
> -		refcount_inc(&cs->refcnt);
> -	return cs;
> +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
>  }
>  
>  static void comm_str__put(struct comm_str *cs)
> @@ -67,9 +65,14 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
>  		parent = *p;
>  		iter = rb_entry(parent, struct comm_str, rb_node);
>  
> +		/*
> +		 * If we race with comm_str__put, iter->refcnt is 0
> +		 * and it will be removed within comm_str__put call
> +		 * shortly, ignore it in this search.
> +		 */
>  		cmp = strcmp(str, iter->str);
> -		if (!cmp)
> -			return comm_str__get(iter);
> +		if (!cmp && comm_str__get(iter))
> +			return iter;
>  
>  		if (cmp < 0)
>  			p = &(*p)->rb_left;
> -- 
> 2.17.1

Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash

[Arnaldo Carvalho de Melo]

Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> We occasionaly hit following assert failure in perf top,
> when processing the /proc info in multiple threads.
> 
>   perf: ...include/linux/refcount.h:109: refcount_inc:
>         Assertion `!(!refcount_inc_not_zero(r))' failed.
> 
> The gdb backtrace looks like this:
> 
>   [Switching to Thread 0x7ffff11ba700 (LWP 13749)]
>   0x00007ffff50839fb in raise () from /lib64/libc.so.6
>   (gdb)
>   #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
>   #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
>   #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
>   #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
>   #4  0x0000000000535373 in refcount_inc (r=0x7fffdc009be0)
>       at ...include/linux/refcount.h:109
>   #5  0x00000000005354f1 in comm_str__get (cs=0x7fffdc009bc0)
>       at util/comm.c:24
>   #6  0x00000000005356bd in __comm_str__findnew (str=0x7fffd000b260 ":2",
>       root=0xbed5c0 <comm_str_root>) at util/comm.c:72
>   #7  0x000000000053579e in comm_str__findnew (str=0x7fffd000b260 ":2",
>       root=0xbed5c0 <comm_str_root>) at util/comm.c:95
>   #8  0x000000000053582e in comm__new (str=0x7fffd000b260 ":2",
>       timestamp=0, exec=false) at util/comm.c:111
>   #9  0x00000000005363bc in thread__new (pid=2, tid=2) at util/thread.c:57
>   #10 0x0000000000523da0 in ____machine__findnew_thread (machine=0xbfde38,
>       threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:457
>   #11 0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
>   ...
> 
> The failing assertion is this one:
> 
>   REFCOUNT_WARN(!refcount_inc_not_zero(r), ...
> 
> The problem is that we keep global comm_str_root list, which
> is accessed by multiple threads during the perf top startup
> and following 2 paths can race:
> 
>   thread 1:
>     ...
>     thread__new
>       comm__new
>         comm_str__findnew
>           down_write(&comm_str_lock);
>           __comm_str__findnew
>             comm_str__get
> 
>   thread 2:
>     ...
>     comm__override or comm__free
>       comm_str__put
>         refcount_dec_and_test
>           down_write(&comm_str_lock);
>           rb_erase(&cs->rb_node, &comm_str_root);
> 
> Because thread 2 first decrements the refcnt and only after then it
> removes the struct comm_str from the list, the thread 1 can find this
> object on the list with refcnt equls to 0 and hit the assert.
> 
> This patch fixes the thread 1 __comm_str__findnew path, by ignoring
> objects that already dropped the refcnt to 0. For the rest of the
> objects we take the refcnt before comparing its name and release
> it afterwards with comm_str__put, which can also release the object
> completely.
> 
> Link: http://lkml.kernel.org/n/tip-vrizt6sw1lu1ybsrl9l0wwln@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/comm.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> index 7798a2cc8a86..9c4a18991e41 100644
> --- a/tools/perf/util/comm.c
> +++ b/tools/perf/util/comm.c
> @@ -18,11 +18,9 @@ struct comm_str {
>  static struct rb_root comm_str_root;
>  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
>  
> -static struct comm_str *comm_str__get(struct comm_str *cs)
> +static bool comm_str__get(struct comm_str *cs)
>  {
> -	if (cs)
> -		refcount_inc(&cs->refcnt);
> -	return cs;
> +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
>  }

I don't like changing the semantics of a __get() operation this way, I
think it should stay like all the others, i.e. return the object with
the desired refcount or return NULL if that is not possible.

Otherwise we'll have to switch gears when debugging refcounts in various
objects, that start having slightly different semantics for reference
counting.

We should try to find a fix that maintains the semantics of refcounting.

>  static void comm_str__put(struct comm_str *cs)
> @@ -67,9 +65,14 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
>  		parent = *p;
>  		iter = rb_entry(parent, struct comm_str, rb_node);
>  
> +		/*
> +		 * If we race with comm_str__put, iter->refcnt is 0
> +		 * and it will be removed within comm_str__put call
> +		 * shortly, ignore it in this search.
> +		 */
>  		cmp = strcmp(str, iter->str);
> -		if (!cmp)
> -			return comm_str__get(iter);
> +		if (!cmp && comm_str__get(iter))
> +			return iter;
>  
>  		if (cmp < 0)
>  			p = &(*p)->rb_left;
> -- 
> 2.17.1

Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash

[Arnaldo Carvalho de Melo]

Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> > +++ b/tools/perf/util/comm.c
> > @@ -18,11 +18,9 @@ struct comm_str {
> >  static struct rb_root comm_str_root;
> >  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> >  
> > -static struct comm_str *comm_str__get(struct comm_str *cs)
> > +static bool comm_str__get(struct comm_str *cs)
> >  {
> > -	if (cs)
> > -		refcount_inc(&cs->refcnt);
> > -	return cs;
> > +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
> >  }
> 
> I don't like changing the semantics of a __get() operation this way, I
> think it should stay like all the others, i.e. return the object with
> the desired refcount or return NULL if that is not possible.
> 
> Otherwise we'll have to switch gears when debugging refcounts in various
> objects, that start having slightly different semantics for reference
> counting.
> 
> We should try to find a fix that maintains the semantics of refcounting.

After looking at the code, this refcount_inc_not_zero returns bool comes
from the kernel, trying to see how this is used with __get() operations
there, if at all.

- Arnaldo

Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash

[Namhyung Kim]

Hi Arnaldo,

On Thu, Jul 19, 2018 at 03:31:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> > > +++ b/tools/perf/util/comm.c
> > > @@ -18,11 +18,9 @@ struct comm_str {
> > >  static struct rb_root comm_str_root;
> > >  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > >  
> > > -static struct comm_str *comm_str__get(struct comm_str *cs)
> > > +static bool comm_str__get(struct comm_str *cs)
> > >  {
> > > -	if (cs)
> > > -		refcount_inc(&cs->refcnt);
> > > -	return cs;
> > > +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
> > >  }
> > 
> > I don't like changing the semantics of a __get() operation this way, I
> > think it should stay like all the others, i.e. return the object with
> > the desired refcount or return NULL if that is not possible.
> > 
> > Otherwise we'll have to switch gears when debugging refcounts in various
> > objects, that start having slightly different semantics for reference
> > counting.
> > 
> > We should try to find a fix that maintains the semantics of refcounting.
> 
> After looking at the code, this refcount_inc_not_zero returns bool comes
> from the kernel, trying to see how this is used with __get() operations
> there, if at all.

Something like this?

static struct comm_str *comm_str__get(struct comm_str *cs)
{
    if (cs && refcount_inc_not_zero(&cs->refcnt))
        return cs;
    return NULL;
}


Other than that I don't have better idea, so

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

[PATCHv3 4/4] perf tools: Fix struct comm_str removal crash

[Jiri Olsa]

On Fri, Jul 20, 2018 at 10:20:55AM +0900, Namhyung Kim wrote:
> Hi Arnaldo,
> 
> On Thu, Jul 19, 2018 at 03:31:14PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> > > > +++ b/tools/perf/util/comm.c
> > > > @@ -18,11 +18,9 @@ struct comm_str {
> > > >  static struct rb_root comm_str_root;
> > > >  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > > >  
> > > > -static struct comm_str *comm_str__get(struct comm_str *cs)
> > > > +static bool comm_str__get(struct comm_str *cs)
> > > >  {
> > > > -	if (cs)
> > > > -		refcount_inc(&cs->refcnt);
> > > > -	return cs;
> > > > +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
> > > >  }
> > > 
> > > I don't like changing the semantics of a __get() operation this way, I
> > > think it should stay like all the others, i.e. return the object with
> > > the desired refcount or return NULL if that is not possible.
> > > 
> > > Otherwise we'll have to switch gears when debugging refcounts in various
> > > objects, that start having slightly different semantics for reference
> > > counting.
> > > 
> > > We should try to find a fix that maintains the semantics of refcounting.
> > 
> > After looking at the code, this refcount_inc_not_zero returns bool comes
> > from the kernel, trying to see how this is used with __get() operations
> > there, if at all.
> 
> Something like this?
> 
> static struct comm_str *comm_str__get(struct comm_str *cs)
> {
>     if (cs && refcount_inc_not_zero(&cs->refcnt))
>         return cs;
>     return NULL;
> }
> 
> 
> Other than that I don't have better idea, so
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> 
> Thanks,
> Namhyung

righ, we can change comm_str__get like that, attached v3

thanks,
jirka


---
We occasionaly hit following assert failure in perf top,
when processing the /proc info in multiple threads.

  perf: ...include/linux/refcount.h:109: refcount_inc:
        Assertion `!(!refcount_inc_not_zero(r))' failed.

The gdb backtrace looks like this:

  [Switching to Thread 0x7ffff11ba700 (LWP 13749)]
  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  (gdb)
  #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
  #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
  #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
  #4  0x0000000000535373 in refcount_inc (r=0x7fffdc009be0)
      at ...include/linux/refcount.h:109
  #5  0x00000000005354f1 in comm_str__get (cs=0x7fffdc009bc0)
      at util/comm.c:24
  #6  0x00000000005356bd in __comm_str__findnew (str=0x7fffd000b260 ":2",
      root=0xbed5c0 <comm_str_root>) at util/comm.c:72
  #7  0x000000000053579e in comm_str__findnew (str=0x7fffd000b260 ":2",
      root=0xbed5c0 <comm_str_root>) at util/comm.c:95
  #8  0x000000000053582e in comm__new (str=0x7fffd000b260 ":2",
      timestamp=0, exec=false) at util/comm.c:111
  #9  0x00000000005363bc in thread__new (pid=2, tid=2) at util/thread.c:57
  #10 0x0000000000523da0 in ____machine__findnew_thread (machine=0xbfde38,
      threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:457
  #11 0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
  ...

The failing assertion is this one:

  REFCOUNT_WARN(!refcount_inc_not_zero(r), ...

The problem is that we keep global comm_str_root list, which
is accessed by multiple threads during the perf top startup
and following 2 paths can race:

  thread 1:
    ...
    thread__new
      comm__new
        comm_str__findnew
          down_write(&comm_str_lock);
          __comm_str__findnew
            comm_str__get

  thread 2:
    ...
    comm__override or comm__free
      comm_str__put
        refcount_dec_and_test
          down_write(&comm_str_lock);
          rb_erase(&cs->rb_node, &comm_str_root);

Because thread 2 first decrements the refcnt and only after then it
removes the struct comm_str from the list, the thread 1 can find this
object on the list with refcnt equls to 0 and hit the assert.

This patch fixes the thread 1 __comm_str__findnew path, by ignoring
objects that already dropped the refcnt to 0. For the rest of the
objects we take the refcnt before comparing its name and release
it afterwards with comm_str__put, which can also release the object
completely.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/n/tip-vrizt6sw1lu1ybsrl9l0wwln@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/comm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 7798a2cc8a86..31279a7bd919 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -20,9 +20,10 @@ static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,}
 
 static struct comm_str *comm_str__get(struct comm_str *cs)
 {
-	if (cs)
-		refcount_inc(&cs->refcnt);
-	return cs;
+	if (cs && refcount_inc_not_zero(&cs->refcnt))
+		return cs;
+
+	return NULL;
 }
 
 static void comm_str__put(struct comm_str *cs)
@@ -67,9 +68,14 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
 		parent = *p;
 		iter = rb_entry(parent, struct comm_str, rb_node);
 
+		/*
+		 * If we race with comm_str__put, iter->refcnt is 0
+		 * and it will be removed within comm_str__put call
+		 * shortly, ignore it in this search.
+		 */
 		cmp = strcmp(str, iter->str);
-		if (!cmp)
-			return comm_str__get(iter);
+		if (!cmp && comm_str__get(iter))
+			return iter;
 
 		if (cmp < 0)
 			p = &(*p)->rb_left;
-- 
2.17.1

Re: [PATCHv3 4/4] perf tools: Fix struct comm_str removal crash

[Arnaldo Carvalho de Melo]

Em Fri, Jul 20, 2018 at 12:17:40PM +0200, Jiri Olsa escreveu:
> On Fri, Jul 20, 2018 at 10:20:55AM +0900, Namhyung Kim wrote:
> > Hi Arnaldo,
> > 
> > On Thu, Jul 19, 2018 at 03:31:14PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> > > > > +++ b/tools/perf/util/comm.c
> > > > > @@ -18,11 +18,9 @@ struct comm_str {
> > > > >  static struct rb_root comm_str_root;
> > > > >  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > > > >  
> > > > > -static struct comm_str *comm_str__get(struct comm_str *cs)
> > > > > +static bool comm_str__get(struct comm_str *cs)
> > > > >  {
> > > > > -	if (cs)
> > > > > -		refcount_inc(&cs->refcnt);
> > > > > -	return cs;
> > > > > +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
> > > > >  }
> > > > 
> > > > I don't like changing the semantics of a __get() operation this way, I
> > > > think it should stay like all the others, i.e. return the object with
> > > > the desired refcount or return NULL if that is not possible.
> > > > 
> > > > Otherwise we'll have to switch gears when debugging refcounts in various
> > > > objects, that start having slightly different semantics for reference
> > > > counting.
> > > > 
> > > > We should try to find a fix that maintains the semantics of refcounting.
> > > 
> > > After looking at the code, this refcount_inc_not_zero returns bool comes
> > > from the kernel, trying to see how this is used with __get() operations
> > > there, if at all.
> > 
> > Something like this?
> > 
> > static struct comm_str *comm_str__get(struct comm_str *cs)
> > {
> >     if (cs && refcount_inc_not_zero(&cs->refcnt))
> >         return cs;
> >     return NULL;
> > }
> > 
> > 
> > Other than that I don't have better idea, so
> > 
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > 
> > Thanks,
> > Namhyung
> 
> righ, we can change comm_str__get like that, attached v3

Thanks, glad it was so easy. :-)

- Arnaldo
 
> thanks,
> jirka
> 
> 
> ---
> We occasionaly hit following assert failure in perf top,
> when processing the /proc info in multiple threads.
> 
>   perf: ...include/linux/refcount.h:109: refcount_inc:
>         Assertion `!(!refcount_inc_not_zero(r))' failed.
> 
> The gdb backtrace looks like this:
> 
>   [Switching to Thread 0x7ffff11ba700 (LWP 13749)]
>   0x00007ffff50839fb in raise () from /lib64/libc.so.6
>   (gdb)
>   #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
>   #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
>   #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
>   #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
>   #4  0x0000000000535373 in refcount_inc (r=0x7fffdc009be0)
>       at ...include/linux/refcount.h:109
>   #5  0x00000000005354f1 in comm_str__get (cs=0x7fffdc009bc0)
>       at util/comm.c:24
>   #6  0x00000000005356bd in __comm_str__findnew (str=0x7fffd000b260 ":2",
>       root=0xbed5c0 <comm_str_root>) at util/comm.c:72
>   #7  0x000000000053579e in comm_str__findnew (str=0x7fffd000b260 ":2",
>       root=0xbed5c0 <comm_str_root>) at util/comm.c:95
>   #8  0x000000000053582e in comm__new (str=0x7fffd000b260 ":2",
>       timestamp=0, exec=false) at util/comm.c:111
>   #9  0x00000000005363bc in thread__new (pid=2, tid=2) at util/thread.c:57
>   #10 0x0000000000523da0 in ____machine__findnew_thread (machine=0xbfde38,
>       threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:457
>   #11 0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
>   ...
> 
> The failing assertion is this one:
> 
>   REFCOUNT_WARN(!refcount_inc_not_zero(r), ...
> 
> The problem is that we keep global comm_str_root list, which
> is accessed by multiple threads during the perf top startup
> and following 2 paths can race:
> 
>   thread 1:
>     ...
>     thread__new
>       comm__new
>         comm_str__findnew
>           down_write(&comm_str_lock);
>           __comm_str__findnew
>             comm_str__get
> 
>   thread 2:
>     ...
>     comm__override or comm__free
>       comm_str__put
>         refcount_dec_and_test
>           down_write(&comm_str_lock);
>           rb_erase(&cs->rb_node, &comm_str_root);
> 
> Because thread 2 first decrements the refcnt and only after then it
> removes the struct comm_str from the list, the thread 1 can find this
> object on the list with refcnt equls to 0 and hit the assert.
> 
> This patch fixes the thread 1 __comm_str__findnew path, by ignoring
> objects that already dropped the refcnt to 0. For the rest of the
> objects we take the refcnt before comparing its name and release
> it afterwards with comm_str__put, which can also release the object
> completely.
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Link: http://lkml.kernel.org/n/tip-vrizt6sw1lu1ybsrl9l0wwln@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/comm.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> index 7798a2cc8a86..31279a7bd919 100644
> --- a/tools/perf/util/comm.c
> +++ b/tools/perf/util/comm.c
> @@ -20,9 +20,10 @@ static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,}
>  
>  static struct comm_str *comm_str__get(struct comm_str *cs)
>  {
> -	if (cs)
> -		refcount_inc(&cs->refcnt);
> -	return cs;
> +	if (cs && refcount_inc_not_zero(&cs->refcnt))
> +		return cs;
> +
> +	return NULL;
>  }
>  
>  static void comm_str__put(struct comm_str *cs)
> @@ -67,9 +68,14 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
>  		parent = *p;
>  		iter = rb_entry(parent, struct comm_str, rb_node);
>  
> +		/*
> +		 * If we race with comm_str__put, iter->refcnt is 0
> +		 * and it will be removed within comm_str__put call
> +		 * shortly, ignore it in this search.
> +		 */
>  		cmp = strcmp(str, iter->str);
> -		if (!cmp)
> -			return comm_str__get(iter);
> +		if (!cmp && comm_str__get(iter))
> +			return iter;
>  
>  		if (cmp < 0)
>  			p = &(*p)->rb_left;
> -- 
> 2.17.1

[tip:perf/core] perf tools: Fix struct comm_str removal crash

[tip-bot for Jiri Olsa]

Commit-ID:  46b3722cc7765582354488da633aafffcb138458
Gitweb:     https://git.kernel.org/tip/46b3722cc7765582354488da633aafffcb138458
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Fri, 20 Jul 2018 12:17:40 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Jul 2018 14:54:03 -0300

perf tools: Fix struct comm_str removal crash

We occasionaly hit following assert failure in 'perf top', when processing the
/proc info in multiple threads.

  perf: ...include/linux/refcount.h:109: refcount_inc:
        Assertion `!(!refcount_inc_not_zero(r))' failed.

The gdb backtrace looks like this:

  [Switching to Thread 0x7ffff11ba700 (LWP 13749)]
  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  (gdb)
  #0  0x00007ffff50839fb in raise () from /lib64/libc.so.6
  #1  0x00007ffff5085800 in abort () from /lib64/libc.so.6
  #2  0x00007ffff507c0da in __assert_fail_base () from /lib64/libc.so.6
  #3  0x00007ffff507c152 in __assert_fail () from /lib64/libc.so.6
  #4  0x0000000000535373 in refcount_inc (r=0x7fffdc009be0)
      at ...include/linux/refcount.h:109
  #5  0x00000000005354f1 in comm_str__get (cs=0x7fffdc009bc0)
      at util/comm.c:24
  #6  0x00000000005356bd in __comm_str__findnew (str=0x7fffd000b260 ":2",
      root=0xbed5c0 <comm_str_root>) at util/comm.c:72
  #7  0x000000000053579e in comm_str__findnew (str=0x7fffd000b260 ":2",
      root=0xbed5c0 <comm_str_root>) at util/comm.c:95
  #8  0x000000000053582e in comm__new (str=0x7fffd000b260 ":2",
      timestamp=0, exec=false) at util/comm.c:111
  #9  0x00000000005363bc in thread__new (pid=2, tid=2) at util/thread.c:57
  #10 0x0000000000523da0 in ____machine__findnew_thread (machine=0xbfde38,
      threads=0xbfdf28, pid=2, tid=2, create=true) at util/machine.c:457
  #11 0x0000000000523eb4 in __machine__findnew_thread (machine=0xbfde38,
  ...

The failing assertion is this one:

  REFCOUNT_WARN(!refcount_inc_not_zero(r), ...

The problem is that we keep global comm_str_root list, which
is accessed by multiple threads during the 'perf top' startup
and following 2 paths can race:

  thread 1:
    ...
    thread__new
      comm__new
        comm_str__findnew
          down_write(&comm_str_lock);
          __comm_str__findnew
            comm_str__get

  thread 2:
    ...
    comm__override or comm__free
      comm_str__put
        refcount_dec_and_test
          down_write(&comm_str_lock);
          rb_erase(&cs->rb_node, &comm_str_root);

Because thread 2 first decrements the refcnt and only after then it removes the
struct comm_str from the list, the thread 1 can find this object on the list
with refcnt equls to 0 and hit the assert.

This patch fixes the thread 1 __comm_str__findnew path, by ignoring objects
that already dropped the refcnt to 0. For the rest of the objects we take the
refcnt before comparing its name and release it afterwards with comm_str__put,
which can also release the object completely.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Lukasz Odzioba <lukasz.odzioba@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: kernel-team@lge.com
Link: http://lkml.kernel.org/r/20180720101740.GA27176@krava
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/comm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 7798a2cc8a86..31279a7bd919 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -20,9 +20,10 @@ static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,}
 
 static struct comm_str *comm_str__get(struct comm_str *cs)
 {
-	if (cs)
-		refcount_inc(&cs->refcnt);
-	return cs;
+	if (cs && refcount_inc_not_zero(&cs->refcnt))
+		return cs;
+
+	return NULL;
 }
 
 static void comm_str__put(struct comm_str *cs)
@@ -67,9 +68,14 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
 		parent = *p;
 		iter = rb_entry(parent, struct comm_str, rb_node);
 
+		/*
+		 * If we race with comm_str__put, iter->refcnt is 0
+		 * and it will be removed within comm_str__put call
+		 * shortly, ignore it in this search.
+		 */
 		cmp = strcmp(str, iter->str);
-		if (!cmp)
-			return comm_str__get(iter);
+		if (!cmp && comm_str__get(iter))
+			return iter;
 
 		if (cmp < 0)
 			p = &(*p)->rb_left;