perf tools: Fix top crashes

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/comm.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

tools/perf/util/comm.c +9 -6

index 7798a2cc8a86..9c4a18991e41 100644
@@ -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;

Signature · 1 line

2.17.1

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/comm.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

tools/perf/util/comm.c +0 -0

index 7798a2cc8a86..9c4a18991e41 100644
@@ -18,11 +18,9 @@ struct comm_str {
 static struct rb_root comm_str_root;

tools/perf/util/comm.c +7 -2

 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;

Signature · 1 line

2.17.1

Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:

tools/perf/util/comm.c struct comm_str { · line 24

Quoted 13 lines

+++ b/tools/perf/util/comm.c

tools/perf/util/comm.c +2 -4

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

Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 23 lines

Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:

tools/perf/util/comm.c struct comm_str { · line 24

Quoted 13 lines

+++ b/tools/perf/util/comm.c

tools/perf/util/comm.c +2 -4

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

Quoted 1 line

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.

Hi Arnaldo,

Quoted 24 lines · Arnaldo Carvalho de Melo

Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 23 lines

Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:

Review struct comm_str { · line 24

Quoted 13 lines

+++ b/tools/perf/util/comm.c

(quoted diff) +2 -4

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

Quoted 5 lines

Quoted 1 line

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 <[email protected]>

Thanks,
Namhyung

Quoted 43 lines

Hi Arnaldo,

Quoted 24 lines · Arnaldo Carvalho de Melo

Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 23 lines

Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:

tools/perf/util/comm.c struct comm_str { · line 24

Quoted 13 lines

+++ b/tools/perf/util/comm.c

tools/perf/util/comm.c +2 -4

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

Quoted 5 lines

Quoted 1 line

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

Quoted 6 lines

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

Acked-by Namhyung Kim <[email protected]>

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 <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/comm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

tools/perf/util/comm.c +11 -5

index 7798a2cc8a86..31279a7bd919 100644
@@ -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;

Signature · 1 line

2.17.1