perf tools: Protect accesses the dso rbtrees/ lists with a rw lock

tip:perf/core tip-bot for Arnaldo Carvalho de Melo 3 participants 4 messages Jun 9, 2015 – Jun 10, 2015 perf events

Share & tools

tip-bot for Arnaldo Carvalho de Melo

author merge bot

2015-06-09 09:53 UTC #0 lore raw

Commit-ID:  e88078442232f3bbcb4ff1d24b3f9ab3dca472b9
Gitweb:     http://git.kernel.org/tip/e88078442232f3bbcb4ff1d24b3f9ab3dca472b9
Author:     Arnaldo Carvalho de Melo <[email protected]>
AuthorDate: Mon, 1 Jun 2015 15:40:01 -0300
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 8 Jun 2015 10:31:40 -0300

perf tools: Protect accesses the dso rbtrees/lists with a rw lock

To allow concurrent access, next step: refcount struct dso instances, so that we can ditch unused them when the last map pointing to it goes away.

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

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

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/dso.c     | 52 +++++++++++++++++++++++++++++++++-------------
 tools/perf/util/dso.h     | 10 ++++++---
 tools/perf/util/machine.c | 27 ++++++++++++++++++------
 tools/perf/util/vdso.c    | 53 ++++++++++++++++++++++++++---------------------
 4 files changed, 95 insertions(+), 47 deletions(-)

tools/perf/util/dso.c +38 ~~-14~~

