RE: [PATCH RFC V2 05/10] perf tools: lock to protect thread list
> > SNIP > > > + pthread_mutex_unlock(>namespaces_lock); > > + > > return 0; > > } > > > > -void thread__namespaces_id(const struct thread *thread, > > +void thread__namespaces_id(struct thread *thread, > >u64 *dev, u64 *ino) > > { > > struct namespaces *ns; > > > > + pthread_mutex_lock(>namespaces_lock); > > ns = thread__namespaces(thread); > > isn't it just thread__namespaces that needs this lock? I also wanted to protect *dev = ns ? ns->link_info[CGROUP_NS_INDEX].dev : 0; *ino = ns ? ns->link_info[CGROUP_NS_INDEX].ino : 0; Because I was not sure if ns is still accurate when we try to use it later. But for our case (perf top event synthesizing), it looks I worried too much. Namespaces event isn't processed at all. So yes, we don't need patch 4 for the optimization. Based on the same reason, I used comm_str in patch 3. It's not help for the optimization either, but should be useful for future. Anyway, I think I will drop patch 3 & 4 for V3. Thanks, Kan > > if that's the case we don't need the change for __hists__add_entry in > previous patch > > jirka
RE: [PATCH RFC V2 05/10] perf tools: lock to protect thread list
> > SNIP > > > + pthread_mutex_unlock(>namespaces_lock); > > + > > return 0; > > } > > > > -void thread__namespaces_id(const struct thread *thread, > > +void thread__namespaces_id(struct thread *thread, > >u64 *dev, u64 *ino) > > { > > struct namespaces *ns; > > > > + pthread_mutex_lock(>namespaces_lock); > > ns = thread__namespaces(thread); > > isn't it just thread__namespaces that needs this lock? I also wanted to protect *dev = ns ? ns->link_info[CGROUP_NS_INDEX].dev : 0; *ino = ns ? ns->link_info[CGROUP_NS_INDEX].ino : 0; Because I was not sure if ns is still accurate when we try to use it later. But for our case (perf top event synthesizing), it looks I worried too much. Namespaces event isn't processed at all. So yes, we don't need patch 4 for the optimization. Based on the same reason, I used comm_str in patch 3. It's not help for the optimization either, but should be useful for future. Anyway, I think I will drop patch 3 & 4 for V3. Thanks, Kan > > if that's the case we don't need the change for __hists__add_entry in > previous patch > > jirka
Re: [PATCH RFC V2 05/10] perf tools: lock to protect thread list
On Sun, Sep 10, 2017 at 07:23:18PM -0700, kan.li...@intel.com wrote: SNIP > + pthread_mutex_unlock(>namespaces_lock); > + > return 0; > } > > -void thread__namespaces_id(const struct thread *thread, > +void thread__namespaces_id(struct thread *thread, > u64 *dev, u64 *ino) > { > struct namespaces *ns; > > + pthread_mutex_lock(>namespaces_lock); > ns = thread__namespaces(thread); isn't it just thread__namespaces that needs this lock? if that's the case we don't need the change for __hists__add_entry in previous patch jirka
Re: [PATCH RFC V2 05/10] perf tools: lock to protect thread list
On Sun, Sep 10, 2017 at 07:23:18PM -0700, kan.li...@intel.com wrote: SNIP > + pthread_mutex_unlock(>namespaces_lock); > + > return 0; > } > > -void thread__namespaces_id(const struct thread *thread, > +void thread__namespaces_id(struct thread *thread, > u64 *dev, u64 *ino) > { > struct namespaces *ns; > > + pthread_mutex_lock(>namespaces_lock); > ns = thread__namespaces(thread); isn't it just thread__namespaces that needs this lock? if that's the case we don't need the change for __hists__add_entry in previous patch jirka
[PATCH RFC V2 05/10] perf tools: lock to protect thread list
From: Kan LiangAdd two locks to protect namespaces_list and comm_list. The comm which is used in db-export are not protected. Because the multithread code will not touch it. It can be added later if required. Signed-off-by: Kan Liang --- tools/perf/ui/browsers/hists.c | 2 +- tools/perf/util/thread.c | 60 +- tools/perf/util/thread.h | 6 +++-- 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 13dfb0a..429e1dd 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -2370,7 +2370,7 @@ static int perf_evsel_browser_title(struct hist_browser *browser, char unit; int printed; const struct dso *dso = hists->dso_filter; - const struct thread *thread = hists->thread_filter; + struct thread *thread = hists->thread_filter; int socket_id = hists->socket_filter; unsigned long nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE]; u64 nr_events = hists->stats.total_period; diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c index b7957da..25830b2 100644 --- a/tools/perf/util/thread.c +++ b/tools/perf/util/thread.c @@ -45,6 +45,8 @@ struct thread *thread__new(pid_t pid, pid_t tid) thread->cpu = -1; INIT_LIST_HEAD(>namespaces_list); INIT_LIST_HEAD(>comm_list); + pthread_mutex_init(>namespaces_lock, NULL); + pthread_mutex_init(>comm_lock, NULL); comm_str = malloc(32); if (!comm_str) @@ -83,15 +85,21 @@ void thread__delete(struct thread *thread) map_groups__put(thread->mg); thread->mg = NULL; } + pthread_mutex_lock(>namespaces_lock); list_for_each_entry_safe(namespaces, tmp_namespaces, >namespaces_list, list) { list_del(>list); namespaces__free(namespaces); } + pthread_mutex_unlock(>namespaces_lock); + + pthread_mutex_lock(>comm_lock); list_for_each_entry_safe(comm, tmp_comm, >comm_list, list) { list_del(>list); comm__free(comm); } + pthread_mutex_unlock(>comm_lock); + unwind__finish_access(thread); nsinfo__zput(thread->nsinfo); @@ -128,11 +136,17 @@ struct namespaces *thread__namespaces(const struct thread *thread) int thread__set_namespaces(struct thread *thread, u64 timestamp, struct namespaces_event *event) { - struct namespaces *new, *curr = thread__namespaces(thread); + struct namespaces *new, *curr; + + pthread_mutex_lock(>namespaces_lock); + + curr = thread__namespaces(thread); new = namespaces__new(event); - if (!new) + if (!new) { + pthread_mutex_unlock(>namespaces_lock); return -ENOMEM; + } list_add(>list, >namespaces_list); @@ -146,17 +160,21 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp, curr->end_time = timestamp; } + pthread_mutex_unlock(>namespaces_lock); + return 0; } -void thread__namespaces_id(const struct thread *thread, +void thread__namespaces_id(struct thread *thread, u64 *dev, u64 *ino) { struct namespaces *ns; + pthread_mutex_lock(>namespaces_lock); ns = thread__namespaces(thread); *dev = ns ? ns->link_info[CGROUP_NS_INDEX].dev : 0; *ino = ns ? ns->link_info[CGROUP_NS_INDEX].ino : 0; + pthread_mutex_unlock(>namespaces_lock); } struct comm *thread__comm(const struct thread *thread) @@ -183,17 +201,23 @@ struct comm *thread__exec_comm(const struct thread *thread) int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp, bool exec) { - struct comm *new, *curr = thread__comm(thread); + struct comm *new, *curr; + int err = 0; + + pthread_mutex_lock(>comm_lock); + curr = thread__comm(thread); /* Override the default :tid entry */ if (!thread->comm_set) { - int err = comm__override(curr, str, timestamp, exec); + err = comm__override(curr, str, timestamp, exec); if (err) - return err; + goto unlock; } else { new = comm__new(str, timestamp, exec); - if (!new) - return -ENOMEM; + if (!new) { + err = -ENOMEM; + goto unlock; + } list_add(>list, >comm_list); if (exec) @@ -202,7 +226,9 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp, thread->comm_set = true; -
[PATCH RFC V2 05/10] perf tools: lock to protect thread list
From: Kan Liang Add two locks to protect namespaces_list and comm_list. The comm which is used in db-export are not protected. Because the multithread code will not touch it. It can be added later if required. Signed-off-by: Kan Liang --- tools/perf/ui/browsers/hists.c | 2 +- tools/perf/util/thread.c | 60 +- tools/perf/util/thread.h | 6 +++-- 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 13dfb0a..429e1dd 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -2370,7 +2370,7 @@ static int perf_evsel_browser_title(struct hist_browser *browser, char unit; int printed; const struct dso *dso = hists->dso_filter; - const struct thread *thread = hists->thread_filter; + struct thread *thread = hists->thread_filter; int socket_id = hists->socket_filter; unsigned long nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE]; u64 nr_events = hists->stats.total_period; diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c index b7957da..25830b2 100644 --- a/tools/perf/util/thread.c +++ b/tools/perf/util/thread.c @@ -45,6 +45,8 @@ struct thread *thread__new(pid_t pid, pid_t tid) thread->cpu = -1; INIT_LIST_HEAD(>namespaces_list); INIT_LIST_HEAD(>comm_list); + pthread_mutex_init(>namespaces_lock, NULL); + pthread_mutex_init(>comm_lock, NULL); comm_str = malloc(32); if (!comm_str) @@ -83,15 +85,21 @@ void thread__delete(struct thread *thread) map_groups__put(thread->mg); thread->mg = NULL; } + pthread_mutex_lock(>namespaces_lock); list_for_each_entry_safe(namespaces, tmp_namespaces, >namespaces_list, list) { list_del(>list); namespaces__free(namespaces); } + pthread_mutex_unlock(>namespaces_lock); + + pthread_mutex_lock(>comm_lock); list_for_each_entry_safe(comm, tmp_comm, >comm_list, list) { list_del(>list); comm__free(comm); } + pthread_mutex_unlock(>comm_lock); + unwind__finish_access(thread); nsinfo__zput(thread->nsinfo); @@ -128,11 +136,17 @@ struct namespaces *thread__namespaces(const struct thread *thread) int thread__set_namespaces(struct thread *thread, u64 timestamp, struct namespaces_event *event) { - struct namespaces *new, *curr = thread__namespaces(thread); + struct namespaces *new, *curr; + + pthread_mutex_lock(>namespaces_lock); + + curr = thread__namespaces(thread); new = namespaces__new(event); - if (!new) + if (!new) { + pthread_mutex_unlock(>namespaces_lock); return -ENOMEM; + } list_add(>list, >namespaces_list); @@ -146,17 +160,21 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp, curr->end_time = timestamp; } + pthread_mutex_unlock(>namespaces_lock); + return 0; } -void thread__namespaces_id(const struct thread *thread, +void thread__namespaces_id(struct thread *thread, u64 *dev, u64 *ino) { struct namespaces *ns; + pthread_mutex_lock(>namespaces_lock); ns = thread__namespaces(thread); *dev = ns ? ns->link_info[CGROUP_NS_INDEX].dev : 0; *ino = ns ? ns->link_info[CGROUP_NS_INDEX].ino : 0; + pthread_mutex_unlock(>namespaces_lock); } struct comm *thread__comm(const struct thread *thread) @@ -183,17 +201,23 @@ struct comm *thread__exec_comm(const struct thread *thread) int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp, bool exec) { - struct comm *new, *curr = thread__comm(thread); + struct comm *new, *curr; + int err = 0; + + pthread_mutex_lock(>comm_lock); + curr = thread__comm(thread); /* Override the default :tid entry */ if (!thread->comm_set) { - int err = comm__override(curr, str, timestamp, exec); + err = comm__override(curr, str, timestamp, exec); if (err) - return err; + goto unlock; } else { new = comm__new(str, timestamp, exec); - if (!new) - return -ENOMEM; + if (!new) { + err = -ENOMEM; + goto unlock; + } list_add(>list, >comm_list); if (exec) @@ -202,7 +226,9 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp, thread->comm_set = true; - return 0; +unlock: +