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]
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(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5ec9e89..ff0204a 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -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,
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index ba2d90e..c16ab5d 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -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,
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index dfd4197..0cf56d6 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -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);
}
diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
index 2e8f688..c646c74 100644
--- a/tools/perf/util/vdso.c
+++ b/tools/perf/util/vdso.c
@@ -120,14 +120,14 @@ void machine__exit_vdso(struct machine *machine)
zfree(&machine->vdso_info);
}
-static struct dso *machine__addnew_vdso(struct machine *machine, const char *short_name,
- const char *long_name)
+static struct dso *__machine__addnew_vdso(struct machine *machine, const char *short_name,
+ const char *long_name)
{
struct dso *dso;
dso = dso__new(short_name);
if (dso != NULL) {
- dsos__add(&machine->dsos, dso);
+ __dsos__add(&machine->dsos, dso);
dso__set_long_name(dso, long_name, false);
}
@@ -230,27 +230,31 @@ static const char *vdso__get_compat_file(struct vdso_file *vdso_file)
return vdso_file->temp_file_name;
}
-static struct dso *vdso__findnew_compat(struct machine *machine,
- struct vdso_file *vdso_file)
+static struct dso *__machine__findnew_compat(struct machine *machine,
+ struct vdso_file *vdso_file)
{
const char *file_name;
struct dso *dso;
- dso = dsos__find(&machine->dsos, vdso_file->dso_name, true);
+ pthread_rwlock_wrlock(&machine->dsos.lock);
+ dso = __dsos__find(&machine->dsos, vdso_file->dso_name, true);
if (dso)
- return dso;
+ goto out_unlock;
file_name = vdso__get_compat_file(vdso_file);
if (!file_name)
- return NULL;
+ goto out_unlock;
- return machine__addnew_vdso(machine, vdso_file->dso_name, file_name);
+ dso = __machine__addnew_vdso(machine, vdso_file->dso_name, file_name);
+out_unlock:
+ pthread_rwlock_unlock(&machine->dsos.lock);
+ return dso;
}
-static int machine__findnew_vdso_compat(struct machine *machine,
- struct thread *thread,
- struct vdso_info *vdso_info,
- struct dso **dso)
+static int __machine__findnew_vdso_compat(struct machine *machine,
+ struct thread *thread,
+ struct vdso_info *vdso_info,
+ struct dso **dso)
{
enum dso_type dso_type;
@@ -267,10 +271,10 @@ static int machine__findnew_vdso_compat(struct machine *machine,
switch (dso_type) {
case DSO__TYPE_32BIT:
- *dso = vdso__findnew_compat(machine, &vdso_info->vdso32);
+ *dso = __machine__findnew_compat(machine, &vdso_info->vdso32);
return 1;
case DSO__TYPE_X32BIT:
- *dso = vdso__findnew_compat(machine, &vdso_info->vdsox32);
+ *dso = __machine__findnew_compat(machine, &vdso_info->vdsox32);
return 1;
case DSO__TYPE_UNKNOWN:
case DSO__TYPE_64BIT:
@@ -285,31 +289,32 @@ struct dso *machine__findnew_vdso(struct machine *machine,
struct thread *thread __maybe_unused)
{
struct vdso_info *vdso_info;
- struct dso *dso;
+ struct dso *dso = NULL;
+ pthread_rwlock_wrlock(&machine->dsos.lock);
if (!machine->vdso_info)
machine->vdso_info = vdso_info__new();
vdso_info = machine->vdso_info;
if (!vdso_info)
- return NULL;
+ goto out_unlock;
#if BITS_PER_LONG == 64
- if (machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
- return dso;
+ if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
+ goto out_unlock;
#endif
- dso = dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+ dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
if (!dso) {
char *file;
file = get_file(&vdso_info->vdso);
- if (!file)
- return NULL;
-
- dso = machine__addnew_vdso(machine, DSO__NAME_VDSO, file);
+ if (file)
+ dso = __machine__addnew_vdso(machine, DSO__NAME_VDSO, file);
}
+out_unlock:
+ pthread_rwlock_unlock(&machine->dsos.lock);
return dso;
}
* tip-bot for Arnaldo Carvalho de Melo <[email protected]> wrote:
> +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
Em Tue, Jun 09, 2015 at 11:56:10AM +0200, Ingo Molnar escreveu:
> * tip-bot for Arnaldo Carvalho de Melo <[email protected]> wrote:
> > +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.
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
* Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Tue, Jun 09, 2015 at 11:56:10AM +0200, Ingo Molnar escreveu:
> > * tip-bot for Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> > > +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.
>
> 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