index 5ec9e89..ff0204a 100644
@@ -889,8 +889,8 @@ struct dso *machine__findnew_kernel(struct machine *machine, const char *name,
  * Either one of the dso or name parameter must be non-NULL or the
  * function will not work.
  */
-static struct dso *dso__findlink_by_longname(struct rb_root *root,
-					     struct dso *dso, const char *name)
+static struct dso *__dso__findlink_by_longname(struct rb_root *root,
+					       struct dso *dso, const char *name)
 {
 	struct rb_node **p = &root->rb_node;
 	struct rb_node  *parent = NULL;

@@ -937,10 +937,10 @@ static struct dso *dso__findlink_by_longname(struct rb_root *root,
 	return NULL;
 }
 
-static inline struct dso *
-dso__find_by_longname(const struct rb_root *root, const char *name)
+static inline struct dso *__dso__find_by_longname(struct rb_root *root,
+						  const char *name)
 {
-	return dso__findlink_by_longname((struct rb_root *)root, NULL, name);
+	return __dso__findlink_by_longname(root, NULL, name);
 }
 
 void dso__set_long_name(struct dso *dso, const char *name, bool name_allocated)

@@ -1149,14 +1149,20 @@ bool __dsos__read_build_ids(struct list_head *head, bool with_hits)
 	return have_build_id;
 }
 
-void dsos__add(struct dsos *dsos, struct dso *dso)
+void __dsos__add(struct dsos *dsos, struct dso *dso)
 {
 	list_add_tail(&dso->node, &dsos->head);
-	dso__findlink_by_longname(&dsos->root, dso, NULL);
+	__dso__findlink_by_longname(&dsos->root, dso, NULL);
+}
+
+void dsos__add(struct dsos *dsos, struct dso *dso)
+{
+	pthread_rwlock_wrlock(&dsos->lock);
+	__dsos__add(dsos, dso);
+	pthread_rwlock_unlock(&dsos->lock);
 }
 
-struct dso *dsos__find(const struct dsos *dsos, const char *name,
-		       bool cmp_short)
+struct dso *__dsos__find(struct dsos *dsos, const char *name, bool cmp_short)
 {
 	struct dso *pos;

@@ -1166,15 +1172,24 @@ struct dso *dsos__find(const struct dsos *dsos, const char *name,
 				return pos;
 		return NULL;
 	}
-	return dso__find_by_longname(&dsos->root, name);
+	return __dso__find_by_longname(&dsos->root, name);
 }
 
-struct dso *dsos__addnew(struct dsos *dsos, const char *name)
+struct dso *dsos__find(struct dsos *dsos, const char *name, bool cmp_short)
+{
+	struct dso *dso;
+	pthread_rwlock_rdlock(&dsos->lock);
+	dso = __dsos__find(dsos, name, cmp_short);
+	pthread_rwlock_unlock(&dsos->lock);
+	return dso;
+}
+
+struct dso *__dsos__addnew(struct dsos *dsos, const char *name)
 {
 	struct dso *dso = dso__new(name);
 
 	if (dso != NULL) {
-		dsos__add(dsos, dso);
+		__dsos__add(dsos, dso);
 		dso__set_basename(dso);
 	}
 	return dso;

@@ -1182,9 +1197,18 @@ struct dso *dsos__addnew(struct dsos *dsos, const char *name)
 
 struct dso *__dsos__findnew(struct dsos *dsos, const char *name)
 {
-	struct dso *dso = dsos__find(dsos, name, false);
+	struct dso *dso = __dsos__find(dsos, name, false);
+
+	return dso ? dso : __dsos__addnew(dsos, name);
+}
 
-	return dso ? dso : dsos__addnew(dsos, name);
+struct dso *dsos__findnew(struct dsos *dsos, const char *name)
+{
+	struct dso *dso;
+	pthread_rwlock_wrlock(&dsos->lock);
+	dso = __dsos__findnew(dsos, name);
+	pthread_rwlock_unlock(&dsos->lock);
+	return dso;
 }
 
 size_t __dsos__fprintf_buildid(struct list_head *head, FILE *fp,

tools/perf/util/dso.h +7 -3

index ba2d90e..c16ab5d 100644
@@ -4,6 +4,7 @@
 #include <linux/types.h>
 #include <linux/rbtree.h>
 #include <stdbool.h>
+#include <pthread.h>
 #include <linux/types.h>
 #include <linux/bitops.h>
 #include "map.h"

@@ -124,6 +125,7 @@ struct dso_cache {
 struct dsos {
 	struct list_head head;
 	struct rb_root	 root;	/* rbtree root sorted by long name */
+	pthread_rwlock_t lock;
 };
 
 struct auxtrace_cache;

@@ -297,11 +299,13 @@ struct map *dso__new_map(const char *name);
 struct dso *machine__findnew_kernel(struct machine *machine, const char *name,
 				    const char *short_name, int dso_type);
 
+void __dsos__add(struct dsos *dsos, struct dso *dso);
 void dsos__add(struct dsos *dsos, struct dso *dso);
-struct dso *dsos__addnew(struct dsos *dsos, const char *name);
-struct dso *dsos__find(const struct dsos *dsos, const char *name,
-		       bool cmp_short);
+struct dso *__dsos__addnew(struct dsos *dsos, const char *name);
+struct dso *__dsos__find(struct dsos *dsos, const char *name, bool cmp_short);
+struct dso *dsos__find(struct dsos *dsos, const char *name, bool cmp_short);
 struct dso *__dsos__findnew(struct dsos *dsos, const char *name);
+struct dso *dsos__findnew(struct dsos *dsos, const char *name);
 bool __dsos__read_build_ids(struct list_head *head, bool with_hits);
 
 size_t __dsos__fprintf_buildid(struct list_head *head, FILE *fp,

tools/perf/util/machine.c +21 -6

index dfd4197..0cf56d6 100644
@@ -20,6 +20,7 @@ static void dsos__init(struct dsos *dsos)
 {
 	INIT_LIST_HEAD(&dsos->head);
 	dsos->root = RB_ROOT;
+	pthread_rwlock_init(&dsos->lock, NULL);
 }
 
 int machine__init(struct machine *machine, const char *root_dir, pid_t pid)

@@ -81,15 +82,21 @@ out_delete:
 	return NULL;
 }
 
-static void dsos__delete(struct dsos *dsos)
+static void dsos__exit(struct dsos *dsos)
 {
 	struct dso *pos, *n;
 
+	pthread_rwlock_wrlock(&dsos->lock);
+
 	list_for_each_entry_safe(pos, n, &dsos->head, node) {
 		RB_CLEAR_NODE(&pos->rb_node);
 		list_del(&pos->node);
 		dso__delete(pos);
 	}
+
+	pthread_rwlock_unlock(&dsos->lock);
+
+	pthread_rwlock_destroy(&dsos->lock);
 }
 
 void machine__delete_threads(struct machine *machine)

@@ -110,7 +117,7 @@ void machine__delete_threads(struct machine *machine)
 void machine__exit(struct machine *machine)
 {
 	map_groups__exit(&machine->kmaps);
-	dsos__delete(&machine->dsos);
+	dsos__exit(&machine->dsos);
 	machine__exit_vdso(machine);
 	zfree(&machine->root_dir);
 	zfree(&machine->current_tid);

@@ -496,11 +503,13 @@ static struct dso *machine__findnew_module_dso(struct machine *machine,
 {
 	struct dso *dso;
 
-	dso = dsos__find(&machine->dsos, m->name, true);
+	pthread_rwlock_wrlock(&machine->dsos.lock);
+
+	dso = __dsos__find(&machine->dsos, m->name, true);
 	if (!dso) {
-		dso = dsos__addnew(&machine->dsos, m->name);
+		dso = __dsos__addnew(&machine->dsos, m->name);
 		if (dso == NULL)
-			return NULL;
+			goto out_unlock;
 
 		if (machine__is_host(machine))
 			dso->symtab_type = DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE;

@@ -515,6 +524,8 @@ static struct dso *machine__findnew_module_dso(struct machine *machine,
 		dso__set_long_name(dso, strdup(filename), true);
 	}
 
+out_unlock:
+	pthread_rwlock_unlock(&machine->dsos.lock);
 	return dso;
 }

@@ -1156,6 +1167,8 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
 		struct dso *kernel = NULL;
 		struct dso *dso;
 
+		pthread_rwlock_rdlock(&machine->dsos.lock);
+
 		list_for_each_entry(dso, &machine->dsos.head, node) {
 
 			/*

@@ -1184,6 +1197,8 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
 			break;
 		}
 
+		pthread_rwlock_unlock(&machine->dsos.lock);
+
 		if (kernel == NULL)
 			kernel = machine__findnew_dso(machine, kmmap_prefix);
 		if (kernel == NULL)

@@ -1948,5 +1963,5 @@ int machine__get_kernel_start(struct machine *machine)
 
 struct dso *machine__findnew_dso(struct machine *machine, const char *filename)
 {
-	return __dsos__findnew(&machine->dsos, filename);
+	return dsos__findnew(&machine->dsos, filename);
 }

tools/perf/util/vdso.c +29 ~~-24~~

111 lines not on this page · raw message

Ingo Molnar

2015-06-09 09:56 UTC #1 lore raw

tools/perf/util/dso.c bool __dsos__read_build_ids(struct list_head *head, bool with_hits) · line 1163

Quoted 6 lines · tip-bot for Arnaldo Carvalho de Melo

tools/perf/util/dso.c +5 -0

+void dsos__add(struct dsos *dsos, struct dso *dso)
+{
+	pthread_rwlock_wrlock(&dsos->lock);
+	__dsos__add(dsos, dso);
+	pthread_rwlock_unlock(&dsos->lock);
 }

Please introduce wrappers and use the kernel API names:
read_lock()/read_unlock()/etc. (and name the mutex primitives mutex_lock()/unlock())

That way kernel developers will find their way around the perf locking details easily, and we can also use liblockdep to check locking correctness.

Thanks!

Ingo

Arnaldo Carvalho de Melo

2015-06-09 14:25 UTC #2 lore raw

Em Tue, Jun 09, 2015 at 11:56:10AM +0200, Ingo Molnar escreveu:

Quoted 13 lines · tip-bot for Arnaldo Carvalho de Melo

tools/perf/util/dso.c bool __dsos__read_build_ids(struct list_head *head, bool with_hits) · line 1163
Quoted 7 lines
tools/perf/util/dso.c +5 -0
+void dsos__add(struct dsos *dsos, struct dso *dso)
+{
+	pthread_rwlock_wrlock(&dsos->lock);
+	__dsos__add(dsos, dso);
+	pthread_rwlock_unlock(&dsos->lock);
 }
Please introduce wrappers and use the kernel API names:
read_lock()/read_unlock()/etc. (and name the mutex primitives mutex_lock()/unlock())

Quoted 2 lines

That way kernel developers will find their way around the perf locking details easily, and we can also use liblockdep to check locking correctness.

Yeah, we discussed about that, will do it eventually, you said this was already present in some tools/ code, but I couldn't find it:

[acme@zoo linux]$ find tools/ -type f | xargs grep '\<mutex_unlock' [acme@zoo linux]$

Anyway, worthy goal, will do.

- Arnaldo

Ingo Molnar

2015-06-10 07:04 UTC #3 lore raw

Quoted 14 lines · Arnaldo Carvalho de Melo

Em Tue, Jun 09, 2015 at 11:56:10AM +0200, Ingo Molnar escreveu:
Quoted 13 lines · tip-bot for Arnaldo Carvalho de Melo
tools/perf/util/dso.c bool __dsos__read_build_ids(struct list_head *head, bool with_hits) · line 1163
Quoted 7 lines
tools/perf/util/dso.c +5 -0
+void dsos__add(struct dsos *dsos, struct dso *dso)
+{
+	pthread_rwlock_wrlock(&dsos->lock);
+	__dsos__add(dsos, dso);
+	pthread_rwlock_unlock(&dsos->lock);
 }
Please introduce wrappers and use the kernel API names:
read_lock()/read_unlock()/etc. (and name the mutex primitives mutex_lock()/unlock())

Quoted 8 lines

Quoted 2 lines
That way kernel developers will find their way around the perf locking details easily, and we can also use liblockdep to check locking correctness.
Yeah, we discussed about that, will do it eventually, you said this was already present in some tools/ code, but I couldn't find it:
[acme@zoo linux]$ find tools/ -type f | xargs grep '\<mutex_unlock' [acme@zoo linux]$

it's in -tip in tools/kvm/:

triton:~/tip> git grep mutex_lock tools/kvm/include/kvm/mutex.h tools/kvm/include/kvm/mutex.h:static inline void mutex_lock(struct mutex *lock)

triton:~/tip> git grep down tools/kvm/include/kvm/rwsem.h tools/kvm/include/kvm/rwsem.h:static inline void down_read(pthread_rwlock_t *rwsem) tools/kvm/include/kvm/rwsem.h:static inline void down_write(pthread_rwlock_t *rwsem)

Thanks,

Ingo