When fixing a locking race and memory leak in:
https://lore.kernel.org/linux-perf-users/[email protected]/
It was requested that debug mutex code be separated out into its own
files. This was, in part, done by Pavithra Gurushankar in:
https://lore.kernel.org/lkml/[email protected]/
These patches fix issues with the previous patches, add in the
original dso->nsinfo fix and then build on our mutex wrapper with
clang's -Wthread-safety analysis. The analysis found missing unlocks
in builtin-sched.c which are fixed and -Wthread-safety is enabled by
default when building with clang.
v3. Adds a missing new line to the error messages and removes the
pshared argument to mutex_init by having two functions, mutex_init
and mutex_init_pshared. These changes were suggested by Adrian Hunter.
v2. Breaks apart changes that s/pthread_mutex/mutex/g and the lock
annotations as requested by Arnaldo and Namhyung. A boolean is
added to builtin-sched.c to terminate thread funcs rather than
leaving them blocked on delted mutexes.
Ian Rogers (17):
perf bench: Update use of pthread mutex/cond
perf tests: Avoid pthread.h inclusion
perf hist: Update use of pthread mutex
perf bpf: Remove unused pthread.h include
perf lock: Remove unused pthread.h include
perf record: Update use of pthread mutex
perf sched: Update use of pthread mutex
perf ui: Update use of pthread mutex
perf mmap: Remove unnecessary pthread.h include
perf dso: Update use of pthread mutex
perf annotate: Update use of pthread mutex
perf top: Update use of pthread mutex
perf dso: Hold lock when accessing nsinfo
perf mutex: Add thread safety annotations
perf sched: Fixes for thread safety analysis
perf top: Fixes for thread safety analysis
perf build: Enable -Wthread-safety with clang
Pavithra Gurushankar (1):
perf mutex: Wrapped usage of mutex and cond
tools/perf/Makefile.config | 5 +
tools/perf/bench/epoll-ctl.c | 33 +++---
tools/perf/bench/epoll-wait.c | 33 +++---
tools/perf/bench/futex-hash.c | 33 +++---
tools/perf/bench/futex-lock-pi.c | 33 +++---
tools/perf/bench/futex-requeue.c | 33 +++---
tools/perf/bench/futex-wake-parallel.c | 33 +++---
tools/perf/bench/futex-wake.c | 33 +++---
tools/perf/bench/numa.c | 93 ++++++----------
tools/perf/builtin-inject.c | 4 +
tools/perf/builtin-lock.c | 1 -
tools/perf/builtin-record.c | 13 ++-
tools/perf/builtin-sched.c | 105 +++++++++---------
tools/perf/builtin-top.c | 45 ++++----
tools/perf/tests/mmap-basic.c | 2 -
tools/perf/tests/openat-syscall-all-cpus.c | 2 +-
tools/perf/tests/perf-record.c | 2 -
tools/perf/ui/browser.c | 20 ++--
tools/perf/ui/browsers/annotate.c | 12 +--
tools/perf/ui/setup.c | 5 +-
tools/perf/ui/tui/helpline.c | 5 +-
tools/perf/ui/tui/progress.c | 8 +-
tools/perf/ui/tui/setup.c | 8 +-
tools/perf/ui/tui/util.c | 18 ++--
tools/perf/ui/ui.h | 4 +-
tools/perf/util/Build | 1 +
tools/perf/util/annotate.c | 15 +--
tools/perf/util/annotate.h | 4 +-
tools/perf/util/bpf-event.h | 1 -
tools/perf/util/build-id.c | 12 ++-
tools/perf/util/dso.c | 19 ++--
tools/perf/util/dso.h | 4 +-
tools/perf/util/hist.c | 6 +-
tools/perf/util/hist.h | 4 +-
tools/perf/util/map.c | 3 +
tools/perf/util/mmap.h | 1 -
tools/perf/util/mutex.c | 119 +++++++++++++++++++++
tools/perf/util/mutex.h | 109 +++++++++++++++++++
tools/perf/util/probe-event.c | 3 +
tools/perf/util/symbol.c | 4 +-
tools/perf/util/top.h | 5 +-
41 files changed, 570 insertions(+), 323 deletions(-)
create mode 100644 tools/perf/util/mutex.c
create mode 100644 tools/perf/util/mutex.h
--
2.37.2.609.g9ff673ca1a-goog
No pthread usage in bpf-event.h.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/bpf-event.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
index 144a8a24cc69..1bcbd4fb6c66 100644
--- a/tools/perf/util/bpf-event.h
+++ b/tools/perf/util/bpf-event.h
@@ -4,7 +4,6 @@
#include <linux/compiler.h>
#include <linux/rbtree.h>
-#include <pthread.h>
#include <api/fd/array.h>
#include <stdio.h>
--
2.37.2.609.g9ff673ca1a-goog
The comment says it is for cpu_set_t which isn't used in the header.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/mmap.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
index cd8b0777473b..cd4ccec7f361 100644
--- a/tools/perf/util/mmap.h
+++ b/tools/perf/util/mmap.h
@@ -9,7 +9,6 @@
#include <linux/bitops.h>
#include <perf/cpumap.h>
#include <stdbool.h>
-#include <pthread.h> // for cpu_set_t
#ifdef HAVE_AIO_SUPPORT
#include <aio.h>
#endif
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/dso.c | 12 ++++++------
tools/perf/util/dso.h | 4 ++--
tools/perf/util/symbol.c | 4 ++--
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5ac13958d1bd..a9789a955403 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -795,7 +795,7 @@ dso_cache__free(struct dso *dso)
struct rb_root *root = &dso->data.cache;
struct rb_node *next = rb_first(root);
- pthread_mutex_lock(&dso->lock);
+ mutex_lock(&dso->lock);
while (next) {
struct dso_cache *cache;
@@ -804,7 +804,7 @@ dso_cache__free(struct dso *dso)
rb_erase(&cache->rb_node, root);
free(cache);
}
- pthread_mutex_unlock(&dso->lock);
+ mutex_unlock(&dso->lock);
}
static struct dso_cache *__dso_cache__find(struct dso *dso, u64 offset)
@@ -841,7 +841,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
struct dso_cache *cache;
u64 offset = new->offset;
- pthread_mutex_lock(&dso->lock);
+ mutex_lock(&dso->lock);
while (*p != NULL) {
u64 end;
@@ -862,7 +862,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
cache = NULL;
out:
- pthread_mutex_unlock(&dso->lock);
+ mutex_unlock(&dso->lock);
return cache;
}
@@ -1297,7 +1297,7 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
dso->root = NULL;
INIT_LIST_HEAD(&dso->node);
INIT_LIST_HEAD(&dso->data.open_entry);
- pthread_mutex_init(&dso->lock, NULL);
+ mutex_init(&dso->lock);
refcount_set(&dso->refcnt, 1);
}
@@ -1336,7 +1336,7 @@ void dso__delete(struct dso *dso)
dso__free_a2l(dso);
zfree(&dso->symsrc_filename);
nsinfo__zput(dso->nsinfo);
- pthread_mutex_destroy(&dso->lock);
+ mutex_destroy(&dso->lock);
free(dso);
}
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 66981c7a9a18..58d94175e714 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -2,7 +2,6 @@
#ifndef __PERF_DSO
#define __PERF_DSO
-#include <pthread.h>
#include <linux/refcount.h>
#include <linux/types.h>
#include <linux/rbtree.h>
@@ -11,6 +10,7 @@
#include <stdio.h>
#include <linux/bitops.h>
#include "build-id.h"
+#include "mutex.h"
struct machine;
struct map;
@@ -145,7 +145,7 @@ struct dso_cache {
struct auxtrace_cache;
struct dso {
- pthread_mutex_t lock;
+ struct mutex lock;
struct list_head node;
struct rb_node rb_node; /* rbtree node sorted by long name */
struct rb_root *root; /* root of rbtree that rb_node is in */
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a4b22caa7c24..656d9b4dd456 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1800,7 +1800,7 @@ int dso__load(struct dso *dso, struct map *map)
}
nsinfo__mountns_enter(dso->nsinfo, &nsc);
- pthread_mutex_lock(&dso->lock);
+ mutex_lock(&dso->lock);
/* check again under the dso->lock */
if (dso__loaded(dso)) {
@@ -1964,7 +1964,7 @@ int dso__load(struct dso *dso, struct map *map)
ret = 0;
out:
dso__set_loaded(dso);
- pthread_mutex_unlock(&dso->lock);
+ mutex_unlock(&dso->lock);
nsinfo__mountns_exit(&nsc);
return ret;
--
2.37.2.609.g9ff673ca1a-goog
If building with clang then enable -Wthread-safety warnings.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/Makefile.config | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index c41a090c0652..72dadafdbad9 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -19,6 +19,11 @@ detected_var = $(shell echo "$(1)=$($(1))" >> $(OUTPUT).config-detected)
CFLAGS := $(EXTRA_CFLAGS) $(filter-out -Wnested-externs,$(EXTRA_WARNINGS))
HOSTCFLAGS := $(filter-out -Wnested-externs,$(EXTRA_WARNINGS))
+# Enabled Wthread-safety analysis for clang builds.
+ifeq ($(CC_NO_CLANG), 0)
+ CFLAGS += -Wthread-safety
+endif
+
include $(srctree)/tools/scripts/Makefile.arch
$(call detected_var,SRCARCH)
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error
checking. Update cmd_sched so that we always explicitly destroy the
mutexes.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-sched.c | 67 ++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 2f6cd1b8b662..7e4006d6b8bc 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -7,6 +7,7 @@
#include "util/evlist.h"
#include "util/evsel.h"
#include "util/evsel_fprintf.h"
+#include "util/mutex.h"
#include "util/symbol.h"
#include "util/thread.h"
#include "util/header.h"
@@ -184,8 +185,8 @@ struct perf_sched {
struct task_desc **pid_to_task;
struct task_desc **tasks;
const struct trace_sched_handler *tp_handler;
- pthread_mutex_t start_work_mutex;
- pthread_mutex_t work_done_wait_mutex;
+ struct mutex start_work_mutex;
+ struct mutex work_done_wait_mutex;
int profile_cpu;
/*
* Track the current task - that way we can know whether there's any
@@ -635,10 +636,8 @@ static void *thread_func(void *ctx)
again:
ret = sem_post(&this_task->ready_for_work);
BUG_ON(ret);
- ret = pthread_mutex_lock(&sched->start_work_mutex);
- BUG_ON(ret);
- ret = pthread_mutex_unlock(&sched->start_work_mutex);
- BUG_ON(ret);
+ mutex_lock(&sched->start_work_mutex);
+ mutex_unlock(&sched->start_work_mutex);
cpu_usage_0 = get_cpu_usage_nsec_self(fd);
@@ -652,10 +651,8 @@ static void *thread_func(void *ctx)
ret = sem_post(&this_task->work_done_sem);
BUG_ON(ret);
- ret = pthread_mutex_lock(&sched->work_done_wait_mutex);
- BUG_ON(ret);
- ret = pthread_mutex_unlock(&sched->work_done_wait_mutex);
- BUG_ON(ret);
+ mutex_lock(&sched->work_done_wait_mutex);
+ mutex_unlock(&sched->work_done_wait_mutex);
goto again;
}
@@ -672,10 +669,8 @@ static void create_tasks(struct perf_sched *sched)
err = pthread_attr_setstacksize(&attr,
(size_t) max(16 * 1024, (int)PTHREAD_STACK_MIN));
BUG_ON(err);
- err = pthread_mutex_lock(&sched->start_work_mutex);
- BUG_ON(err);
- err = pthread_mutex_lock(&sched->work_done_wait_mutex);
- BUG_ON(err);
+ mutex_lock(&sched->start_work_mutex);
+ mutex_lock(&sched->work_done_wait_mutex);
for (i = 0; i < sched->nr_tasks; i++) {
struct sched_thread_parms *parms = malloc(sizeof(*parms));
BUG_ON(parms == NULL);
@@ -699,7 +694,7 @@ static void wait_for_tasks(struct perf_sched *sched)
sched->start_time = get_nsecs();
sched->cpu_usage = 0;
- pthread_mutex_unlock(&sched->work_done_wait_mutex);
+ mutex_unlock(&sched->work_done_wait_mutex);
for (i = 0; i < sched->nr_tasks; i++) {
task = sched->tasks[i];
@@ -707,12 +702,11 @@ static void wait_for_tasks(struct perf_sched *sched)
BUG_ON(ret);
sem_init(&task->ready_for_work, 0, 0);
}
- ret = pthread_mutex_lock(&sched->work_done_wait_mutex);
- BUG_ON(ret);
+ mutex_lock(&sched->work_done_wait_mutex);
cpu_usage_0 = get_cpu_usage_nsec_parent();
- pthread_mutex_unlock(&sched->start_work_mutex);
+ mutex_unlock(&sched->start_work_mutex);
for (i = 0; i < sched->nr_tasks; i++) {
task = sched->tasks[i];
@@ -734,8 +728,7 @@ static void wait_for_tasks(struct perf_sched *sched)
sched->runavg_parent_cpu_usage = (sched->runavg_parent_cpu_usage * (sched->replay_repeat - 1) +
sched->parent_cpu_usage)/sched->replay_repeat;
- ret = pthread_mutex_lock(&sched->start_work_mutex);
- BUG_ON(ret);
+ mutex_lock(&sched->start_work_mutex);
for (i = 0; i < sched->nr_tasks; i++) {
task = sched->tasks[i];
@@ -3430,8 +3423,6 @@ int cmd_sched(int argc, const char **argv)
},
.cmp_pid = LIST_HEAD_INIT(sched.cmp_pid),
.sort_list = LIST_HEAD_INIT(sched.sort_list),
- .start_work_mutex = PTHREAD_MUTEX_INITIALIZER,
- .work_done_wait_mutex = PTHREAD_MUTEX_INITIALIZER,
.sort_order = default_sort_order,
.replay_repeat = 10,
.profile_cpu = -1,
@@ -3545,8 +3536,10 @@ int cmd_sched(int argc, const char **argv)
.fork_event = replay_fork_event,
};
unsigned int i;
- int ret;
+ int ret = 0;
+ mutex_init(&sched.start_work_mutex);
+ mutex_init(&sched.work_done_wait_mutex);
for (i = 0; i < ARRAY_SIZE(sched.curr_pid); i++)
sched.curr_pid[i] = -1;
@@ -3558,11 +3551,10 @@ int cmd_sched(int argc, const char **argv)
/*
* Aliased to 'perf script' for now:
*/
- if (!strcmp(argv[0], "script"))
- return cmd_script(argc, argv);
-
- if (strlen(argv[0]) > 2 && strstarts("record", argv[0])) {
- return __cmd_record(argc, argv);
+ if (!strcmp(argv[0], "script")) {
+ ret = cmd_script(argc, argv);
+ } else if (strlen(argv[0]) > 2 && strstarts("record", argv[0])) {
+ ret = __cmd_record(argc, argv);
} else if (strlen(argv[0]) > 2 && strstarts("latency", argv[0])) {
sched.tp_handler = &lat_ops;
if (argc > 1) {
@@ -3571,7 +3563,7 @@ int cmd_sched(int argc, const char **argv)
usage_with_options(latency_usage, latency_options);
}
setup_sorting(&sched, latency_options, latency_usage);
- return perf_sched__lat(&sched);
+ ret = perf_sched__lat(&sched);
} else if (!strcmp(argv[0], "map")) {
if (argc) {
argc = parse_options(argc, argv, map_options, map_usage, 0);
@@ -3580,7 +3572,7 @@ int cmd_sched(int argc, const char **argv)
}
sched.tp_handler = &map_ops;
setup_sorting(&sched, latency_options, latency_usage);
- return perf_sched__map(&sched);
+ ret = perf_sched__map(&sched);
} else if (strlen(argv[0]) > 2 && strstarts("replay", argv[0])) {
sched.tp_handler = &replay_ops;
if (argc) {
@@ -3588,7 +3580,7 @@ int cmd_sched(int argc, const char **argv)
if (argc)
usage_with_options(replay_usage, replay_options);
}
- return perf_sched__replay(&sched);
+ ret = perf_sched__replay(&sched);
} else if (!strcmp(argv[0], "timehist")) {
if (argc) {
argc = parse_options(argc, argv, timehist_options,
@@ -3604,16 +3596,21 @@ int cmd_sched(int argc, const char **argv)
parse_options_usage(NULL, timehist_options, "w", true);
if (sched.show_next)
parse_options_usage(NULL, timehist_options, "n", true);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
ret = symbol__validate_sym_arguments();
if (ret)
- return ret;
+ goto out;
- return perf_sched__timehist(&sched);
+ ret = perf_sched__timehist(&sched);
} else {
usage_with_options(sched_usage, sched_options);
}
- return 0;
+out:
+ mutex_destroy(&sched.start_work_mutex);
+ mutex_destroy(&sched.work_done_wait_mutex);
+
+ return ret;
}
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-top.c | 8 ++++----
tools/perf/util/hist.c | 6 +++---
tools/perf/util/hist.h | 4 ++--
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index fd8fd913c533..14e60f6f219c 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -220,7 +220,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
* This function is now called with he->hists->lock held.
* Release it before going to sleep.
*/
- pthread_mutex_unlock(&he->hists->lock);
+ mutex_unlock(&he->hists->lock);
if (err == -ERANGE && !he->ms.map->erange_warned)
ui__warn_map_erange(he->ms.map, sym, ip);
@@ -230,7 +230,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
sleep(1);
}
- pthread_mutex_lock(&he->hists->lock);
+ mutex_lock(&he->hists->lock);
}
}
@@ -836,12 +836,12 @@ static void perf_event__process_sample(struct perf_tool *tool,
else
iter.ops = &hist_iter_normal;
- pthread_mutex_lock(&hists->lock);
+ mutex_lock(&hists->lock);
if (hist_entry_iter__add(&iter, &al, top->max_stack, top) < 0)
pr_err("Problem incrementing symbol period, skipping event\n");
- pthread_mutex_unlock(&hists->lock);
+ mutex_unlock(&hists->lock);
}
addr_location__put(&al);
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1c085ab56534..698add038cec 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1622,13 +1622,13 @@ struct rb_root_cached *hists__get_rotate_entries_in(struct hists *hists)
{
struct rb_root_cached *root;
- pthread_mutex_lock(&hists->lock);
+ mutex_lock(&hists->lock);
root = hists->entries_in;
if (++hists->entries_in > &hists->entries_in_array[1])
hists->entries_in = &hists->entries_in_array[0];
- pthread_mutex_unlock(&hists->lock);
+ mutex_unlock(&hists->lock);
return root;
}
@@ -2805,7 +2805,7 @@ int __hists__init(struct hists *hists, struct perf_hpp_list *hpp_list)
hists->entries_in = &hists->entries_in_array[0];
hists->entries_collapsed = RB_ROOT_CACHED;
hists->entries = RB_ROOT_CACHED;
- pthread_mutex_init(&hists->lock, NULL);
+ mutex_init(&hists->lock);
hists->socket_filter = -1;
hists->hpp_list = hpp_list;
INIT_LIST_HEAD(&hists->hpp_formats);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 7ed4648d2fc2..508428b2c1b2 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -4,10 +4,10 @@
#include <linux/rbtree.h>
#include <linux/types.h>
-#include <pthread.h>
#include "evsel.h"
#include "color.h"
#include "events_stats.h"
+#include "mutex.h"
struct hist_entry;
struct hist_entry_ops;
@@ -98,7 +98,7 @@ struct hists {
const struct dso *dso_filter;
const char *uid_filter_str;
const char *symbol_filter_str;
- pthread_mutex_t lock;
+ struct mutex lock;
struct hists_stats stats;
u64 event_stream;
u16 col_len[HISTC_NR_COLS];
--
2.37.2.609.g9ff673ca1a-goog
Add annotations to describe lock behavior.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-top.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 5af3347eedc1..e89208b4ad4b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -196,6 +196,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
struct hist_entry *he,
struct perf_sample *sample,
struct evsel *evsel, u64 ip)
+ EXCLUSIVE_LOCKS_REQUIRED(he->hists->lock)
{
struct annotation *notes;
struct symbol *sym = he->ms.sym;
@@ -724,13 +725,13 @@ static void *display_thread(void *arg)
static int hist_iter__top_callback(struct hist_entry_iter *iter,
struct addr_location *al, bool single,
void *arg)
+ EXCLUSIVE_LOCKS_REQUIRED(iter->he->hists->lock)
{
struct perf_top *top = arg;
- struct hist_entry *he = iter->he;
struct evsel *evsel = iter->evsel;
if (perf_hpp_list.sym && single)
- perf_top__record_precise_ip(top, he, iter->sample, evsel, al->addr);
+ perf_top__record_precise_ip(top, iter->he, iter->sample, evsel, al->addr);
hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
!(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY),
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking
for synth_lock.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-record.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 4713f0f3a6cf..a7b7a317d81b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -21,6 +21,7 @@
#include "util/evsel.h"
#include "util/debug.h"
#include "util/mmap.h"
+#include "util/mutex.h"
#include "util/target.h"
#include "util/session.h"
#include "util/tool.h"
@@ -608,17 +609,18 @@ static int process_synthesized_event(struct perf_tool *tool,
return record__write(rec, NULL, event, event->header.size);
}
+static struct mutex synth_lock;
+
static int process_locked_synthesized_event(struct perf_tool *tool,
union perf_event *event,
struct perf_sample *sample __maybe_unused,
struct machine *machine __maybe_unused)
{
- static pthread_mutex_t synth_lock = PTHREAD_MUTEX_INITIALIZER;
int ret;
- pthread_mutex_lock(&synth_lock);
+ mutex_lock(&synth_lock);
ret = process_synthesized_event(tool, event, sample, machine);
- pthread_mutex_unlock(&synth_lock);
+ mutex_unlock(&synth_lock);
return ret;
}
@@ -1917,6 +1919,7 @@ static int record__synthesize(struct record *rec, bool tail)
}
if (rec->opts.nr_threads_synthesize > 1) {
+ mutex_init(&synth_lock);
perf_set_multithreaded();
f = process_locked_synthesized_event;
}
@@ -1930,8 +1933,10 @@ static int record__synthesize(struct record *rec, bool tail)
rec->opts.nr_threads_synthesize);
}
- if (rec->opts.nr_threads_synthesize > 1)
+ if (rec->opts.nr_threads_synthesize > 1) {
perf_set_singlethreaded();
+ mutex_destroy(&synth_lock);
+ }
out:
return err;
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/ui/browser.c | 20 ++++++++++----------
tools/perf/ui/browsers/annotate.c | 2 +-
tools/perf/ui/setup.c | 5 +++--
tools/perf/ui/tui/helpline.c | 5 ++---
tools/perf/ui/tui/progress.c | 8 ++++----
tools/perf/ui/tui/setup.c | 8 ++++----
tools/perf/ui/tui/util.c | 18 +++++++++---------
tools/perf/ui/ui.h | 4 ++--
8 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index fa5bd5c20e96..78fb01d6ad63 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
void ui_browser__show_title(struct ui_browser *browser, const char *title)
{
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
__ui_browser__show_title(browser, title);
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
int ui_browser__show(struct ui_browser *browser, const char *title,
@@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
browser->refresh_dimensions(browser);
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
__ui_browser__show_title(browser, title);
browser->title = title;
@@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
va_end(ap);
if (err > 0)
ui_helpline__push(browser->helpline);
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
return err ? 0 : -1;
}
void ui_browser__hide(struct ui_browser *browser)
{
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
ui_helpline__pop();
zfree(&browser->helpline);
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
static void ui_browser__scrollbar_set(struct ui_browser *browser)
@@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
int ui_browser__refresh(struct ui_browser *browser)
{
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
__ui_browser__refresh(browser);
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
return 0;
}
@@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
while (1) {
off_t offset;
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
err = __ui_browser__refresh(browser);
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
if (err < 0)
break;
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 44ba900828f6..b8747e8dd9ea 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -8,11 +8,11 @@
#include "../../util/hist.h"
#include "../../util/sort.h"
#include "../../util/map.h"
+#include "../../util/mutex.h"
#include "../../util/symbol.h"
#include "../../util/evsel.h"
#include "../../util/evlist.h"
#include <inttypes.h>
-#include <pthread.h>
#include <linux/kernel.h>
#include <linux/string.h>
#include <linux/zalloc.h>
diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
index 700335cde618..25ded88801a3 100644
--- a/tools/perf/ui/setup.c
+++ b/tools/perf/ui/setup.c
@@ -1,5 +1,4 @@
// SPDX-License-Identifier: GPL-2.0
-#include <pthread.h>
#include <dlfcn.h>
#include <unistd.h>
@@ -8,7 +7,7 @@
#include "../util/hist.h"
#include "ui.h"
-pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
+struct mutex ui__lock;
void *perf_gtk_handle;
int use_browser = -1;
@@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
void setup_browser(bool fallback_to_pager)
{
+ mutex_init(&ui__lock);
if (use_browser < 2 && (!isatty(1) || dump_trace))
use_browser = 0;
@@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
default:
break;
}
+ mutex_destroy(&ui__lock);
}
diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
index 298d6af82fdd..db4952f5990b 100644
--- a/tools/perf/ui/tui/helpline.c
+++ b/tools/perf/ui/tui/helpline.c
@@ -2,7 +2,6 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
-#include <pthread.h>
#include <linux/kernel.h>
#include <linux/string.h>
@@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
int ret;
static int backlog;
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
ret = vscnprintf(ui_helpline__last_msg + backlog,
sizeof(ui_helpline__last_msg) - backlog, format, ap);
backlog += ret;
@@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
SLsmg_refresh();
backlog = 0;
}
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
return ret;
}
diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
index 3d74af5a7ece..71b6c8d9474f 100644
--- a/tools/perf/ui/tui/progress.c
+++ b/tools/perf/ui/tui/progress.c
@@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
}
ui__refresh_dimensions(false);
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
y = SLtt_Screen_Rows / 2 - 2;
SLsmg_set_color(0);
SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
@@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
SLsmg_fill_region(y, 1, 1, bar, ' ');
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
static void tui_progress__finish(void)
@@ -67,12 +67,12 @@ static void tui_progress__finish(void)
return;
ui__refresh_dimensions(false);
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
y = SLtt_Screen_Rows / 2 - 2;
SLsmg_set_color(0);
SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
static struct ui_progress_ops tui_progress__ops = {
diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
index b1be59b4e2a4..a3b8c397c24d 100644
--- a/tools/perf/ui/tui/setup.c
+++ b/tools/perf/ui/tui/setup.c
@@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
{
if (force || ui__need_resize) {
ui__need_resize = 0;
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
SLtt_get_screen_size();
SLsmg_reinit_smg();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
}
@@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
"Press any key...", 0);
SLtt_set_cursor_visibility(1);
- if (!pthread_mutex_trylock(&ui__lock)) {
+ if (mutex_trylock(&ui__lock)) {
SLsmg_refresh();
SLsmg_reset_smg();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
SLang_reset_tty();
perf_error__unregister(&perf_tui_eops);
diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
index 0f562e2cb1e8..3c5174854ac8 100644
--- a/tools/perf/ui/tui/util.c
+++ b/tools/perf/ui/tui/util.c
@@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
t = sep + 1;
}
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
max_len += 2;
nr_lines += 8;
@@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
SLsmg_write_nstring((char *)exit_msg, max_len);
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
x += 2;
len = 0;
key = ui__getch(delay_secs);
while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
if (key == K_BKSPC) {
if (len == 0) {
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
goto next_key;
}
SLsmg_gotorc(y, x + --len);
@@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
}
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
/* XXX more graceful overflow handling needed */
if (len == sizeof(buf) - 1) {
@@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
void ui__info_window(const char *title, const char *text)
{
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
__ui__info_window(title, text, NULL);
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
}
int ui__question_window(const char *title, const char *text,
const char *exit_msg, int delay_secs)
{
- pthread_mutex_lock(&ui__lock);
+ mutex_lock(&ui__lock);
__ui__info_window(title, text, exit_msg);
SLsmg_refresh();
- pthread_mutex_unlock(&ui__lock);
+ mutex_unlock(&ui__lock);
return ui__getch(delay_secs);
}
diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
index 9b6fdf06e1d2..99f8d2fe9bc5 100644
--- a/tools/perf/ui/ui.h
+++ b/tools/perf/ui/ui.h
@@ -2,11 +2,11 @@
#ifndef _PERF_UI_H_
#define _PERF_UI_H_ 1
-#include <pthread.h>
+#include "../util/mutex.h"
#include <stdbool.h>
#include <linux/compiler.h>
-extern pthread_mutex_t ui__lock;
+extern struct mutex ui__lock;
extern void *perf_gtk_handle;
extern int use_browser;
--
2.37.2.609.g9ff673ca1a-goog
pthread.h is being included for the side-effect of getting sched.h and
macros like CPU_CLR. Switch to directly using sched.h, or if that is
already present, just remove the pthread.h inclusion entirely.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/tests/mmap-basic.c | 2 --
tools/perf/tests/openat-syscall-all-cpus.c | 2 +-
tools/perf/tests/perf-record.c | 2 --
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
index dfb6173b2a82..21b5e68179d7 100644
--- a/tools/perf/tests/mmap-basic.c
+++ b/tools/perf/tests/mmap-basic.c
@@ -1,8 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <errno.h>
#include <inttypes.h>
-/* For the CLR_() macros */
-#include <pthread.h>
#include <stdlib.h>
#include <perf/cpumap.h>
diff --git a/tools/perf/tests/openat-syscall-all-cpus.c b/tools/perf/tests/openat-syscall-all-cpus.c
index 90828ae03ef5..f3275be83a33 100644
--- a/tools/perf/tests/openat-syscall-all-cpus.c
+++ b/tools/perf/tests/openat-syscall-all-cpus.c
@@ -2,7 +2,7 @@
#include <errno.h>
#include <inttypes.h>
/* For the CPU_* macros */
-#include <pthread.h>
+#include <sched.h>
#include <sys/types.h>
#include <sys/stat.h>
diff --git a/tools/perf/tests/perf-record.c b/tools/perf/tests/perf-record.c
index 6a001fcfed68..b386ade9ed06 100644
--- a/tools/perf/tests/perf-record.c
+++ b/tools/perf/tests/perf-record.c
@@ -2,8 +2,6 @@
#include <errno.h>
#include <inttypes.h>
#include <linux/string.h>
-/* For the CLR_() macros */
-#include <pthread.h>
#include <sched.h>
#include <perf/mmap.h>
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/bench/epoll-ctl.c | 33 ++++-----
tools/perf/bench/epoll-wait.c | 33 ++++-----
tools/perf/bench/futex-hash.c | 33 ++++-----
tools/perf/bench/futex-lock-pi.c | 33 ++++-----
tools/perf/bench/futex-requeue.c | 33 ++++-----
tools/perf/bench/futex-wake-parallel.c | 33 ++++-----
tools/perf/bench/futex-wake.c | 33 ++++-----
tools/perf/bench/numa.c | 93 ++++++++++----------------
8 files changed, 153 insertions(+), 171 deletions(-)
diff --git a/tools/perf/bench/epoll-ctl.c b/tools/perf/bench/epoll-ctl.c
index 4256dc5d6236..521d1ff97b06 100644
--- a/tools/perf/bench/epoll-ctl.c
+++ b/tools/perf/bench/epoll-ctl.c
@@ -23,6 +23,7 @@
#include <sys/eventfd.h>
#include <perf/cpumap.h>
+#include "../util/mutex.h"
#include "../util/stat.h"
#include <subcmd/parse-options.h>
#include "bench.h"
@@ -58,10 +59,10 @@ static unsigned int nested = 0;
/* amount of fds to monitor, per thread */
static unsigned int nfds = 64;
-static pthread_mutex_t thread_lock;
+static struct mutex thread_lock;
static unsigned int threads_starting;
static struct stats all_stats[EPOLL_NR_OPS];
-static pthread_cond_t thread_parent, thread_worker;
+static struct cond thread_parent, thread_worker;
struct worker {
int tid;
@@ -174,12 +175,12 @@ static void *workerfn(void *arg)
struct timespec ts = { .tv_sec = 0,
.tv_nsec = 250 };
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
/* Let 'em loose */
do {
@@ -367,9 +368,9 @@ int bench_epoll_ctl(int argc, const char **argv)
for (i = 0; i < EPOLL_NR_OPS; i++)
init_stats(&all_stats[i]);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
threads_starting = nthreads;
@@ -377,11 +378,11 @@ int bench_epoll_ctl(int argc, const char **argv)
do_threads(worker, cpu);
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
sleep(nsecs);
toggle_done(0, NULL, NULL);
@@ -394,9 +395,9 @@ int bench_epoll_ctl(int argc, const char **argv)
}
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
for (i = 0; i < nthreads; i++) {
unsigned long t[EPOLL_NR_OPS];
diff --git a/tools/perf/bench/epoll-wait.c b/tools/perf/bench/epoll-wait.c
index 2728b0140853..c1cdf03c075d 100644
--- a/tools/perf/bench/epoll-wait.c
+++ b/tools/perf/bench/epoll-wait.c
@@ -79,6 +79,7 @@
#include <perf/cpumap.h>
#include "../util/stat.h"
+#include "../util/mutex.h"
#include <subcmd/parse-options.h>
#include "bench.h"
@@ -109,10 +110,10 @@ static bool multiq; /* use an epoll instance per thread */
/* amount of fds to monitor, per thread */
static unsigned int nfds = 64;
-static pthread_mutex_t thread_lock;
+static struct mutex thread_lock;
static unsigned int threads_starting;
static struct stats throughput_stats;
-static pthread_cond_t thread_parent, thread_worker;
+static struct cond thread_parent, thread_worker;
struct worker {
int tid;
@@ -189,12 +190,12 @@ static void *workerfn(void *arg)
int to = nonblocking? 0 : -1;
int efd = multiq ? w->epollfd : epollfd;
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
do {
/*
@@ -485,9 +486,9 @@ int bench_epoll_wait(int argc, const char **argv)
getpid(), nthreads, oneshot ? " (EPOLLONESHOT semantics)": "", nfds, nsecs);
init_stats(&throughput_stats);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
threads_starting = nthreads;
@@ -495,11 +496,11 @@ int bench_epoll_wait(int argc, const char **argv)
do_threads(worker, cpu);
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
/*
* At this point the workers should be blocked waiting for read events
@@ -522,9 +523,9 @@ int bench_epoll_wait(int argc, const char **argv)
err(EXIT_FAILURE, "pthread_join");
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
/* sort the array back before reporting */
if (randomize)
diff --git a/tools/perf/bench/futex-hash.c b/tools/perf/bench/futex-hash.c
index f05db4cf983d..2005a3fa3026 100644
--- a/tools/perf/bench/futex-hash.c
+++ b/tools/perf/bench/futex-hash.c
@@ -23,6 +23,7 @@
#include <sys/mman.h>
#include <perf/cpumap.h>
+#include "../util/mutex.h"
#include "../util/stat.h"
#include <subcmd/parse-options.h>
#include "bench.h"
@@ -34,10 +35,10 @@ static bool done = false;
static int futex_flag = 0;
struct timeval bench__start, bench__end, bench__runtime;
-static pthread_mutex_t thread_lock;
+static struct mutex thread_lock;
static unsigned int threads_starting;
static struct stats throughput_stats;
-static pthread_cond_t thread_parent, thread_worker;
+static struct cond thread_parent, thread_worker;
struct worker {
int tid;
@@ -73,12 +74,12 @@ static void *workerfn(void *arg)
unsigned int i;
unsigned long ops = w->ops; /* avoid cacheline bouncing */
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
do {
for (i = 0; i < params.nfutexes; i++, ops++) {
@@ -165,9 +166,9 @@ int bench_futex_hash(int argc, const char **argv)
getpid(), params.nthreads, params.nfutexes, params.fshared ? "shared":"private", params.runtime);
init_stats(&throughput_stats);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
threads_starting = params.nthreads;
pthread_attr_init(&thread_attr);
@@ -203,11 +204,11 @@ int bench_futex_hash(int argc, const char **argv)
CPU_FREE(cpuset);
pthread_attr_destroy(&thread_attr);
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
sleep(params.runtime);
toggle_done(0, NULL, NULL);
@@ -219,9 +220,9 @@ int bench_futex_hash(int argc, const char **argv)
}
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
for (i = 0; i < params.nthreads; i++) {
unsigned long t = bench__runtime.tv_sec > 0 ?
diff --git a/tools/perf/bench/futex-lock-pi.c b/tools/perf/bench/futex-lock-pi.c
index 0abb3f7ee24f..2d0417949727 100644
--- a/tools/perf/bench/futex-lock-pi.c
+++ b/tools/perf/bench/futex-lock-pi.c
@@ -8,6 +8,7 @@
#include <pthread.h>
#include <signal.h>
+#include "../util/mutex.h"
#include "../util/stat.h"
#include <subcmd/parse-options.h>
#include <linux/compiler.h>
@@ -34,10 +35,10 @@ static u_int32_t global_futex = 0;
static struct worker *worker;
static bool done = false;
static int futex_flag = 0;
-static pthread_mutex_t thread_lock;
+static struct mutex thread_lock;
static unsigned int threads_starting;
static struct stats throughput_stats;
-static pthread_cond_t thread_parent, thread_worker;
+static struct cond thread_parent, thread_worker;
static struct bench_futex_parameters params = {
.runtime = 10,
@@ -83,12 +84,12 @@ static void *workerfn(void *arg)
struct worker *w = (struct worker *) arg;
unsigned long ops = w->ops;
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
do {
int ret;
@@ -197,9 +198,9 @@ int bench_futex_lock_pi(int argc, const char **argv)
getpid(), params.nthreads, params.runtime);
init_stats(&throughput_stats);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
threads_starting = params.nthreads;
pthread_attr_init(&thread_attr);
@@ -208,11 +209,11 @@ int bench_futex_lock_pi(int argc, const char **argv)
create_threads(worker, thread_attr, cpu);
pthread_attr_destroy(&thread_attr);
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
sleep(params.runtime);
toggle_done(0, NULL, NULL);
@@ -224,9 +225,9 @@ int bench_futex_lock_pi(int argc, const char **argv)
}
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
for (i = 0; i < params.nthreads; i++) {
unsigned long t = bench__runtime.tv_sec > 0 ?
diff --git a/tools/perf/bench/futex-requeue.c b/tools/perf/bench/futex-requeue.c
index b6faabfafb8e..69ad896f556c 100644
--- a/tools/perf/bench/futex-requeue.c
+++ b/tools/perf/bench/futex-requeue.c
@@ -15,6 +15,7 @@
#include <pthread.h>
#include <signal.h>
+#include "../util/mutex.h"
#include "../util/stat.h"
#include <subcmd/parse-options.h>
#include <linux/compiler.h>
@@ -34,8 +35,8 @@ static u_int32_t futex1 = 0, futex2 = 0;
static pthread_t *worker;
static bool done = false;
-static pthread_mutex_t thread_lock;
-static pthread_cond_t thread_parent, thread_worker;
+static struct mutex thread_lock;
+static struct cond thread_parent, thread_worker;
static struct stats requeuetime_stats, requeued_stats;
static unsigned int threads_starting;
static int futex_flag = 0;
@@ -82,12 +83,12 @@ static void *workerfn(void *arg __maybe_unused)
{
int ret;
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
while (1) {
if (!params.pi) {
@@ -209,9 +210,9 @@ int bench_futex_requeue(int argc, const char **argv)
init_stats(&requeued_stats);
init_stats(&requeuetime_stats);
pthread_attr_init(&thread_attr);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
for (j = 0; j < bench_repeat && !done; j++) {
unsigned int nrequeued = 0, wakeups = 0;
@@ -221,11 +222,11 @@ int bench_futex_requeue(int argc, const char **argv)
block_threads(worker, thread_attr, cpu);
/* make sure all threads are already blocked */
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
usleep(100000);
@@ -297,9 +298,9 @@ int bench_futex_requeue(int argc, const char **argv)
}
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
pthread_attr_destroy(&thread_attr);
print_summary();
diff --git a/tools/perf/bench/futex-wake-parallel.c b/tools/perf/bench/futex-wake-parallel.c
index e47f46a3a47e..6682e49d0ee0 100644
--- a/tools/perf/bench/futex-wake-parallel.c
+++ b/tools/perf/bench/futex-wake-parallel.c
@@ -10,6 +10,7 @@
#include "bench.h"
#include <linux/compiler.h>
#include "../util/debug.h"
+#include "../util/mutex.h"
#ifndef HAVE_PTHREAD_BARRIER
int bench_futex_wake_parallel(int argc __maybe_unused, const char **argv __maybe_unused)
@@ -49,8 +50,8 @@ static u_int32_t futex = 0;
static pthread_t *blocked_worker;
static bool done = false;
-static pthread_mutex_t thread_lock;
-static pthread_cond_t thread_parent, thread_worker;
+static struct mutex thread_lock;
+static struct cond thread_parent, thread_worker;
static pthread_barrier_t barrier;
static struct stats waketime_stats, wakeup_stats;
static unsigned int threads_starting;
@@ -125,12 +126,12 @@ static void wakeup_threads(struct thread_data *td, pthread_attr_t thread_attr)
static void *blocked_workerfn(void *arg __maybe_unused)
{
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
while (1) { /* handle spurious wakeups */
if (futex_wait(&futex, 0, NULL, futex_flag) != EINTR)
@@ -294,9 +295,9 @@ int bench_futex_wake_parallel(int argc, const char **argv)
init_stats(&waketime_stats);
pthread_attr_init(&thread_attr);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
for (j = 0; j < bench_repeat && !done; j++) {
waking_worker = calloc(params.nwakes, sizeof(*waking_worker));
@@ -307,11 +308,11 @@ int bench_futex_wake_parallel(int argc, const char **argv)
block_threads(blocked_worker, thread_attr, cpu);
/* make sure all threads are already blocked */
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
usleep(100000);
@@ -332,9 +333,9 @@ int bench_futex_wake_parallel(int argc, const char **argv)
}
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
pthread_attr_destroy(&thread_attr);
print_summary();
diff --git a/tools/perf/bench/futex-wake.c b/tools/perf/bench/futex-wake.c
index 201a3555f09a..9ecab6620a87 100644
--- a/tools/perf/bench/futex-wake.c
+++ b/tools/perf/bench/futex-wake.c
@@ -14,6 +14,7 @@
#include <pthread.h>
#include <signal.h>
+#include "../util/mutex.h"
#include "../util/stat.h"
#include <subcmd/parse-options.h>
#include <linux/compiler.h>
@@ -34,8 +35,8 @@ static u_int32_t futex1 = 0;
static pthread_t *worker;
static bool done = false;
-static pthread_mutex_t thread_lock;
-static pthread_cond_t thread_parent, thread_worker;
+static struct mutex thread_lock;
+static struct cond thread_parent, thread_worker;
static struct stats waketime_stats, wakeup_stats;
static unsigned int threads_starting;
static int futex_flag = 0;
@@ -65,12 +66,12 @@ static const char * const bench_futex_wake_usage[] = {
static void *workerfn(void *arg __maybe_unused)
{
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
threads_starting--;
if (!threads_starting)
- pthread_cond_signal(&thread_parent);
- pthread_cond_wait(&thread_worker, &thread_lock);
- pthread_mutex_unlock(&thread_lock);
+ cond_signal(&thread_parent);
+ cond_wait(&thread_worker, &thread_lock);
+ mutex_unlock(&thread_lock);
while (1) {
if (futex_wait(&futex1, 0, NULL, futex_flag) != EINTR)
@@ -178,9 +179,9 @@ int bench_futex_wake(int argc, const char **argv)
init_stats(&wakeup_stats);
init_stats(&waketime_stats);
pthread_attr_init(&thread_attr);
- pthread_mutex_init(&thread_lock, NULL);
- pthread_cond_init(&thread_parent, NULL);
- pthread_cond_init(&thread_worker, NULL);
+ mutex_init(&thread_lock);
+ cond_init(&thread_parent);
+ cond_init(&thread_worker);
for (j = 0; j < bench_repeat && !done; j++) {
unsigned int nwoken = 0;
@@ -190,11 +191,11 @@ int bench_futex_wake(int argc, const char **argv)
block_threads(worker, thread_attr, cpu);
/* make sure all threads are already blocked */
- pthread_mutex_lock(&thread_lock);
+ mutex_lock(&thread_lock);
while (threads_starting)
- pthread_cond_wait(&thread_parent, &thread_lock);
- pthread_cond_broadcast(&thread_worker);
- pthread_mutex_unlock(&thread_lock);
+ cond_wait(&thread_parent, &thread_lock);
+ cond_broadcast(&thread_worker);
+ mutex_unlock(&thread_lock);
usleep(100000);
@@ -224,9 +225,9 @@ int bench_futex_wake(int argc, const char **argv)
}
/* cleanup & report results */
- pthread_cond_destroy(&thread_parent);
- pthread_cond_destroy(&thread_worker);
- pthread_mutex_destroy(&thread_lock);
+ cond_destroy(&thread_parent);
+ cond_destroy(&thread_worker);
+ mutex_destroy(&thread_lock);
pthread_attr_destroy(&thread_attr);
print_summary();
diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
index 20eed1e53f80..e78dedf9e682 100644
--- a/tools/perf/bench/numa.c
+++ b/tools/perf/bench/numa.c
@@ -6,8 +6,6 @@
*/
#include <inttypes.h>
-/* For the CLR_() macros */
-#include <pthread.h>
#include <subcmd/parse-options.h>
#include "../util/cloexec.h"
@@ -35,6 +33,7 @@
#include <linux/zalloc.h>
#include "../util/header.h"
+#include "../util/mutex.h"
#include <numa.h>
#include <numaif.h>
@@ -67,7 +66,7 @@ struct thread_data {
u64 system_time_ns;
u64 user_time_ns;
double speed_gbs;
- pthread_mutex_t *process_lock;
+ struct mutex *process_lock;
};
/* Parameters set by options: */
@@ -137,16 +136,16 @@ struct params {
struct global_info {
u8 *data;
- pthread_mutex_t startup_mutex;
- pthread_cond_t startup_cond;
+ struct mutex startup_mutex;
+ struct cond startup_cond;
int nr_tasks_started;
- pthread_mutex_t start_work_mutex;
- pthread_cond_t start_work_cond;
+ struct mutex start_work_mutex;
+ struct cond start_work_cond;
int nr_tasks_working;
bool start_work;
- pthread_mutex_t stop_work_mutex;
+ struct mutex stop_work_mutex;
u64 bytes_done;
struct thread_data *threads;
@@ -524,30 +523,6 @@ static void * setup_private_data(ssize_t bytes)
return alloc_data(bytes, MAP_PRIVATE, 0, g->p.init_cpu0, g->p.thp, g->p.init_random);
}
-/*
- * Return a process-shared (global) mutex:
- */
-static void init_global_mutex(pthread_mutex_t *mutex)
-{
- pthread_mutexattr_t attr;
-
- pthread_mutexattr_init(&attr);
- pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
- pthread_mutex_init(mutex, &attr);
-}
-
-/*
- * Return a process-shared (global) condition variable:
- */
-static void init_global_cond(pthread_cond_t *cond)
-{
- pthread_condattr_t attr;
-
- pthread_condattr_init(&attr);
- pthread_condattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
- pthread_cond_init(cond, &attr);
-}
-
static int parse_cpu_list(const char *arg)
{
p0.cpu_list_str = strdup(arg);
@@ -1220,22 +1195,22 @@ static void *worker_thread(void *__tdata)
}
if (g->p.serialize_startup) {
- pthread_mutex_lock(&g->startup_mutex);
+ mutex_lock(&g->startup_mutex);
g->nr_tasks_started++;
/* The last thread wakes the main process. */
if (g->nr_tasks_started == g->p.nr_tasks)
- pthread_cond_signal(&g->startup_cond);
+ cond_signal(&g->startup_cond);
- pthread_mutex_unlock(&g->startup_mutex);
+ mutex_unlock(&g->startup_mutex);
/* Here we will wait for the main process to start us all at once: */
- pthread_mutex_lock(&g->start_work_mutex);
+ mutex_lock(&g->start_work_mutex);
g->start_work = false;
g->nr_tasks_working++;
while (!g->start_work)
- pthread_cond_wait(&g->start_work_cond, &g->start_work_mutex);
+ cond_wait(&g->start_work_cond, &g->start_work_mutex);
- pthread_mutex_unlock(&g->start_work_mutex);
+ mutex_unlock(&g->start_work_mutex);
}
gettimeofday(&start0, NULL);
@@ -1254,17 +1229,17 @@ static void *worker_thread(void *__tdata)
val += do_work(thread_data, g->p.bytes_thread, 0, 1, l, val);
if (g->p.sleep_usecs) {
- pthread_mutex_lock(td->process_lock);
+ mutex_lock(td->process_lock);
usleep(g->p.sleep_usecs);
- pthread_mutex_unlock(td->process_lock);
+ mutex_unlock(td->process_lock);
}
/*
* Amount of work to be done under a process-global lock:
*/
if (g->p.bytes_process_locked) {
- pthread_mutex_lock(td->process_lock);
+ mutex_lock(td->process_lock);
val += do_work(process_data, g->p.bytes_process_locked, thread_nr, g->p.nr_threads, l, val);
- pthread_mutex_unlock(td->process_lock);
+ mutex_unlock(td->process_lock);
}
work_done = g->p.bytes_global + g->p.bytes_process +
@@ -1361,9 +1336,9 @@ static void *worker_thread(void *__tdata)
free_data(thread_data, g->p.bytes_thread);
- pthread_mutex_lock(&g->stop_work_mutex);
+ mutex_lock(&g->stop_work_mutex);
g->bytes_done += bytes_done;
- pthread_mutex_unlock(&g->stop_work_mutex);
+ mutex_unlock(&g->stop_work_mutex);
return NULL;
}
@@ -1373,7 +1348,7 @@ static void *worker_thread(void *__tdata)
*/
static void worker_process(int process_nr)
{
- pthread_mutex_t process_lock;
+ struct mutex process_lock;
struct thread_data *td;
pthread_t *pthreads;
u8 *process_data;
@@ -1381,7 +1356,7 @@ static void worker_process(int process_nr)
int ret;
int t;
- pthread_mutex_init(&process_lock, NULL);
+ mutex_init(&process_lock);
set_taskname("process %d", process_nr);
/*
@@ -1540,11 +1515,11 @@ static int init(void)
g->data = setup_shared_data(g->p.bytes_global);
/* Startup serialization: */
- init_global_mutex(&g->start_work_mutex);
- init_global_cond(&g->start_work_cond);
- init_global_mutex(&g->startup_mutex);
- init_global_cond(&g->startup_cond);
- init_global_mutex(&g->stop_work_mutex);
+ mutex_init_pshared(&g->start_work_mutex);
+ cond_init_pshared(&g->start_work_cond);
+ mutex_init_pshared(&g->startup_mutex);
+ cond_init_pshared(&g->startup_cond);
+ mutex_init_pshared(&g->stop_work_mutex);
init_thread_data();
@@ -1633,17 +1608,17 @@ static int __bench_numa(const char *name)
* Wait for all the threads to start up. The last thread will
* signal this process.
*/
- pthread_mutex_lock(&g->startup_mutex);
+ mutex_lock(&g->startup_mutex);
while (g->nr_tasks_started != g->p.nr_tasks)
- pthread_cond_wait(&g->startup_cond, &g->startup_mutex);
+ cond_wait(&g->startup_cond, &g->startup_mutex);
- pthread_mutex_unlock(&g->startup_mutex);
+ mutex_unlock(&g->startup_mutex);
/* Wait for all threads to be at the start_work_cond. */
while (!threads_ready) {
- pthread_mutex_lock(&g->start_work_mutex);
+ mutex_lock(&g->start_work_mutex);
threads_ready = (g->nr_tasks_working == g->p.nr_tasks);
- pthread_mutex_unlock(&g->start_work_mutex);
+ mutex_unlock(&g->start_work_mutex);
if (!threads_ready)
usleep(1);
}
@@ -1661,10 +1636,10 @@ static int __bench_numa(const char *name)
start = stop;
/* Start all threads running. */
- pthread_mutex_lock(&g->start_work_mutex);
+ mutex_lock(&g->start_work_mutex);
g->start_work = true;
- pthread_mutex_unlock(&g->start_work_mutex);
- pthread_cond_broadcast(&g->start_work_cond);
+ mutex_unlock(&g->start_work_mutex);
+ cond_broadcast(&g->start_work_cond);
} else {
gettimeofday(&start, NULL);
}
--
2.37.2.609.g9ff673ca1a-goog
There may be threads racing to update dso->nsinfo:
https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/
Holding the dso->lock avoids use-after-free, memory leaks and other
such bugs. Apply the fix in:
https://lore.kernel.org/linux-perf-users/[email protected]/
of there being a missing nsinfo__put now that the accesses are data race
free. Fixes test "Lookup mmap thread" when compiled with address
sanitizer.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-inject.c | 4 ++++
tools/perf/util/annotate.c | 2 ++
tools/perf/util/build-id.c | 12 +++++++++---
tools/perf/util/dso.c | 7 ++++++-
tools/perf/util/map.c | 3 +++
tools/perf/util/probe-event.c | 3 +++
tools/perf/util/symbol.c | 2 +-
7 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 8ec955402488..e254f18986f7 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -436,8 +436,10 @@ static struct dso *findnew_dso(int pid, int tid, const char *filename,
}
if (dso) {
+ mutex_lock(&dso->lock);
nsinfo__put(dso->nsinfo);
dso->nsinfo = nsi;
+ mutex_unlock(&dso->lock);
} else
nsinfo__put(nsi);
@@ -620,6 +622,7 @@ static int dso__read_build_id(struct dso *dso)
if (dso->has_build_id)
return 0;
+ mutex_lock(&dso->lock);
nsinfo__mountns_enter(dso->nsinfo, &nsc);
if (filename__read_build_id(dso->long_name, &dso->bid) > 0)
dso->has_build_id = true;
@@ -633,6 +636,7 @@ static int dso__read_build_id(struct dso *dso)
free(new_name);
}
nsinfo__mountns_exit(&nsc);
+ mutex_unlock(&dso->lock);
return dso->has_build_id ? 0 : -1;
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 9d7dd6489a05..5bc63c9e0324 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1697,6 +1697,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
*/
__symbol__join_symfs(filename, filename_size, dso->long_name);
+ mutex_lock(&dso->lock);
if (access(filename, R_OK) && errno == ENOENT && dso->nsinfo) {
char *new_name = filename_with_chroot(dso->nsinfo->pid,
filename);
@@ -1705,6 +1706,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
free(new_name);
}
}
+ mutex_unlock(&dso->lock);
}
free(build_id_path);
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index ec18ed5caf3e..a839b30c981b 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -898,11 +898,15 @@ static int filename__read_build_id_ns(const char *filename,
static bool dso__build_id_mismatch(struct dso *dso, const char *name)
{
struct build_id bid;
+ bool ret = false;
- if (filename__read_build_id_ns(name, &bid, dso->nsinfo) < 0)
- return false;
+ mutex_lock(&dso->lock);
+ if (filename__read_build_id_ns(name, &bid, dso->nsinfo) >= 0)
+ ret = !dso__build_id_equal(dso, &bid);
- return !dso__build_id_equal(dso, &bid);
+ mutex_unlock(&dso->lock);
+
+ return ret;
}
static int dso__cache_build_id(struct dso *dso, struct machine *machine,
@@ -941,8 +945,10 @@ static int dso__cache_build_id(struct dso *dso, struct machine *machine,
if (!is_kallsyms && dso__build_id_mismatch(dso, name))
goto out_free;
+ mutex_lock(&dso->lock);
ret = build_id_cache__add_b(&dso->bid, name, dso->nsinfo,
is_kallsyms, is_vdso, proper_name, root_dir);
+ mutex_unlock(&dso->lock);
out_free:
free(allocated_name);
return ret;
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index a9789a955403..f1a14c0ad26d 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -501,6 +501,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
if (!name)
return -ENOMEM;
+ mutex_lock(&dso->lock);
if (machine)
root_dir = machine->root_dir;
@@ -541,6 +542,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
unlink(name);
out:
+ mutex_unlock(&dso->lock);
free(name);
return fd;
}
@@ -559,8 +561,11 @@ static int open_dso(struct dso *dso, struct machine *machine)
int fd;
struct nscookie nsc;
- if (dso->binary_type != DSO_BINARY_TYPE__BUILD_ID_CACHE)
+ if (dso->binary_type != DSO_BINARY_TYPE__BUILD_ID_CACHE) {
+ mutex_lock(&dso->lock);
nsinfo__mountns_enter(dso->nsinfo, &nsc);
+ mutex_unlock(&dso->lock);
+ }
fd = __open_dso(dso, machine);
if (dso->binary_type != DSO_BINARY_TYPE__BUILD_ID_CACHE)
nsinfo__mountns_exit(&nsc);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index e0aa4a254583..f3a3d9b3a40d 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -181,7 +181,10 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
if (!(prot & PROT_EXEC))
dso__set_loaded(dso);
}
+ mutex_lock(&dso->lock);
+ nsinfo__put(dso->nsinfo);
dso->nsinfo = nsi;
+ mutex_unlock(&dso->lock);
if (build_id__is_defined(bid)) {
dso__set_build_id(dso, bid);
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 785246ff4179..0c24bc7afbca 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -29,6 +29,7 @@
#include "color.h"
#include "map.h"
#include "maps.h"
+#include "mutex.h"
#include "symbol.h"
#include <api/fs/fs.h>
#include "trace-event.h" /* For __maybe_unused */
@@ -180,8 +181,10 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
map = dso__new_map(target);
if (map && map->dso) {
+ mutex_lock(&map->dso->lock);
nsinfo__put(map->dso->nsinfo);
map->dso->nsinfo = nsinfo__get(nsi);
+ mutex_unlock(&map->dso->lock);
}
return map;
} else {
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 656d9b4dd456..a3a165ae933a 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1791,6 +1791,7 @@ int dso__load(struct dso *dso, struct map *map)
char newmapname[PATH_MAX];
const char *map_path = dso->long_name;
+ mutex_lock(&dso->lock);
perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0;
if (perfmap) {
if (dso->nsinfo && (dso__find_perf_map(newmapname,
@@ -1800,7 +1801,6 @@ int dso__load(struct dso *dso, struct map *map)
}
nsinfo__mountns_enter(dso->nsinfo, &nsc);
- mutex_lock(&dso->lock);
/* check again under the dso->lock */
if (dso__loaded(dso)) {
--
2.37.2.609.g9ff673ca1a-goog
Add thread safety annotations to struct mutex so that when compiled with
clang's -Wthread-safety warnings are generated for erroneous lock
patterns. NO_THREAD_SAFETY_ANALYSIS is needed for
mutex_lock/mutex_unlock as the analysis doesn't under pthread calls.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/mutex.c | 2 ++
tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
index 892294ac1769..ec813093276d 100644
--- a/tools/perf/util/mutex.c
+++ b/tools/perf/util/mutex.c
@@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx)
}
void mutex_lock(struct mutex *mtx)
+ NO_THREAD_SAFETY_ANALYSIS
{
CHECK_ERR(pthread_mutex_lock(&mtx->lock));
}
void mutex_unlock(struct mutex *mtx)
+ NO_THREAD_SAFETY_ANALYSIS
{
CHECK_ERR(pthread_mutex_unlock(&mtx->lock));
}
diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
index c9e110a2b55e..48a2d87598f0 100644
--- a/tools/perf/util/mutex.h
+++ b/tools/perf/util/mutex.h
@@ -5,11 +5,73 @@
#include <pthread.h>
#include <stdbool.h>
+/*
+ * A function-like feature checking macro that is a wrapper around
+ * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a
+ * nonzero constant integer if the attribute is supported or 0 if not.
+ */
+#ifdef __has_attribute
+#define HAVE_ATTRIBUTE(x) __has_attribute(x)
+#else
+#define HAVE_ATTRIBUTE(x) 0
+#endif
+
+
+#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \
+ HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \
+ HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \
+ HAVE_ATTRIBUTE(no_thread_safety_analysis)
+
+/* Documents if a shared field or global variable needs to be protected by a mutex. */
+#define GUARDED_BY(x) __attribute__((guarded_by(x)))
+
+/*
+ * Documents if the memory location pointed to by a pointer should be guarded by
+ * a mutex when dereferencing the pointer.
+ */
+#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
+
+/* Documents if a type is a lockable type. */
+#define LOCKABLE __attribute__((capability("lockable")))
+
+/* Documents functions that acquire a lock in the body of a function, and do not release it. */
+#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
+
+/*
+ * Documents functions that expect a lock to be held on entry to the function,
+ * and release it in the body of the function.
+ */
+#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
+
+/* Documents functions that try to acquire a lock, and return success or failure. */
+#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \
+ __attribute__((exclusive_trylock_function(__VA_ARGS__)))
+
+
+/* Documents a function that expects a mutex to be held prior to entry. */
+#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
+
+/* Turns off thread safety checking within the body of a particular function. */
+#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
+
+#else
+
+#define GUARDED_BY(x)
+#define PT_GUARDED_BY(x)
+#define LOCKABLE
+#define EXCLUSIVE_LOCK_FUNCTION(...)
+#define UNLOCK_FUNCTION(...)
+#define EXCLUSIVE_TRYLOCK_FUNCTION(...)
+#define EXCLUSIVE_LOCKS_REQUIRED(...)
+#define NO_THREAD_SAFETY_ANALYSIS
+
+#endif
+
/*
* A wrapper around the mutex implementation that allows perf to error check
* usage, etc.
*/
-struct mutex {
+struct LOCKABLE mutex {
pthread_mutex_t lock;
};
@@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx);
void mutex_init_pshared(struct mutex *mtx);
void mutex_destroy(struct mutex *mtx);
-void mutex_lock(struct mutex *mtx);
-void mutex_unlock(struct mutex *mtx);
-bool mutex_trylock(struct mutex *mtx);
+void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx);
+void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx);
+bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx);
/* Default initialize the cond struct. */
void cond_init(struct cond *cnd);
@@ -40,7 +102,7 @@ void cond_init(struct cond *cnd);
void cond_init_pshared(struct cond *cnd);
void cond_destroy(struct cond *cnd);
-void cond_wait(struct cond *cnd, struct mutex *mtx);
+void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx);
void cond_signal(struct cond *cnd);
void cond_broadcast(struct cond *cnd);
--
2.37.2.609.g9ff673ca1a-goog
Add annotations to describe lock behavior. Add unlocks so that mutexes
aren't conditionally held on exit from perf_sched__replay. Add an exit
variable so that thread_func can terminate, rather than leaving the
threads blocked on mutexes.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 7e4006d6b8bc..b483ff0d432e 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -246,6 +246,7 @@ struct perf_sched {
const char *time_str;
struct perf_time_interval ptime;
struct perf_time_interval hist_time;
+ volatile bool thread_funcs_exit;
};
/* per thread run time data */
@@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
prctl(PR_SET_NAME, comm2);
if (fd < 0)
return NULL;
-again:
- ret = sem_post(&this_task->ready_for_work);
- BUG_ON(ret);
- mutex_lock(&sched->start_work_mutex);
- mutex_unlock(&sched->start_work_mutex);
- cpu_usage_0 = get_cpu_usage_nsec_self(fd);
+ while (!sched->thread_funcs_exit) {
+ ret = sem_post(&this_task->ready_for_work);
+ BUG_ON(ret);
+ mutex_lock(&sched->start_work_mutex);
+ mutex_unlock(&sched->start_work_mutex);
- for (i = 0; i < this_task->nr_events; i++) {
- this_task->curr_event = i;
- perf_sched__process_event(sched, this_task->atoms[i]);
- }
+ cpu_usage_0 = get_cpu_usage_nsec_self(fd);
- cpu_usage_1 = get_cpu_usage_nsec_self(fd);
- this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
- ret = sem_post(&this_task->work_done_sem);
- BUG_ON(ret);
+ for (i = 0; i < this_task->nr_events; i++) {
+ this_task->curr_event = i;
+ perf_sched__process_event(sched, this_task->atoms[i]);
+ }
- mutex_lock(&sched->work_done_wait_mutex);
- mutex_unlock(&sched->work_done_wait_mutex);
+ cpu_usage_1 = get_cpu_usage_nsec_self(fd);
+ this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
+ ret = sem_post(&this_task->work_done_sem);
+ BUG_ON(ret);
- goto again;
+ mutex_lock(&sched->work_done_wait_mutex);
+ mutex_unlock(&sched->work_done_wait_mutex);
+ }
+ return NULL;
}
static void create_tasks(struct perf_sched *sched)
+ EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
+ EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
{
struct task_desc *task;
pthread_attr_t attr;
@@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
}
static void wait_for_tasks(struct perf_sched *sched)
+ EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
+ EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
{
u64 cpu_usage_0, cpu_usage_1;
struct task_desc *task;
@@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
}
static void run_one_test(struct perf_sched *sched)
+ EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
+ EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
{
u64 T0, T1, delta, avg_delta, fluct;
@@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
print_task_traces(sched);
add_cross_task_wakeups(sched);
+ sched->thread_funcs_exit = false;
create_tasks(sched);
printf("------------------------------------------------------------\n");
for (i = 0; i < sched->replay_repeat; i++)
run_one_test(sched);
+ sched->thread_funcs_exit = true;
+ mutex_unlock(&sched->start_work_mutex);
+ mutex_unlock(&sched->work_done_wait_mutex);
return 0;
}
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-top.c | 18 +++++++++---------
tools/perf/util/top.h | 5 +++--
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index b96bb9a23ac0..5af3347eedc1 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -893,10 +893,10 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
perf_mmap__consume(&md->core);
if (top->qe.rotate) {
- pthread_mutex_lock(&top->qe.mutex);
+ mutex_lock(&top->qe.mutex);
top->qe.rotate = false;
- pthread_cond_signal(&top->qe.cond);
- pthread_mutex_unlock(&top->qe.mutex);
+ cond_signal(&top->qe.cond);
+ mutex_unlock(&top->qe.mutex);
}
}
@@ -1100,10 +1100,10 @@ static void *process_thread(void *arg)
out = rotate_queues(top);
- pthread_mutex_lock(&top->qe.mutex);
+ mutex_lock(&top->qe.mutex);
top->qe.rotate = true;
- pthread_cond_wait(&top->qe.cond, &top->qe.mutex);
- pthread_mutex_unlock(&top->qe.mutex);
+ cond_wait(&top->qe.cond, &top->qe.mutex);
+ mutex_unlock(&top->qe.mutex);
if (ordered_events__flush(out, OE_FLUSH__TOP))
pr_err("failed to process events\n");
@@ -1217,8 +1217,8 @@ static void init_process_thread(struct perf_top *top)
ordered_events__set_copy_on_queue(&top->qe.data[0], true);
ordered_events__set_copy_on_queue(&top->qe.data[1], true);
top->qe.in = &top->qe.data[0];
- pthread_mutex_init(&top->qe.mutex, NULL);
- pthread_cond_init(&top->qe.cond, NULL);
+ mutex_init(&top->qe.mutex);
+ cond_init(&top->qe.cond);
}
static int __cmd_top(struct perf_top *top)
@@ -1349,7 +1349,7 @@ static int __cmd_top(struct perf_top *top)
out_join:
pthread_join(thread, NULL);
out_join_thread:
- pthread_cond_signal(&top->qe.cond);
+ cond_signal(&top->qe.cond);
pthread_join(thread_process, NULL);
return ret;
}
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index 1c2c0a838430..a8b0d79bd96c 100644
--- a/tools/perf/util/top.h
+++ b/tools/perf/util/top.h
@@ -5,6 +5,7 @@
#include "tool.h"
#include "evswitch.h"
#include "annotate.h"
+#include "mutex.h"
#include "ordered-events.h"
#include "record.h"
#include <linux/types.h>
@@ -53,8 +54,8 @@ struct perf_top {
struct ordered_events *in;
struct ordered_events data[2];
bool rotate;
- pthread_mutex_t mutex;
- pthread_cond_t cond;
+ struct mutex mutex;
+ struct cond cond;
} qe;
};
--
2.37.2.609.g9ff673ca1a-goog
Switch to the use of mutex wrappers that provide better error checking.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-top.c | 14 +++++++-------
tools/perf/ui/browsers/annotate.c | 10 +++++-----
tools/perf/util/annotate.c | 13 ++++++-------
tools/perf/util/annotate.h | 4 ++--
4 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 14e60f6f219c..b96bb9a23ac0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -136,10 +136,10 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
}
notes = symbol__annotation(sym);
- pthread_mutex_lock(¬es->lock);
+ mutex_lock(¬es->lock);
if (!symbol__hists(sym, top->evlist->core.nr_entries)) {
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
pr_err("Not enough memory for annotating '%s' symbol!\n",
sym->name);
sleep(1);
@@ -155,7 +155,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
pr_err("Couldn't annotate %s: %s\n", sym->name, msg);
}
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
return err;
}
@@ -208,12 +208,12 @@ static void perf_top__record_precise_ip(struct perf_top *top,
notes = symbol__annotation(sym);
- if (pthread_mutex_trylock(¬es->lock))
+ if (!mutex_trylock(¬es->lock))
return;
err = hist_entry__inc_addr_samples(he, sample, evsel, ip);
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
if (unlikely(err)) {
/*
@@ -250,7 +250,7 @@ static void perf_top__show_details(struct perf_top *top)
symbol = he->ms.sym;
notes = symbol__annotation(symbol);
- pthread_mutex_lock(¬es->lock);
+ mutex_lock(¬es->lock);
symbol__calc_percent(symbol, evsel);
@@ -271,7 +271,7 @@ static void perf_top__show_details(struct perf_top *top)
if (more != 0)
printf("%d lines not displayed, maybe increase display entries [e]\n", more);
out_unlock:
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
}
static void perf_top__resort_hists(struct perf_top *t)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index b8747e8dd9ea..9bc1076374ff 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -319,7 +319,7 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
browser->entries = RB_ROOT;
- pthread_mutex_lock(¬es->lock);
+ mutex_lock(¬es->lock);
symbol__calc_percent(sym, evsel);
@@ -348,7 +348,7 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
}
disasm_rb_tree__insert(browser, &pos->al);
}
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
browser->curr_hot = rb_last(&browser->entries);
}
@@ -474,10 +474,10 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
}
notes = symbol__annotation(dl->ops.target.sym);
- pthread_mutex_lock(¬es->lock);
+ mutex_lock(¬es->lock);
if (!symbol__hists(dl->ops.target.sym, evsel->evlist->core.nr_entries)) {
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
ui__warning("Not enough memory for annotating '%s' symbol!\n",
dl->ops.target.sym->name);
return true;
@@ -486,7 +486,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
target_ms.maps = ms->maps;
target_ms.map = ms->map;
target_ms.sym = dl->ops.target.sym;
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
symbol__tui_annotate(&target_ms, evsel, hbt, browser->opts);
sym_title(ms->sym, ms->map, title, sizeof(title), browser->opts->percent_type);
ui_browser__show_title(&browser->b, title);
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 2c6a485c3de5..9d7dd6489a05 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -35,7 +35,6 @@
#include "arch/common.h"
#include "namespaces.h"
#include <regex.h>
-#include <pthread.h>
#include <linux/bitops.h>
#include <linux/kernel.h>
#include <linux/string.h>
@@ -821,7 +820,7 @@ void symbol__annotate_zero_histograms(struct symbol *sym)
{
struct annotation *notes = symbol__annotation(sym);
- pthread_mutex_lock(¬es->lock);
+ mutex_lock(¬es->lock);
if (notes->src != NULL) {
memset(notes->src->histograms, 0,
notes->src->nr_histograms * notes->src->sizeof_sym_hist);
@@ -829,7 +828,7 @@ void symbol__annotate_zero_histograms(struct symbol *sym)
memset(notes->src->cycles_hist, 0,
symbol__size(sym) * sizeof(struct cyc_hist));
}
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
}
static int __symbol__account_cycles(struct cyc_hist *ch,
@@ -1086,7 +1085,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
notes->hit_insn = 0;
notes->cover_insn = 0;
- pthread_mutex_lock(¬es->lock);
+ mutex_lock(¬es->lock);
for (offset = size - 1; offset >= 0; --offset) {
struct cyc_hist *ch;
@@ -1105,7 +1104,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
notes->have_cycles = true;
}
}
- pthread_mutex_unlock(¬es->lock);
+ mutex_unlock(¬es->lock);
}
int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
@@ -1258,13 +1257,13 @@ int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool r
void annotation__init(struct annotation *notes)
{
- pthread_mutex_init(¬es->lock, NULL);
+ mutex_init(¬es->lock);
}
void annotation__exit(struct annotation *notes)
{
annotated_source__delete(notes->src);
- pthread_mutex_destroy(¬es->lock);
+ mutex_destroy(¬es->lock);
}
static void annotation_line__add(struct annotation_line *al, struct list_head *head)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 986f2bbe4870..3cbd883e4d7a 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -8,9 +8,9 @@
#include <linux/types.h>
#include <linux/list.h>
#include <linux/rbtree.h>
-#include <pthread.h>
#include <asm/bug.h>
#include "symbol_conf.h"
+#include "mutex.h"
#include "spark.h"
struct hist_browser_timer;
@@ -273,7 +273,7 @@ struct annotated_source {
};
struct annotation {
- pthread_mutex_t lock;
+ struct mutex lock;
u64 max_coverage;
u64 start;
u64 hit_cycles;
--
2.37.2.609.g9ff673ca1a-goog
No pthread usage in builtin-lock.c.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-lock.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index dd11d3471baf..70197c0593b1 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -28,7 +28,6 @@
#include <sys/types.h>
#include <sys/prctl.h>
#include <semaphore.h>
-#include <pthread.h>
#include <math.h>
#include <limits.h>
--
2.37.2.609.g9ff673ca1a-goog
On 25/08/22 15:30, Arnaldo Carvalho de Melo wrote:
> On Wed, Aug 24, 2022, 12:39 PM Ian Rogers <[email protected] <mailto:[email protected]>> wrote:
>
> When fixing a locking race and memory leak in:
> https://lore.kernel.org/linux-perf-users/[email protected]/ <https://lore.kernel.org/linux-perf-users/[email protected]/>
>
> It was requested that debug mutex code be separated out into its own
> files. This was, in part, done by Pavithra Gurushankar in:
> https://lore.kernel.org/lkml/[email protected]/ <https://lore.kernel.org/lkml/[email protected]/>
>
> These patches fix issues with the previous patches, add in the
> original dso->nsinfo fix and then build on our mutex wrapper with
> clang's -Wthread-safety analysis. The analysis found missing unlocks
> in builtin-sched.c which are fixed and -Wthread-safety is enabled by
> default when building with clang.
>
> v3. Adds a missing new line to the error messages and removes the
> Â Â pshared argument to mutex_init by having two functions, mutex_init
> Â Â and mutex_init_pshared. These changes were suggested by Adrian Hunter.
>
>
> Adrian, can I have your Acked-by or, better, Reviewed-by?
Sure, just let me have another look. Should get to it
tomorrow.
>
> Thanks,Â
>
> -Â ArnaldoÂ
>
> v2. Breaks apart changes that s/pthread_mutex/mutex/g and the lock
> Â Â annotations as requested by Arnaldo and Namhyung. A boolean is
> Â Â added to builtin-sched.c to terminate thread funcs rather than
> Â Â leaving them blocked on delted mutexes.
>
> Ian Rogers (17):
> Â perf bench: Update use of pthread mutex/cond
> Â perf tests: Avoid pthread.h inclusion
> Â perf hist: Update use of pthread mutex
> Â perf bpf: Remove unused pthread.h include
> Â perf lock: Remove unused pthread.h include
> Â perf record: Update use of pthread mutex
> Â perf sched: Update use of pthread mutex
> Â perf ui: Update use of pthread mutex
> Â perf mmap: Remove unnecessary pthread.h include
> Â perf dso: Update use of pthread mutex
> Â perf annotate: Update use of pthread mutex
> Â perf top: Update use of pthread mutex
> Â perf dso: Hold lock when accessing nsinfo
> Â perf mutex: Add thread safety annotations
> Â perf sched: Fixes for thread safety analysis
> Â perf top: Fixes for thread safety analysis
> Â perf build: Enable -Wthread-safety with clang
>
> Pavithra Gurushankar (1):
> Â perf mutex: Wrapped usage of mutex and cond
>
>  tools/perf/Makefile.config         |  5 +
>  tools/perf/bench/epoll-ctl.c        | 33 +++---
>  tools/perf/bench/epoll-wait.c       | 33 +++---
>  tools/perf/bench/futex-hash.c       | 33 +++---
>  tools/perf/bench/futex-lock-pi.c      | 33 +++---
>  tools/perf/bench/futex-requeue.c      | 33 +++---
>  tools/perf/bench/futex-wake-parallel.c   | 33 +++---
>  tools/perf/bench/futex-wake.c       | 33 +++---
>  tools/perf/bench/numa.c          | 93 ++++++----------
>  tools/perf/builtin-inject.c        |  4 +
>  tools/perf/builtin-lock.c         |  1 -
>  tools/perf/builtin-record.c        | 13 ++-
>  tools/perf/builtin-sched.c         | 105 +++++++++---------
>  tools/perf/builtin-top.c          | 45 ++++----
>  tools/perf/tests/mmap-basic.c       |  2 -
> Â tools/perf/tests/openat-syscall-all-cpus.c |Â Â 2 +-
>  tools/perf/tests/perf-record.c       |  2 -
>  tools/perf/ui/browser.c          | 20 ++--
>  tools/perf/ui/browsers/annotate.c     | 12 +--
>  tools/perf/ui/setup.c           |  5 +-
>  tools/perf/ui/tui/helpline.c        |  5 +-
>  tools/perf/ui/tui/progress.c        |  8 +-
>  tools/perf/ui/tui/setup.c         |  8 +-
>  tools/perf/ui/tui/util.c          | 18 ++--
>  tools/perf/ui/ui.h             |  4 +-
>  tools/perf/util/Build           |  1 +
>  tools/perf/util/annotate.c         | 15 +--
>  tools/perf/util/annotate.h         |  4 +-
>  tools/perf/util/bpf-event.h        |  1 -
>  tools/perf/util/build-id.c         | 12 ++-
>  tools/perf/util/dso.c           | 19 ++--
>  tools/perf/util/dso.h           |  4 +-
>  tools/perf/util/hist.c           |  6 +-
>  tools/perf/util/hist.h           |  4 +-
>  tools/perf/util/map.c           |  3 +
>  tools/perf/util/mmap.h           |  1 -
>  tools/perf/util/mutex.c          | 119 +++++++++++++++++++++
>  tools/perf/util/mutex.h          | 109 +++++++++++++++++++
>  tools/perf/util/probe-event.c       |  3 +
>  tools/perf/util/symbol.c          |  4 +-
>  tools/perf/util/top.h           |  5 +-
> Â 41 files changed, 570 insertions(+), 323 deletions(-)
> Â create mode 100644 tools/perf/util/mutex.c
> Â create mode 100644 tools/perf/util/mutex.h
>
> --
> 2.37.2.609.g9ff673ca1a-goog
>
On 24/08/22 18:38, Ian Rogers wrote:
> Switch to the use of mutex wrappers that provide better error checking.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/dso.c | 12 ++++++------
Some not done yet
$ grep -i pthread_mut tools/perf/util/dso.c
static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_lock(&dso__data_open_lock);
pthread_mutex_unlock(&dso__data_open_lock);
if (pthread_mutex_lock(&dso__data_open_lock) < 0)
pthread_mutex_unlock(&dso__data_open_lock);
pthread_mutex_unlock(&dso__data_open_lock);
pthread_mutex_lock(&dso__data_open_lock);
pthread_mutex_unlock(&dso__data_open_lock);
pthread_mutex_lock(&dso__data_open_lock);
pthread_mutex_unlock(&dso__data_open_lock);
> tools/perf/util/dso.h | 4 ++--
> tools/perf/util/symbol.c | 4 ++--
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 5ac13958d1bd..a9789a955403 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -795,7 +795,7 @@ dso_cache__free(struct dso *dso)
> struct rb_root *root = &dso->data.cache;
> struct rb_node *next = rb_first(root);
>
> - pthread_mutex_lock(&dso->lock);
> + mutex_lock(&dso->lock);
> while (next) {
> struct dso_cache *cache;
>
> @@ -804,7 +804,7 @@ dso_cache__free(struct dso *dso)
> rb_erase(&cache->rb_node, root);
> free(cache);
> }
> - pthread_mutex_unlock(&dso->lock);
> + mutex_unlock(&dso->lock);
> }
>
> static struct dso_cache *__dso_cache__find(struct dso *dso, u64 offset)
> @@ -841,7 +841,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
> struct dso_cache *cache;
> u64 offset = new->offset;
>
> - pthread_mutex_lock(&dso->lock);
> + mutex_lock(&dso->lock);
> while (*p != NULL) {
> u64 end;
>
> @@ -862,7 +862,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
>
> cache = NULL;
> out:
> - pthread_mutex_unlock(&dso->lock);
> + mutex_unlock(&dso->lock);
> return cache;
> }
>
> @@ -1297,7 +1297,7 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
> dso->root = NULL;
> INIT_LIST_HEAD(&dso->node);
> INIT_LIST_HEAD(&dso->data.open_entry);
> - pthread_mutex_init(&dso->lock, NULL);
> + mutex_init(&dso->lock);
> refcount_set(&dso->refcnt, 1);
> }
>
> @@ -1336,7 +1336,7 @@ void dso__delete(struct dso *dso)
> dso__free_a2l(dso);
> zfree(&dso->symsrc_filename);
> nsinfo__zput(dso->nsinfo);
> - pthread_mutex_destroy(&dso->lock);
> + mutex_destroy(&dso->lock);
> free(dso);
> }
>
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 66981c7a9a18..58d94175e714 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -2,7 +2,6 @@
> #ifndef __PERF_DSO
> #define __PERF_DSO
>
> -#include <pthread.h>
> #include <linux/refcount.h>
> #include <linux/types.h>
> #include <linux/rbtree.h>
> @@ -11,6 +10,7 @@
> #include <stdio.h>
> #include <linux/bitops.h>
> #include "build-id.h"
> +#include "mutex.h"
>
> struct machine;
> struct map;
> @@ -145,7 +145,7 @@ struct dso_cache {
> struct auxtrace_cache;
>
> struct dso {
> - pthread_mutex_t lock;
> + struct mutex lock;
> struct list_head node;
> struct rb_node rb_node; /* rbtree node sorted by long name */
> struct rb_root *root; /* root of rbtree that rb_node is in */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a4b22caa7c24..656d9b4dd456 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1800,7 +1800,7 @@ int dso__load(struct dso *dso, struct map *map)
> }
>
> nsinfo__mountns_enter(dso->nsinfo, &nsc);
> - pthread_mutex_lock(&dso->lock);
> + mutex_lock(&dso->lock);
>
> /* check again under the dso->lock */
> if (dso__loaded(dso)) {
> @@ -1964,7 +1964,7 @@ int dso__load(struct dso *dso, struct map *map)
> ret = 0;
> out:
> dso__set_loaded(dso);
> - pthread_mutex_unlock(&dso->lock);
> + mutex_unlock(&dso->lock);
> nsinfo__mountns_exit(&nsc);
>
> return ret;
On 24/08/22 18:38, Ian Rogers wrote:
> Switch to the use of mutex wrappers that provide better error checking.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/ui/browser.c | 20 ++++++++++----------
> tools/perf/ui/browsers/annotate.c | 2 +-
> tools/perf/ui/setup.c | 5 +++--
> tools/perf/ui/tui/helpline.c | 5 ++---
> tools/perf/ui/tui/progress.c | 8 ++++----
> tools/perf/ui/tui/setup.c | 8 ++++----
> tools/perf/ui/tui/util.c | 18 +++++++++---------
> tools/perf/ui/ui.h | 4 ++--
> 8 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index fa5bd5c20e96..78fb01d6ad63 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>
> void ui_browser__show_title(struct ui_browser *browser, const char *title)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui_browser__show_title(browser, title);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> int ui_browser__show(struct ui_browser *browser, const char *title,
> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>
> browser->refresh_dimensions(browser);
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui_browser__show_title(browser, title);
>
> browser->title = title;
> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> va_end(ap);
> if (err > 0)
> ui_helpline__push(browser->helpline);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> return err ? 0 : -1;
> }
>
> void ui_browser__hide(struct ui_browser *browser)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> ui_helpline__pop();
> zfree(&browser->helpline);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> static void ui_browser__scrollbar_set(struct ui_browser *browser)
> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>
> int ui_browser__refresh(struct ui_browser *browser)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui_browser__refresh(browser);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> return 0;
> }
> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> while (1) {
> off_t offset;
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> err = __ui_browser__refresh(browser);
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> if (err < 0)
> break;
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 44ba900828f6..b8747e8dd9ea 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -8,11 +8,11 @@
> #include "../../util/hist.h"
> #include "../../util/sort.h"
> #include "../../util/map.h"
> +#include "../../util/mutex.h"
> #include "../../util/symbol.h"
> #include "../../util/evsel.h"
> #include "../../util/evlist.h"
> #include <inttypes.h>
> -#include <pthread.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
> #include <linux/zalloc.h>
> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> index 700335cde618..25ded88801a3 100644
> --- a/tools/perf/ui/setup.c
> +++ b/tools/perf/ui/setup.c
> @@ -1,5 +1,4 @@
> // SPDX-License-Identifier: GPL-2.0
> -#include <pthread.h>
> #include <dlfcn.h>
> #include <unistd.h>
>
> @@ -8,7 +7,7 @@
> #include "../util/hist.h"
> #include "ui.h"
>
> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> +struct mutex ui__lock;
> void *perf_gtk_handle;
> int use_browser = -1;
>
> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>
> void setup_browser(bool fallback_to_pager)
> {
> + mutex_init(&ui__lock);
> if (use_browser < 2 && (!isatty(1) || dump_trace))
> use_browser = 0;
>
> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> default:
> break;
> }
> + mutex_destroy(&ui__lock);
Looks like exit_browser() can be called even when setup_browser()
was never called.
Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
pthread won't notice.
> }
> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> index 298d6af82fdd..db4952f5990b 100644
> --- a/tools/perf/ui/tui/helpline.c
> +++ b/tools/perf/ui/tui/helpline.c
> @@ -2,7 +2,6 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> -#include <pthread.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
>
> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> int ret;
> static int backlog;
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> ret = vscnprintf(ui_helpline__last_msg + backlog,
> sizeof(ui_helpline__last_msg) - backlog, format, ap);
> backlog += ret;
> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> SLsmg_refresh();
> backlog = 0;
> }
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> return ret;
> }
> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> index 3d74af5a7ece..71b6c8d9474f 100644
> --- a/tools/perf/ui/tui/progress.c
> +++ b/tools/perf/ui/tui/progress.c
> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> }
>
> ui__refresh_dimensions(false);
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> y = SLtt_Screen_Rows / 2 - 2;
> SLsmg_set_color(0);
> SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> SLsmg_fill_region(y, 1, 1, bar, ' ');
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> static void tui_progress__finish(void)
> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> return;
>
> ui__refresh_dimensions(false);
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> y = SLtt_Screen_Rows / 2 - 2;
> SLsmg_set_color(0);
> SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> static struct ui_progress_ops tui_progress__ops = {
> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> index b1be59b4e2a4..a3b8c397c24d 100644
> --- a/tools/perf/ui/tui/setup.c
> +++ b/tools/perf/ui/tui/setup.c
> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> {
> if (force || ui__need_resize) {
> ui__need_resize = 0;
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> SLtt_get_screen_size();
> SLsmg_reinit_smg();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
> }
>
> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> "Press any key...", 0);
>
> SLtt_set_cursor_visibility(1);
> - if (!pthread_mutex_trylock(&ui__lock)) {
> + if (mutex_trylock(&ui__lock)) {
> SLsmg_refresh();
> SLsmg_reset_smg();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
> SLang_reset_tty();
> perf_error__unregister(&perf_tui_eops);
> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> index 0f562e2cb1e8..3c5174854ac8 100644
> --- a/tools/perf/ui/tui/util.c
> +++ b/tools/perf/ui/tui/util.c
> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> t = sep + 1;
> }
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
>
> max_len += 2;
> nr_lines += 8;
> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> SLsmg_write_nstring((char *)exit_msg, max_len);
> SLsmg_refresh();
>
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> x += 2;
> len = 0;
> key = ui__getch(delay_secs);
> while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
>
> if (key == K_BKSPC) {
> if (len == 0) {
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> goto next_key;
> }
> SLsmg_gotorc(y, x + --len);
> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> }
> SLsmg_refresh();
>
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> /* XXX more graceful overflow handling needed */
> if (len == sizeof(buf) - 1) {
> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
>
> void ui__info_window(const char *title, const char *text)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui__info_window(title, text, NULL);
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> int ui__question_window(const char *title, const char *text,
> const char *exit_msg, int delay_secs)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui__info_window(title, text, exit_msg);
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> return ui__getch(delay_secs);
> }
>
> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> --- a/tools/perf/ui/ui.h
> +++ b/tools/perf/ui/ui.h
> @@ -2,11 +2,11 @@
> #ifndef _PERF_UI_H_
> #define _PERF_UI_H_ 1
>
> -#include <pthread.h>
> +#include "../util/mutex.h"
> #include <stdbool.h>
> #include <linux/compiler.h>
>
> -extern pthread_mutex_t ui__lock;
> +extern struct mutex ui__lock;
> extern void *perf_gtk_handle;
>
> extern int use_browser;
On 24/08/22 18:38, Ian Rogers wrote:
> Switch to the use of mutex wrappers that provide better error checking.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/ui/browser.c | 20 ++++++++++----------
> tools/perf/ui/browsers/annotate.c | 2 +-
Other changes to tools/perf/ui/browsers/annotate.c are in patch 12
> tools/perf/ui/setup.c | 5 +++--
> tools/perf/ui/tui/helpline.c | 5 ++---
> tools/perf/ui/tui/progress.c | 8 ++++----
> tools/perf/ui/tui/setup.c | 8 ++++----
> tools/perf/ui/tui/util.c | 18 +++++++++---------
> tools/perf/ui/ui.h | 4 ++--
> 8 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index fa5bd5c20e96..78fb01d6ad63 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>
> void ui_browser__show_title(struct ui_browser *browser, const char *title)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui_browser__show_title(browser, title);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> int ui_browser__show(struct ui_browser *browser, const char *title,
> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>
> browser->refresh_dimensions(browser);
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui_browser__show_title(browser, title);
>
> browser->title = title;
> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> va_end(ap);
> if (err > 0)
> ui_helpline__push(browser->helpline);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> return err ? 0 : -1;
> }
>
> void ui_browser__hide(struct ui_browser *browser)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> ui_helpline__pop();
> zfree(&browser->helpline);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> static void ui_browser__scrollbar_set(struct ui_browser *browser)
> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>
> int ui_browser__refresh(struct ui_browser *browser)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui_browser__refresh(browser);
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> return 0;
> }
> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> while (1) {
> off_t offset;
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> err = __ui_browser__refresh(browser);
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> if (err < 0)
> break;
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 44ba900828f6..b8747e8dd9ea 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -8,11 +8,11 @@
> #include "../../util/hist.h"
> #include "../../util/sort.h"
> #include "../../util/map.h"
> +#include "../../util/mutex.h"
> #include "../../util/symbol.h"
> #include "../../util/evsel.h"
> #include "../../util/evlist.h"
> #include <inttypes.h>
> -#include <pthread.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
> #include <linux/zalloc.h>
> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> index 700335cde618..25ded88801a3 100644
> --- a/tools/perf/ui/setup.c
> +++ b/tools/perf/ui/setup.c
> @@ -1,5 +1,4 @@
> // SPDX-License-Identifier: GPL-2.0
> -#include <pthread.h>
> #include <dlfcn.h>
> #include <unistd.h>
>
> @@ -8,7 +7,7 @@
> #include "../util/hist.h"
> #include "ui.h"
>
> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> +struct mutex ui__lock;
> void *perf_gtk_handle;
> int use_browser = -1;
>
> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>
> void setup_browser(bool fallback_to_pager)
> {
> + mutex_init(&ui__lock);
> if (use_browser < 2 && (!isatty(1) || dump_trace))
> use_browser = 0;
>
> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> default:
> break;
> }
> + mutex_destroy(&ui__lock);
> }
> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> index 298d6af82fdd..db4952f5990b 100644
> --- a/tools/perf/ui/tui/helpline.c
> +++ b/tools/perf/ui/tui/helpline.c
> @@ -2,7 +2,6 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> -#include <pthread.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
>
> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> int ret;
> static int backlog;
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> ret = vscnprintf(ui_helpline__last_msg + backlog,
> sizeof(ui_helpline__last_msg) - backlog, format, ap);
> backlog += ret;
> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> SLsmg_refresh();
> backlog = 0;
> }
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> return ret;
> }
> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> index 3d74af5a7ece..71b6c8d9474f 100644
> --- a/tools/perf/ui/tui/progress.c
> +++ b/tools/perf/ui/tui/progress.c
> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> }
>
> ui__refresh_dimensions(false);
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> y = SLtt_Screen_Rows / 2 - 2;
> SLsmg_set_color(0);
> SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> SLsmg_fill_region(y, 1, 1, bar, ' ');
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> static void tui_progress__finish(void)
> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> return;
>
> ui__refresh_dimensions(false);
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> y = SLtt_Screen_Rows / 2 - 2;
> SLsmg_set_color(0);
> SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> static struct ui_progress_ops tui_progress__ops = {
> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> index b1be59b4e2a4..a3b8c397c24d 100644
> --- a/tools/perf/ui/tui/setup.c
> +++ b/tools/perf/ui/tui/setup.c
> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> {
> if (force || ui__need_resize) {
> ui__need_resize = 0;
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> SLtt_get_screen_size();
> SLsmg_reinit_smg();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
> }
>
> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> "Press any key...", 0);
>
> SLtt_set_cursor_visibility(1);
> - if (!pthread_mutex_trylock(&ui__lock)) {
> + if (mutex_trylock(&ui__lock)) {
> SLsmg_refresh();
> SLsmg_reset_smg();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
> SLang_reset_tty();
> perf_error__unregister(&perf_tui_eops);
> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> index 0f562e2cb1e8..3c5174854ac8 100644
> --- a/tools/perf/ui/tui/util.c
> +++ b/tools/perf/ui/tui/util.c
> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> t = sep + 1;
> }
>
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
>
> max_len += 2;
> nr_lines += 8;
> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> SLsmg_write_nstring((char *)exit_msg, max_len);
> SLsmg_refresh();
>
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> x += 2;
> len = 0;
> key = ui__getch(delay_secs);
> while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
>
> if (key == K_BKSPC) {
> if (len == 0) {
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> goto next_key;
> }
> SLsmg_gotorc(y, x + --len);
> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> }
> SLsmg_refresh();
>
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
>
> /* XXX more graceful overflow handling needed */
> if (len == sizeof(buf) - 1) {
> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
>
> void ui__info_window(const char *title, const char *text)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui__info_window(title, text, NULL);
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> }
>
> int ui__question_window(const char *title, const char *text,
> const char *exit_msg, int delay_secs)
> {
> - pthread_mutex_lock(&ui__lock);
> + mutex_lock(&ui__lock);
> __ui__info_window(title, text, exit_msg);
> SLsmg_refresh();
> - pthread_mutex_unlock(&ui__lock);
> + mutex_unlock(&ui__lock);
> return ui__getch(delay_secs);
> }
>
> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> --- a/tools/perf/ui/ui.h
> +++ b/tools/perf/ui/ui.h
> @@ -2,11 +2,11 @@
> #ifndef _PERF_UI_H_
> #define _PERF_UI_H_ 1
>
> -#include <pthread.h>
> +#include "../util/mutex.h"
> #include <stdbool.h>
> #include <linux/compiler.h>
>
> -extern pthread_mutex_t ui__lock;
> +extern struct mutex ui__lock;
> extern void *perf_gtk_handle;
>
> extern int use_browser;
On 24/08/22 18:38, Ian Rogers wrote:
> Add thread safety annotations to struct mutex so that when compiled with
> clang's -Wthread-safety warnings are generated for erroneous lock
> patterns. NO_THREAD_SAFETY_ANALYSIS is needed for
> mutex_lock/mutex_unlock as the analysis doesn't under pthread calls.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/mutex.c | 2 ++
> tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> index 892294ac1769..ec813093276d 100644
> --- a/tools/perf/util/mutex.c
> +++ b/tools/perf/util/mutex.c
> @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx)
> }
>
> void mutex_lock(struct mutex *mtx)
> + NO_THREAD_SAFETY_ANALYSIS
> {
> CHECK_ERR(pthread_mutex_lock(&mtx->lock));
> }
>
> void mutex_unlock(struct mutex *mtx)
> + NO_THREAD_SAFETY_ANALYSIS
> {
> CHECK_ERR(pthread_mutex_unlock(&mtx->lock));
> }
> diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> index c9e110a2b55e..48a2d87598f0 100644
> --- a/tools/perf/util/mutex.h
> +++ b/tools/perf/util/mutex.h
> @@ -5,11 +5,73 @@
> #include <pthread.h>
> #include <stdbool.h>
>
> +/*
> + * A function-like feature checking macro that is a wrapper around
> + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a
> + * nonzero constant integer if the attribute is supported or 0 if not.
> + */
> +#ifdef __has_attribute
> +#define HAVE_ATTRIBUTE(x) __has_attribute(x)
> +#else
> +#define HAVE_ATTRIBUTE(x) 0
> +#endif
> +
> +
Multiple blank lines
> +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \
> + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \
> + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \
> + HAVE_ATTRIBUTE(no_thread_safety_analysis)
> +
> +/* Documents if a shared field or global variable needs to be protected by a mutex. */
> +#define GUARDED_BY(x) __attribute__((guarded_by(x)))
> +
> +/*
> + * Documents if the memory location pointed to by a pointer should be guarded by
> + * a mutex when dereferencing the pointer.
> + */
> +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
> +
> +/* Documents if a type is a lockable type. */
> +#define LOCKABLE __attribute__((capability("lockable")))
> +
> +/* Documents functions that acquire a lock in the body of a function, and do not release it. */
> +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
> +
> +/*
> + * Documents functions that expect a lock to be held on entry to the function,
> + * and release it in the body of the function.
> + */
> +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
> +
> +/* Documents functions that try to acquire a lock, and return success or failure. */
> +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \
> + __attribute__((exclusive_trylock_function(__VA_ARGS__)))
> +
> +
Multiple blank lines
> +/* Documents a function that expects a mutex to be held prior to entry. */
> +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
> +
> +/* Turns off thread safety checking within the body of a particular function. */
> +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
> +
> +#else
> +
> +#define GUARDED_BY(x)
> +#define PT_GUARDED_BY(x)
> +#define LOCKABLE
> +#define EXCLUSIVE_LOCK_FUNCTION(...)
> +#define UNLOCK_FUNCTION(...)
> +#define EXCLUSIVE_TRYLOCK_FUNCTION(...)
> +#define EXCLUSIVE_LOCKS_REQUIRED(...)
> +#define NO_THREAD_SAFETY_ANALYSIS
> +
> +#endif
> +
> /*
> * A wrapper around the mutex implementation that allows perf to error check
> * usage, etc.
> */
> -struct mutex {
> +struct LOCKABLE mutex {
> pthread_mutex_t lock;
> };
>
> @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx);
> void mutex_init_pshared(struct mutex *mtx);
> void mutex_destroy(struct mutex *mtx);
>
> -void mutex_lock(struct mutex *mtx);
> -void mutex_unlock(struct mutex *mtx);
> -bool mutex_trylock(struct mutex *mtx);
> +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx);
> +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx);
> +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx);
>
> /* Default initialize the cond struct. */
> void cond_init(struct cond *cnd);
> @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd);
> void cond_init_pshared(struct cond *cnd);
> void cond_destroy(struct cond *cnd);
>
> -void cond_wait(struct cond *cnd, struct mutex *mtx);
> +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx);
> void cond_signal(struct cond *cnd);
> void cond_broadcast(struct cond *cnd);
>
On 24/08/22 18:38, Ian Rogers wrote:
> Add annotations to describe lock behavior. Add unlocks so that mutexes
> aren't conditionally held on exit from perf_sched__replay. Add an exit
> variable so that thread_func can terminate, rather than leaving the
> threads blocked on mutexes.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 7e4006d6b8bc..b483ff0d432e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -246,6 +246,7 @@ struct perf_sched {
> const char *time_str;
> struct perf_time_interval ptime;
> struct perf_time_interval hist_time;
> + volatile bool thread_funcs_exit;
> };
>
> /* per thread run time data */
> @@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
> prctl(PR_SET_NAME, comm2);
> if (fd < 0)
> return NULL;
> -again:
> - ret = sem_post(&this_task->ready_for_work);
> - BUG_ON(ret);
> - mutex_lock(&sched->start_work_mutex);
> - mutex_unlock(&sched->start_work_mutex);
>
> - cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> + while (!sched->thread_funcs_exit) {
> + ret = sem_post(&this_task->ready_for_work);
> + BUG_ON(ret);
> + mutex_lock(&sched->start_work_mutex);
> + mutex_unlock(&sched->start_work_mutex);
>
> - for (i = 0; i < this_task->nr_events; i++) {
> - this_task->curr_event = i;
> - perf_sched__process_event(sched, this_task->atoms[i]);
> - }
> + cpu_usage_0 = get_cpu_usage_nsec_self(fd);
>
> - cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> - this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> - ret = sem_post(&this_task->work_done_sem);
> - BUG_ON(ret);
> + for (i = 0; i < this_task->nr_events; i++) {
> + this_task->curr_event = i;
> + perf_sched__process_event(sched, this_task->atoms[i]);
> + }
>
> - mutex_lock(&sched->work_done_wait_mutex);
> - mutex_unlock(&sched->work_done_wait_mutex);
> + cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> + this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> + ret = sem_post(&this_task->work_done_sem);
> + BUG_ON(ret);
>
> - goto again;
> + mutex_lock(&sched->work_done_wait_mutex);
> + mutex_unlock(&sched->work_done_wait_mutex);
> + }
> + return NULL;
> }
>
> static void create_tasks(struct perf_sched *sched)
> + EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
> + EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
> {
> struct task_desc *task;
> pthread_attr_t attr;
> @@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
> }
>
> static void wait_for_tasks(struct perf_sched *sched)
> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> {
> u64 cpu_usage_0, cpu_usage_1;
> struct task_desc *task;
> @@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
> }
>
> static void run_one_test(struct perf_sched *sched)
> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> {
> u64 T0, T1, delta, avg_delta, fluct;
>
> @@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
> print_task_traces(sched);
> add_cross_task_wakeups(sched);
>
> + sched->thread_funcs_exit = false;
> create_tasks(sched);
> printf("------------------------------------------------------------\n");
> for (i = 0; i < sched->replay_repeat; i++)
> run_one_test(sched);
>
> + sched->thread_funcs_exit = true;
> + mutex_unlock(&sched->start_work_mutex);
> + mutex_unlock(&sched->work_done_wait_mutex);
I think you still need to wait for the threads to exit before
destroying the mutexes.
> return 0;
> }
>
On 25/08/22 19:14, Adrian Hunter wrote:
> On 25/08/22 15:30, Arnaldo Carvalho de Melo wrote:
>> On Wed, Aug 24, 2022, 12:39 PM Ian Rogers <[email protected] <mailto:[email protected]>> wrote:
>>
>> When fixing a locking race and memory leak in:
>> https://lore.kernel.org/linux-perf-users/[email protected]/ <https://lore.kernel.org/linux-perf-users/[email protected]/>
>>
>> It was requested that debug mutex code be separated out into its own
>> files. This was, in part, done by Pavithra Gurushankar in:
>> https://lore.kernel.org/lkml/[email protected]/ <https://lore.kernel.org/lkml/[email protected]/>
>>
>> These patches fix issues with the previous patches, add in the
>> original dso->nsinfo fix and then build on our mutex wrapper with
>> clang's -Wthread-safety analysis. The analysis found missing unlocks
>> in builtin-sched.c which are fixed and -Wthread-safety is enabled by
>> default when building with clang.
>>
>> v3. Adds a missing new line to the error messages and removes the
>> Â Â pshared argument to mutex_init by having two functions, mutex_init
>> Â Â and mutex_init_pshared. These changes were suggested by Adrian Hunter.
>>
>>
>> Adrian, can I have your Acked-by or, better, Reviewed-by?
>
> Sure, just let me have another look. Should get to it
> tomorrow.
Looks good but a couple of things that need to be fixed up.
>
>>
>> Thanks,Â
>>
>> -Â ArnaldoÂ
>>
>> v2. Breaks apart changes that s/pthread_mutex/mutex/g and the lock
>> Â Â annotations as requested by Arnaldo and Namhyung. A boolean is
>> Â Â added to builtin-sched.c to terminate thread funcs rather than
>> Â Â leaving them blocked on delted mutexes.
>>
>> Ian Rogers (17):
>> Â perf bench: Update use of pthread mutex/cond
>> Â perf tests: Avoid pthread.h inclusion
>> Â perf hist: Update use of pthread mutex
>> Â perf bpf: Remove unused pthread.h include
>> Â perf lock: Remove unused pthread.h include
>> Â perf record: Update use of pthread mutex
>> Â perf sched: Update use of pthread mutex
>> Â perf ui: Update use of pthread mutex
>> Â perf mmap: Remove unnecessary pthread.h include
>> Â perf dso: Update use of pthread mutex
>> Â perf annotate: Update use of pthread mutex
>> Â perf top: Update use of pthread mutex
>> Â perf dso: Hold lock when accessing nsinfo
>> Â perf mutex: Add thread safety annotations
>> Â perf sched: Fixes for thread safety analysis
>> Â perf top: Fixes for thread safety analysis
>> Â perf build: Enable -Wthread-safety with clang
>>
>> Pavithra Gurushankar (1):
>> Â perf mutex: Wrapped usage of mutex and cond
>>
>>  tools/perf/Makefile.config         |  5 +
>>  tools/perf/bench/epoll-ctl.c        | 33 +++---
>>  tools/perf/bench/epoll-wait.c       | 33 +++---
>>  tools/perf/bench/futex-hash.c       | 33 +++---
>>  tools/perf/bench/futex-lock-pi.c      | 33 +++---
>>  tools/perf/bench/futex-requeue.c      | 33 +++---
>>  tools/perf/bench/futex-wake-parallel.c   | 33 +++---
>>  tools/perf/bench/futex-wake.c       | 33 +++---
>>  tools/perf/bench/numa.c          | 93 ++++++----------
>>  tools/perf/builtin-inject.c        |  4 +
>>  tools/perf/builtin-lock.c         |  1 -
>>  tools/perf/builtin-record.c        | 13 ++-
>>  tools/perf/builtin-sched.c         | 105 +++++++++---------
>>  tools/perf/builtin-top.c          | 45 ++++----
>>  tools/perf/tests/mmap-basic.c       |  2 -
>> Â tools/perf/tests/openat-syscall-all-cpus.c |Â Â 2 +-
>>  tools/perf/tests/perf-record.c       |  2 -
>>  tools/perf/ui/browser.c          | 20 ++--
>>  tools/perf/ui/browsers/annotate.c     | 12 +--
>>  tools/perf/ui/setup.c           |  5 +-
>>  tools/perf/ui/tui/helpline.c        |  5 +-
>>  tools/perf/ui/tui/progress.c        |  8 +-
>>  tools/perf/ui/tui/setup.c         |  8 +-
>>  tools/perf/ui/tui/util.c          | 18 ++--
>>  tools/perf/ui/ui.h             |  4 +-
>>  tools/perf/util/Build           |  1 +
>>  tools/perf/util/annotate.c         | 15 +--
>>  tools/perf/util/annotate.h         |  4 +-
>>  tools/perf/util/bpf-event.h        |  1 -
>>  tools/perf/util/build-id.c         | 12 ++-
>>  tools/perf/util/dso.c           | 19 ++--
>>  tools/perf/util/dso.h           |  4 +-
>>  tools/perf/util/hist.c           |  6 +-
>>  tools/perf/util/hist.h           |  4 +-
>>  tools/perf/util/map.c           |  3 +
>>  tools/perf/util/mmap.h           |  1 -
>>  tools/perf/util/mutex.c          | 119 +++++++++++++++++++++
>>  tools/perf/util/mutex.h          | 109 +++++++++++++++++++
>>  tools/perf/util/probe-event.c       |  3 +
>>  tools/perf/util/symbol.c          |  4 +-
>>  tools/perf/util/top.h           |  5 +-
>> Â 41 files changed, 570 insertions(+), 323 deletions(-)
>> Â create mode 100644 tools/perf/util/mutex.c
>> Â create mode 100644 tools/perf/util/mutex.h
>>
>> --
>> 2.37.2.609.g9ff673ca1a-goog
>>
>
On Fri, Aug 26, 2022 at 3:37 AM Adrian Hunter <[email protected]> wrote:
>
> On 24/08/22 18:38, Ian Rogers wrote:
> > Switch to the use of mutex wrappers that provide better error checking.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/dso.c | 12 ++++++------
>
> Some not done yet
>
> $ grep -i pthread_mut tools/perf/util/dso.c
> static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
> pthread_mutex_lock(&dso__data_open_lock);
> pthread_mutex_unlock(&dso__data_open_lock);
> if (pthread_mutex_lock(&dso__data_open_lock) < 0)
> pthread_mutex_unlock(&dso__data_open_lock);
> pthread_mutex_unlock(&dso__data_open_lock);
> pthread_mutex_lock(&dso__data_open_lock);
> pthread_mutex_unlock(&dso__data_open_lock);
> pthread_mutex_lock(&dso__data_open_lock);
> pthread_mutex_unlock(&dso__data_open_lock);
Yes, these are all solely dso__data_open_lock that lacks any clear
init/exit code to place the initialization/destruction hooks onto. I
don't plan to alter these in this patch set.
Thanks,
Ian
>
> > tools/perf/util/dso.h | 4 ++--
> > tools/perf/util/symbol.c | 4 ++--
> > 3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 5ac13958d1bd..a9789a955403 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -795,7 +795,7 @@ dso_cache__free(struct dso *dso)
> > struct rb_root *root = &dso->data.cache;
> > struct rb_node *next = rb_first(root);
> >
> > - pthread_mutex_lock(&dso->lock);
> > + mutex_lock(&dso->lock);
> > while (next) {
> > struct dso_cache *cache;
> >
> > @@ -804,7 +804,7 @@ dso_cache__free(struct dso *dso)
> > rb_erase(&cache->rb_node, root);
> > free(cache);
> > }
> > - pthread_mutex_unlock(&dso->lock);
> > + mutex_unlock(&dso->lock);
> > }
> >
> > static struct dso_cache *__dso_cache__find(struct dso *dso, u64 offset)
> > @@ -841,7 +841,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
> > struct dso_cache *cache;
> > u64 offset = new->offset;
> >
> > - pthread_mutex_lock(&dso->lock);
> > + mutex_lock(&dso->lock);
> > while (*p != NULL) {
> > u64 end;
> >
> > @@ -862,7 +862,7 @@ dso_cache__insert(struct dso *dso, struct dso_cache *new)
> >
> > cache = NULL;
> > out:
> > - pthread_mutex_unlock(&dso->lock);
> > + mutex_unlock(&dso->lock);
> > return cache;
> > }
> >
> > @@ -1297,7 +1297,7 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
> > dso->root = NULL;
> > INIT_LIST_HEAD(&dso->node);
> > INIT_LIST_HEAD(&dso->data.open_entry);
> > - pthread_mutex_init(&dso->lock, NULL);
> > + mutex_init(&dso->lock);
> > refcount_set(&dso->refcnt, 1);
> > }
> >
> > @@ -1336,7 +1336,7 @@ void dso__delete(struct dso *dso)
> > dso__free_a2l(dso);
> > zfree(&dso->symsrc_filename);
> > nsinfo__zput(dso->nsinfo);
> > - pthread_mutex_destroy(&dso->lock);
> > + mutex_destroy(&dso->lock);
> > free(dso);
> > }
> >
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index 66981c7a9a18..58d94175e714 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -2,7 +2,6 @@
> > #ifndef __PERF_DSO
> > #define __PERF_DSO
> >
> > -#include <pthread.h>
> > #include <linux/refcount.h>
> > #include <linux/types.h>
> > #include <linux/rbtree.h>
> > @@ -11,6 +10,7 @@
> > #include <stdio.h>
> > #include <linux/bitops.h>
> > #include "build-id.h"
> > +#include "mutex.h"
> >
> > struct machine;
> > struct map;
> > @@ -145,7 +145,7 @@ struct dso_cache {
> > struct auxtrace_cache;
> >
> > struct dso {
> > - pthread_mutex_t lock;
> > + struct mutex lock;
> > struct list_head node;
> > struct rb_node rb_node; /* rbtree node sorted by long name */
> > struct rb_root *root; /* root of rbtree that rb_node is in */
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index a4b22caa7c24..656d9b4dd456 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1800,7 +1800,7 @@ int dso__load(struct dso *dso, struct map *map)
> > }
> >
> > nsinfo__mountns_enter(dso->nsinfo, &nsc);
> > - pthread_mutex_lock(&dso->lock);
> > + mutex_lock(&dso->lock);
> >
> > /* check again under the dso->lock */
> > if (dso__loaded(dso)) {
> > @@ -1964,7 +1964,7 @@ int dso__load(struct dso *dso, struct map *map)
> > ret = 0;
> > out:
> > dso__set_loaded(dso);
> > - pthread_mutex_unlock(&dso->lock);
> > + mutex_unlock(&dso->lock);
> > nsinfo__mountns_exit(&nsc);
> >
> > return ret;
>
On Fri, Aug 26, 2022 at 5:12 AM Adrian Hunter <[email protected]> wrote:
>
> On 24/08/22 18:38, Ian Rogers wrote:
> > Add annotations to describe lock behavior. Add unlocks so that mutexes
> > aren't conditionally held on exit from perf_sched__replay. Add an exit
> > variable so that thread_func can terminate, rather than leaving the
> > threads blocked on mutexes.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
> > 1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 7e4006d6b8bc..b483ff0d432e 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -246,6 +246,7 @@ struct perf_sched {
> > const char *time_str;
> > struct perf_time_interval ptime;
> > struct perf_time_interval hist_time;
> > + volatile bool thread_funcs_exit;
> > };
> >
> > /* per thread run time data */
> > @@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
> > prctl(PR_SET_NAME, comm2);
> > if (fd < 0)
> > return NULL;
> > -again:
> > - ret = sem_post(&this_task->ready_for_work);
> > - BUG_ON(ret);
> > - mutex_lock(&sched->start_work_mutex);
> > - mutex_unlock(&sched->start_work_mutex);
> >
> > - cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> > + while (!sched->thread_funcs_exit) {
> > + ret = sem_post(&this_task->ready_for_work);
> > + BUG_ON(ret);
> > + mutex_lock(&sched->start_work_mutex);
> > + mutex_unlock(&sched->start_work_mutex);
> >
> > - for (i = 0; i < this_task->nr_events; i++) {
> > - this_task->curr_event = i;
> > - perf_sched__process_event(sched, this_task->atoms[i]);
> > - }
> > + cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> >
> > - cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> > - this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> > - ret = sem_post(&this_task->work_done_sem);
> > - BUG_ON(ret);
> > + for (i = 0; i < this_task->nr_events; i++) {
> > + this_task->curr_event = i;
> > + perf_sched__process_event(sched, this_task->atoms[i]);
> > + }
> >
> > - mutex_lock(&sched->work_done_wait_mutex);
> > - mutex_unlock(&sched->work_done_wait_mutex);
> > + cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> > + this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> > + ret = sem_post(&this_task->work_done_sem);
> > + BUG_ON(ret);
> >
> > - goto again;
> > + mutex_lock(&sched->work_done_wait_mutex);
> > + mutex_unlock(&sched->work_done_wait_mutex);
> > + }
> > + return NULL;
> > }
> >
> > static void create_tasks(struct perf_sched *sched)
> > + EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
> > + EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
> > {
> > struct task_desc *task;
> > pthread_attr_t attr;
> > @@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
> > }
> >
> > static void wait_for_tasks(struct perf_sched *sched)
> > + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> > + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> > {
> > u64 cpu_usage_0, cpu_usage_1;
> > struct task_desc *task;
> > @@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
> > }
> >
> > static void run_one_test(struct perf_sched *sched)
> > + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> > + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> > {
> > u64 T0, T1, delta, avg_delta, fluct;
> >
> > @@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
> > print_task_traces(sched);
> > add_cross_task_wakeups(sched);
> >
> > + sched->thread_funcs_exit = false;
> > create_tasks(sched);
> > printf("------------------------------------------------------------\n");
> > for (i = 0; i < sched->replay_repeat; i++)
> > run_one_test(sched);
> >
> > + sched->thread_funcs_exit = true;
> > + mutex_unlock(&sched->start_work_mutex);
> > + mutex_unlock(&sched->work_done_wait_mutex);
>
> I think you still need to wait for the threads to exit before
> destroying the mutexes.
This is a pre-existing issue and beyond the scope of this patch set.
Thanks,
Ian
> > return 0;
> > }
> >
>
On Fri, Aug 26, 2022 at 3:11 AM Adrian Hunter <[email protected]> wrote:
>
> On 24/08/22 18:38, Ian Rogers wrote:
> > Switch to the use of mutex wrappers that provide better error checking.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/ui/browser.c | 20 ++++++++++----------
> > tools/perf/ui/browsers/annotate.c | 2 +-
>
> Other changes to tools/perf/ui/browsers/annotate.c are in patch 12
Yes, these changes relate to the ui__lock and the patch 12 changes
relate to annotation lock. The grouping was done in this way so that
the patches would build independently.
Thanks,
Ian
> > tools/perf/ui/setup.c | 5 +++--
> > tools/perf/ui/tui/helpline.c | 5 ++---
> > tools/perf/ui/tui/progress.c | 8 ++++----
> > tools/perf/ui/tui/setup.c | 8 ++++----
> > tools/perf/ui/tui/util.c | 18 +++++++++---------
> > tools/perf/ui/ui.h | 4 ++--
> > 8 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > index fa5bd5c20e96..78fb01d6ad63 100644
> > --- a/tools/perf/ui/browser.c
> > +++ b/tools/perf/ui/browser.c
> > @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
> >
> > void ui_browser__show_title(struct ui_browser *browser, const char *title)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui_browser__show_title(browser, title);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > int ui_browser__show(struct ui_browser *browser, const char *title,
> > @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >
> > browser->refresh_dimensions(browser);
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui_browser__show_title(browser, title);
> >
> > browser->title = title;
> > @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > va_end(ap);
> > if (err > 0)
> > ui_helpline__push(browser->helpline);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > return err ? 0 : -1;
> > }
> >
> > void ui_browser__hide(struct ui_browser *browser)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > ui_helpline__pop();
> > zfree(&browser->helpline);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > static void ui_browser__scrollbar_set(struct ui_browser *browser)
> > @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
> >
> > int ui_browser__refresh(struct ui_browser *browser)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui_browser__refresh(browser);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > return 0;
> > }
> > @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> > while (1) {
> > off_t offset;
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > err = __ui_browser__refresh(browser);
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > if (err < 0)
> > break;
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 44ba900828f6..b8747e8dd9ea 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -8,11 +8,11 @@
> > #include "../../util/hist.h"
> > #include "../../util/sort.h"
> > #include "../../util/map.h"
> > +#include "../../util/mutex.h"
> > #include "../../util/symbol.h"
> > #include "../../util/evsel.h"
> > #include "../../util/evlist.h"
> > #include <inttypes.h>
> > -#include <pthread.h>
> > #include <linux/kernel.h>
> > #include <linux/string.h>
> > #include <linux/zalloc.h>
> > diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> > index 700335cde618..25ded88801a3 100644
> > --- a/tools/perf/ui/setup.c
> > +++ b/tools/perf/ui/setup.c
> > @@ -1,5 +1,4 @@
> > // SPDX-License-Identifier: GPL-2.0
> > -#include <pthread.h>
> > #include <dlfcn.h>
> > #include <unistd.h>
> >
> > @@ -8,7 +7,7 @@
> > #include "../util/hist.h"
> > #include "ui.h"
> >
> > -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> > +struct mutex ui__lock;
> > void *perf_gtk_handle;
> > int use_browser = -1;
> >
> > @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> >
> > void setup_browser(bool fallback_to_pager)
> > {
> > + mutex_init(&ui__lock);
> > if (use_browser < 2 && (!isatty(1) || dump_trace))
> > use_browser = 0;
> >
> > @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> > default:
> > break;
> > }
> > + mutex_destroy(&ui__lock);
> > }
> > diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> > index 298d6af82fdd..db4952f5990b 100644
> > --- a/tools/perf/ui/tui/helpline.c
> > +++ b/tools/perf/ui/tui/helpline.c
> > @@ -2,7 +2,6 @@
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > -#include <pthread.h>
> > #include <linux/kernel.h>
> > #include <linux/string.h>
> >
> > @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> > int ret;
> > static int backlog;
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > ret = vscnprintf(ui_helpline__last_msg + backlog,
> > sizeof(ui_helpline__last_msg) - backlog, format, ap);
> > backlog += ret;
> > @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> > SLsmg_refresh();
> > backlog = 0;
> > }
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > return ret;
> > }
> > diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> > index 3d74af5a7ece..71b6c8d9474f 100644
> > --- a/tools/perf/ui/tui/progress.c
> > +++ b/tools/perf/ui/tui/progress.c
> > @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> > }
> >
> > ui__refresh_dimensions(false);
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > y = SLtt_Screen_Rows / 2 - 2;
> > SLsmg_set_color(0);
> > SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> > @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> > bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> > SLsmg_fill_region(y, 1, 1, bar, ' ');
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > static void tui_progress__finish(void)
> > @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> > return;
> >
> > ui__refresh_dimensions(false);
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > y = SLtt_Screen_Rows / 2 - 2;
> > SLsmg_set_color(0);
> > SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > static struct ui_progress_ops tui_progress__ops = {
> > diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> > index b1be59b4e2a4..a3b8c397c24d 100644
> > --- a/tools/perf/ui/tui/setup.c
> > +++ b/tools/perf/ui/tui/setup.c
> > @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> > {
> > if (force || ui__need_resize) {
> > ui__need_resize = 0;
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > SLtt_get_screen_size();
> > SLsmg_reinit_smg();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> > }
> >
> > @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> > "Press any key...", 0);
> >
> > SLtt_set_cursor_visibility(1);
> > - if (!pthread_mutex_trylock(&ui__lock)) {
> > + if (mutex_trylock(&ui__lock)) {
> > SLsmg_refresh();
> > SLsmg_reset_smg();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> > SLang_reset_tty();
> > perf_error__unregister(&perf_tui_eops);
> > diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> > index 0f562e2cb1e8..3c5174854ac8 100644
> > --- a/tools/perf/ui/tui/util.c
> > +++ b/tools/perf/ui/tui/util.c
> > @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> > t = sep + 1;
> > }
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> >
> > max_len += 2;
> > nr_lines += 8;
> > @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> > SLsmg_write_nstring((char *)exit_msg, max_len);
> > SLsmg_refresh();
> >
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > x += 2;
> > len = 0;
> > key = ui__getch(delay_secs);
> > while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> >
> > if (key == K_BKSPC) {
> > if (len == 0) {
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > goto next_key;
> > }
> > SLsmg_gotorc(y, x + --len);
> > @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> > }
> > SLsmg_refresh();
> >
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > /* XXX more graceful overflow handling needed */
> > if (len == sizeof(buf) - 1) {
> > @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
> >
> > void ui__info_window(const char *title, const char *text)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui__info_window(title, text, NULL);
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > int ui__question_window(const char *title, const char *text,
> > const char *exit_msg, int delay_secs)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui__info_window(title, text, exit_msg);
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > return ui__getch(delay_secs);
> > }
> >
> > diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> > index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> > --- a/tools/perf/ui/ui.h
> > +++ b/tools/perf/ui/ui.h
> > @@ -2,11 +2,11 @@
> > #ifndef _PERF_UI_H_
> > #define _PERF_UI_H_ 1
> >
> > -#include <pthread.h>
> > +#include "../util/mutex.h"
> > #include <stdbool.h>
> > #include <linux/compiler.h>
> >
> > -extern pthread_mutex_t ui__lock;
> > +extern struct mutex ui__lock;
> > extern void *perf_gtk_handle;
> >
> > extern int use_browser;
>
Em Wed, Aug 24, 2022 at 08:38:58AM -0700, Ian Rogers escreveu:
> Add thread safety annotations to struct mutex so that when compiled with
> clang's -Wthread-safety warnings are generated for erroneous lock
> patterns. NO_THREAD_SAFETY_ANALYSIS is needed for
> mutex_lock/mutex_unlock as the analysis doesn't under pthread calls.
So even having the guards checking if the attribute is available it
seems at least clang 11.0 is needed, as the "lockable" arg was
introduced there:
37 42.61 fedora:32 : FAIL clang version 10.0.1 (Fedora 10.0.1-3.fc32)
In file included from /git/perf-6.0.0-rc2/tools/perf/util/../ui/ui.h:5:
/git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:74:8: error: invalid capability name 'lockable'; capability name must be 'mutex' or 'role' [-Werror,-Wthread-safety-attributes]
struct LOCKABLE mutex {
^
/git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:35:44: note: expanded from macro 'LOCKABLE'
#define LOCKABLE __attribute__((capability("lockable")))
The next fedora releases are ok with it:
38 124.89 fedora:33 : Ok gcc (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1) , clang version 11.0.0 (Fedora 11.0.0-3.fc33)
39 132.20 fedora:34 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 12.0.1 (Fedora 12.0.1-1.fc34)
40 21.44 fedora:34-x-ARC-glibc : Ok arc-linux-gcc (ARC HS GNU/Linux glibc toolchain 2019.03-rc1) 8.3.1 20190225
41 19.43 fedora:34-x-ARC-uClibc : Ok arc-linux-gcc (ARCv2 ISA Linux uClibc toolchain 2019.03-rc1) 8.3.1 20190225
42 136.33 fedora:35 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 13.0.0 (Fedora 13.0.0-3.fc35)
43 137.73 fedora:36 : Ok gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1) , clang version 14.0.0 (Fedora 14.0.0-1.fc36)
44 140.45 fedora:37 : Ok gcc (GCC) 12.1.1 20220628 (Red Hat 12.1.1-3) , clang version 14.0.5 (Fedora 14.0.5-6.fc37)
45 126.80 fedora:38 : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38)
46 127.00 fedora:rawhide : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38)
Is there a way to check if a "capability" is available for the
attribute? Otherwise we can additionally check the clang version?
For my tests I'll make clang 11.0 to be the oldest supported compiler
(i.e. just disable building with older versions), but wanted to let you
know since you try to check if the attribute is available and fallback
to doing nothing in that case.
- Arnaldo
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/mutex.c | 2 ++
> tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> index 892294ac1769..ec813093276d 100644
> --- a/tools/perf/util/mutex.c
> +++ b/tools/perf/util/mutex.c
> @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx)
> }
>
> void mutex_lock(struct mutex *mtx)
> + NO_THREAD_SAFETY_ANALYSIS
> {
> CHECK_ERR(pthread_mutex_lock(&mtx->lock));
> }
>
> void mutex_unlock(struct mutex *mtx)
> + NO_THREAD_SAFETY_ANALYSIS
> {
> CHECK_ERR(pthread_mutex_unlock(&mtx->lock));
> }
> diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> index c9e110a2b55e..48a2d87598f0 100644
> --- a/tools/perf/util/mutex.h
> +++ b/tools/perf/util/mutex.h
> @@ -5,11 +5,73 @@
> #include <pthread.h>
> #include <stdbool.h>
>
> +/*
> + * A function-like feature checking macro that is a wrapper around
> + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a
> + * nonzero constant integer if the attribute is supported or 0 if not.
> + */
> +#ifdef __has_attribute
> +#define HAVE_ATTRIBUTE(x) __has_attribute(x)
> +#else
> +#define HAVE_ATTRIBUTE(x) 0
> +#endif
> +
> +
> +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \
> + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \
> + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \
> + HAVE_ATTRIBUTE(no_thread_safety_analysis)
> +
> +/* Documents if a shared field or global variable needs to be protected by a mutex. */
> +#define GUARDED_BY(x) __attribute__((guarded_by(x)))
> +
> +/*
> + * Documents if the memory location pointed to by a pointer should be guarded by
> + * a mutex when dereferencing the pointer.
> + */
> +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
> +
> +/* Documents if a type is a lockable type. */
> +#define LOCKABLE __attribute__((capability("lockable")))
> +
> +/* Documents functions that acquire a lock in the body of a function, and do not release it. */
> +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
> +
> +/*
> + * Documents functions that expect a lock to be held on entry to the function,
> + * and release it in the body of the function.
> + */
> +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
> +
> +/* Documents functions that try to acquire a lock, and return success or failure. */
> +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \
> + __attribute__((exclusive_trylock_function(__VA_ARGS__)))
> +
> +
> +/* Documents a function that expects a mutex to be held prior to entry. */
> +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
> +
> +/* Turns off thread safety checking within the body of a particular function. */
> +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
> +
> +#else
> +
> +#define GUARDED_BY(x)
> +#define PT_GUARDED_BY(x)
> +#define LOCKABLE
> +#define EXCLUSIVE_LOCK_FUNCTION(...)
> +#define UNLOCK_FUNCTION(...)
> +#define EXCLUSIVE_TRYLOCK_FUNCTION(...)
> +#define EXCLUSIVE_LOCKS_REQUIRED(...)
> +#define NO_THREAD_SAFETY_ANALYSIS
> +
> +#endif
> +
> /*
> * A wrapper around the mutex implementation that allows perf to error check
> * usage, etc.
> */
> -struct mutex {
> +struct LOCKABLE mutex {
> pthread_mutex_t lock;
> };
>
> @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx);
> void mutex_init_pshared(struct mutex *mtx);
> void mutex_destroy(struct mutex *mtx);
>
> -void mutex_lock(struct mutex *mtx);
> -void mutex_unlock(struct mutex *mtx);
> -bool mutex_trylock(struct mutex *mtx);
> +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx);
> +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx);
> +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx);
>
> /* Default initialize the cond struct. */
> void cond_init(struct cond *cnd);
> @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd);
> void cond_init_pshared(struct cond *cnd);
> void cond_destroy(struct cond *cnd);
>
> -void cond_wait(struct cond *cnd, struct mutex *mtx);
> +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx);
> void cond_signal(struct cond *cnd);
> void cond_broadcast(struct cond *cnd);
>
> --
> 2.37.2.609.g9ff673ca1a-goog
--
- Arnaldo
On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <[email protected]> wrote:
>
> On 24/08/22 18:38, Ian Rogers wrote:
> > Switch to the use of mutex wrappers that provide better error checking.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/ui/browser.c | 20 ++++++++++----------
> > tools/perf/ui/browsers/annotate.c | 2 +-
> > tools/perf/ui/setup.c | 5 +++--
> > tools/perf/ui/tui/helpline.c | 5 ++---
> > tools/perf/ui/tui/progress.c | 8 ++++----
> > tools/perf/ui/tui/setup.c | 8 ++++----
> > tools/perf/ui/tui/util.c | 18 +++++++++---------
> > tools/perf/ui/ui.h | 4 ++--
> > 8 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > index fa5bd5c20e96..78fb01d6ad63 100644
> > --- a/tools/perf/ui/browser.c
> > +++ b/tools/perf/ui/browser.c
> > @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
> >
> > void ui_browser__show_title(struct ui_browser *browser, const char *title)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui_browser__show_title(browser, title);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > int ui_browser__show(struct ui_browser *browser, const char *title,
> > @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >
> > browser->refresh_dimensions(browser);
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui_browser__show_title(browser, title);
> >
> > browser->title = title;
> > @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > va_end(ap);
> > if (err > 0)
> > ui_helpline__push(browser->helpline);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > return err ? 0 : -1;
> > }
> >
> > void ui_browser__hide(struct ui_browser *browser)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > ui_helpline__pop();
> > zfree(&browser->helpline);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > static void ui_browser__scrollbar_set(struct ui_browser *browser)
> > @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
> >
> > int ui_browser__refresh(struct ui_browser *browser)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui_browser__refresh(browser);
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > return 0;
> > }
> > @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> > while (1) {
> > off_t offset;
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > err = __ui_browser__refresh(browser);
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > if (err < 0)
> > break;
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 44ba900828f6..b8747e8dd9ea 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -8,11 +8,11 @@
> > #include "../../util/hist.h"
> > #include "../../util/sort.h"
> > #include "../../util/map.h"
> > +#include "../../util/mutex.h"
> > #include "../../util/symbol.h"
> > #include "../../util/evsel.h"
> > #include "../../util/evlist.h"
> > #include <inttypes.h>
> > -#include <pthread.h>
> > #include <linux/kernel.h>
> > #include <linux/string.h>
> > #include <linux/zalloc.h>
> > diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> > index 700335cde618..25ded88801a3 100644
> > --- a/tools/perf/ui/setup.c
> > +++ b/tools/perf/ui/setup.c
> > @@ -1,5 +1,4 @@
> > // SPDX-License-Identifier: GPL-2.0
> > -#include <pthread.h>
> > #include <dlfcn.h>
> > #include <unistd.h>
> >
> > @@ -8,7 +7,7 @@
> > #include "../util/hist.h"
> > #include "ui.h"
> >
> > -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> > +struct mutex ui__lock;
> > void *perf_gtk_handle;
> > int use_browser = -1;
> >
> > @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> >
> > void setup_browser(bool fallback_to_pager)
> > {
> > + mutex_init(&ui__lock);
> > if (use_browser < 2 && (!isatty(1) || dump_trace))
> > use_browser = 0;
> >
> > @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> > default:
> > break;
> > }
> > + mutex_destroy(&ui__lock);
>
> Looks like exit_browser() can be called even when setup_browser()
> was never called.
>
> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
> pthread won't notice.
Memory sanitizer will notice some cases of this and so I didn't want
to code defensively around exit being called ahead of setup.
Thanks,
Ian
> > }
> > diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> > index 298d6af82fdd..db4952f5990b 100644
> > --- a/tools/perf/ui/tui/helpline.c
> > +++ b/tools/perf/ui/tui/helpline.c
> > @@ -2,7 +2,6 @@
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > -#include <pthread.h>
> > #include <linux/kernel.h>
> > #include <linux/string.h>
> >
> > @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> > int ret;
> > static int backlog;
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > ret = vscnprintf(ui_helpline__last_msg + backlog,
> > sizeof(ui_helpline__last_msg) - backlog, format, ap);
> > backlog += ret;
> > @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> > SLsmg_refresh();
> > backlog = 0;
> > }
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > return ret;
> > }
> > diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> > index 3d74af5a7ece..71b6c8d9474f 100644
> > --- a/tools/perf/ui/tui/progress.c
> > +++ b/tools/perf/ui/tui/progress.c
> > @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> > }
> >
> > ui__refresh_dimensions(false);
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > y = SLtt_Screen_Rows / 2 - 2;
> > SLsmg_set_color(0);
> > SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> > @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> > bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> > SLsmg_fill_region(y, 1, 1, bar, ' ');
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > static void tui_progress__finish(void)
> > @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> > return;
> >
> > ui__refresh_dimensions(false);
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > y = SLtt_Screen_Rows / 2 - 2;
> > SLsmg_set_color(0);
> > SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > static struct ui_progress_ops tui_progress__ops = {
> > diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> > index b1be59b4e2a4..a3b8c397c24d 100644
> > --- a/tools/perf/ui/tui/setup.c
> > +++ b/tools/perf/ui/tui/setup.c
> > @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> > {
> > if (force || ui__need_resize) {
> > ui__need_resize = 0;
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > SLtt_get_screen_size();
> > SLsmg_reinit_smg();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> > }
> >
> > @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> > "Press any key...", 0);
> >
> > SLtt_set_cursor_visibility(1);
> > - if (!pthread_mutex_trylock(&ui__lock)) {
> > + if (mutex_trylock(&ui__lock)) {
> > SLsmg_refresh();
> > SLsmg_reset_smg();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> > SLang_reset_tty();
> > perf_error__unregister(&perf_tui_eops);
> > diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> > index 0f562e2cb1e8..3c5174854ac8 100644
> > --- a/tools/perf/ui/tui/util.c
> > +++ b/tools/perf/ui/tui/util.c
> > @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> > t = sep + 1;
> > }
> >
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> >
> > max_len += 2;
> > nr_lines += 8;
> > @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> > SLsmg_write_nstring((char *)exit_msg, max_len);
> > SLsmg_refresh();
> >
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > x += 2;
> > len = 0;
> > key = ui__getch(delay_secs);
> > while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> >
> > if (key == K_BKSPC) {
> > if (len == 0) {
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > goto next_key;
> > }
> > SLsmg_gotorc(y, x + --len);
> > @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> > }
> > SLsmg_refresh();
> >
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> >
> > /* XXX more graceful overflow handling needed */
> > if (len == sizeof(buf) - 1) {
> > @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
> >
> > void ui__info_window(const char *title, const char *text)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui__info_window(title, text, NULL);
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > }
> >
> > int ui__question_window(const char *title, const char *text,
> > const char *exit_msg, int delay_secs)
> > {
> > - pthread_mutex_lock(&ui__lock);
> > + mutex_lock(&ui__lock);
> > __ui__info_window(title, text, exit_msg);
> > SLsmg_refresh();
> > - pthread_mutex_unlock(&ui__lock);
> > + mutex_unlock(&ui__lock);
> > return ui__getch(delay_secs);
> > }
> >
> > diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> > index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> > --- a/tools/perf/ui/ui.h
> > +++ b/tools/perf/ui/ui.h
> > @@ -2,11 +2,11 @@
> > #ifndef _PERF_UI_H_
> > #define _PERF_UI_H_ 1
> >
> > -#include <pthread.h>
> > +#include "../util/mutex.h"
> > #include <stdbool.h>
> > #include <linux/compiler.h>
> >
> > -extern pthread_mutex_t ui__lock;
> > +extern struct mutex ui__lock;
> > extern void *perf_gtk_handle;
> >
> > extern int use_browser;
>
On Fri, Aug 26, 2022 at 9:45 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Wed, Aug 24, 2022 at 08:38:58AM -0700, Ian Rogers escreveu:
> > Add thread safety annotations to struct mutex so that when compiled with
> > clang's -Wthread-safety warnings are generated for erroneous lock
> > patterns. NO_THREAD_SAFETY_ANALYSIS is needed for
> > mutex_lock/mutex_unlock as the analysis doesn't under pthread calls.
>
> So even having the guards checking if the attribute is available it
> seems at least clang 11.0 is needed, as the "lockable" arg was
> introduced there:
>
> 37 42.61 fedora:32 : FAIL clang version 10.0.1 (Fedora 10.0.1-3.fc32)
> In file included from /git/perf-6.0.0-rc2/tools/perf/util/../ui/ui.h:5:
> /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:74:8: error: invalid capability name 'lockable'; capability name must be 'mutex' or 'role' [-Werror,-Wthread-safety-attributes]
> struct LOCKABLE mutex {
> ^
> /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:35:44: note: expanded from macro 'LOCKABLE'
> #define LOCKABLE __attribute__((capability("lockable")))
capability("lockable") can just be lockable, the capability
generalization came in a later clang release.
That is change:
#define LOCKABLE __attribute__((capability("lockable")))
to:
#define LOCKABLE __attribute__((lockable))
and later clangs take the earlier name. Do you want a v5 for this 1 liner?
Thanks,
Ian
> The next fedora releases are ok with it:
>
> 38 124.89 fedora:33 : Ok gcc (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1) , clang version 11.0.0 (Fedora 11.0.0-3.fc33)
> 39 132.20 fedora:34 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 12.0.1 (Fedora 12.0.1-1.fc34)
> 40 21.44 fedora:34-x-ARC-glibc : Ok arc-linux-gcc (ARC HS GNU/Linux glibc toolchain 2019.03-rc1) 8.3.1 20190225
> 41 19.43 fedora:34-x-ARC-uClibc : Ok arc-linux-gcc (ARCv2 ISA Linux uClibc toolchain 2019.03-rc1) 8.3.1 20190225
> 42 136.33 fedora:35 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 13.0.0 (Fedora 13.0.0-3.fc35)
> 43 137.73 fedora:36 : Ok gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1) , clang version 14.0.0 (Fedora 14.0.0-1.fc36)
> 44 140.45 fedora:37 : Ok gcc (GCC) 12.1.1 20220628 (Red Hat 12.1.1-3) , clang version 14.0.5 (Fedora 14.0.5-6.fc37)
> 45 126.80 fedora:38 : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38)
> 46 127.00 fedora:rawhide : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38)
>
> Is there a way to check if a "capability" is available for the
> attribute? Otherwise we can additionally check the clang version?
>
> For my tests I'll make clang 11.0 to be the oldest supported compiler
> (i.e. just disable building with older versions), but wanted to let you
> know since you try to check if the attribute is available and fallback
> to doing nothing in that case.
>
> - Arnaldo
>
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/mutex.c | 2 ++
> > tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 69 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> > index 892294ac1769..ec813093276d 100644
> > --- a/tools/perf/util/mutex.c
> > +++ b/tools/perf/util/mutex.c
> > @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx)
> > }
> >
> > void mutex_lock(struct mutex *mtx)
> > + NO_THREAD_SAFETY_ANALYSIS
> > {
> > CHECK_ERR(pthread_mutex_lock(&mtx->lock));
> > }
> >
> > void mutex_unlock(struct mutex *mtx)
> > + NO_THREAD_SAFETY_ANALYSIS
> > {
> > CHECK_ERR(pthread_mutex_unlock(&mtx->lock));
> > }
> > diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> > index c9e110a2b55e..48a2d87598f0 100644
> > --- a/tools/perf/util/mutex.h
> > +++ b/tools/perf/util/mutex.h
> > @@ -5,11 +5,73 @@
> > #include <pthread.h>
> > #include <stdbool.h>
> >
> > +/*
> > + * A function-like feature checking macro that is a wrapper around
> > + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a
> > + * nonzero constant integer if the attribute is supported or 0 if not.
> > + */
> > +#ifdef __has_attribute
> > +#define HAVE_ATTRIBUTE(x) __has_attribute(x)
> > +#else
> > +#define HAVE_ATTRIBUTE(x) 0
> > +#endif
> > +
> > +
> > +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \
> > + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \
> > + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \
> > + HAVE_ATTRIBUTE(no_thread_safety_analysis)
> > +
> > +/* Documents if a shared field or global variable needs to be protected by a mutex. */
> > +#define GUARDED_BY(x) __attribute__((guarded_by(x)))
> > +
> > +/*
> > + * Documents if the memory location pointed to by a pointer should be guarded by
> > + * a mutex when dereferencing the pointer.
> > + */
> > +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
> > +
> > +/* Documents if a type is a lockable type. */
> > +#define LOCKABLE __attribute__((capability("lockable")))
> > +
> > +/* Documents functions that acquire a lock in the body of a function, and do not release it. */
> > +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
> > +
> > +/*
> > + * Documents functions that expect a lock to be held on entry to the function,
> > + * and release it in the body of the function.
> > + */
> > +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
> > +
> > +/* Documents functions that try to acquire a lock, and return success or failure. */
> > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \
> > + __attribute__((exclusive_trylock_function(__VA_ARGS__)))
> > +
> > +
> > +/* Documents a function that expects a mutex to be held prior to entry. */
> > +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
> > +
> > +/* Turns off thread safety checking within the body of a particular function. */
> > +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
> > +
> > +#else
> > +
> > +#define GUARDED_BY(x)
> > +#define PT_GUARDED_BY(x)
> > +#define LOCKABLE
> > +#define EXCLUSIVE_LOCK_FUNCTION(...)
> > +#define UNLOCK_FUNCTION(...)
> > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...)
> > +#define EXCLUSIVE_LOCKS_REQUIRED(...)
> > +#define NO_THREAD_SAFETY_ANALYSIS
> > +
> > +#endif
> > +
> > /*
> > * A wrapper around the mutex implementation that allows perf to error check
> > * usage, etc.
> > */
> > -struct mutex {
> > +struct LOCKABLE mutex {
> > pthread_mutex_t lock;
> > };
> >
> > @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx);
> > void mutex_init_pshared(struct mutex *mtx);
> > void mutex_destroy(struct mutex *mtx);
> >
> > -void mutex_lock(struct mutex *mtx);
> > -void mutex_unlock(struct mutex *mtx);
> > -bool mutex_trylock(struct mutex *mtx);
> > +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx);
> > +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx);
> > +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx);
> >
> > /* Default initialize the cond struct. */
> > void cond_init(struct cond *cnd);
> > @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd);
> > void cond_init_pshared(struct cond *cnd);
> > void cond_destroy(struct cond *cnd);
> >
> > -void cond_wait(struct cond *cnd, struct mutex *mtx);
> > +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx);
> > void cond_signal(struct cond *cnd);
> > void cond_broadcast(struct cond *cnd);
> >
> > --
> > 2.37.2.609.g9ff673ca1a-goog
>
> --
>
> - Arnaldo
On 26/08/22 19:02, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <[email protected]> wrote:
>>
>> On 24/08/22 18:38, Ian Rogers wrote:
>>> Switch to the use of mutex wrappers that provide better error checking.
>>>
>>> Signed-off-by: Ian Rogers <[email protected]>
>>> ---
>>> tools/perf/ui/browser.c | 20 ++++++++++----------
>>> tools/perf/ui/browsers/annotate.c | 2 +-
>>> tools/perf/ui/setup.c | 5 +++--
>>> tools/perf/ui/tui/helpline.c | 5 ++---
>>> tools/perf/ui/tui/progress.c | 8 ++++----
>>> tools/perf/ui/tui/setup.c | 8 ++++----
>>> tools/perf/ui/tui/util.c | 18 +++++++++---------
>>> tools/perf/ui/ui.h | 4 ++--
>>> 8 files changed, 35 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>>> index fa5bd5c20e96..78fb01d6ad63 100644
>>> --- a/tools/perf/ui/browser.c
>>> +++ b/tools/perf/ui/browser.c
>>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>
>>> void ui_browser__show_title(struct ui_browser *browser, const char *title)
>>> {
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> __ui_browser__show_title(browser, title);
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>>
>>> int ui_browser__show(struct ui_browser *browser, const char *title,
>>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>
>>> browser->refresh_dimensions(browser);
>>>
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> __ui_browser__show_title(browser, title);
>>>
>>> browser->title = title;
>>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>> va_end(ap);
>>> if (err > 0)
>>> ui_helpline__push(browser->helpline);
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> return err ? 0 : -1;
>>> }
>>>
>>> void ui_browser__hide(struct ui_browser *browser)
>>> {
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> ui_helpline__pop();
>>> zfree(&browser->helpline);
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>>
>>> static void ui_browser__scrollbar_set(struct ui_browser *browser)
>>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>>>
>>> int ui_browser__refresh(struct ui_browser *browser)
>>> {
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> __ui_browser__refresh(browser);
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>>
>>> return 0;
>>> }
>>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>>> while (1) {
>>> off_t offset;
>>>
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> err = __ui_browser__refresh(browser);
>>> SLsmg_refresh();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> if (err < 0)
>>> break;
>>>
>>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>>> index 44ba900828f6..b8747e8dd9ea 100644
>>> --- a/tools/perf/ui/browsers/annotate.c
>>> +++ b/tools/perf/ui/browsers/annotate.c
>>> @@ -8,11 +8,11 @@
>>> #include "../../util/hist.h"
>>> #include "../../util/sort.h"
>>> #include "../../util/map.h"
>>> +#include "../../util/mutex.h"
>>> #include "../../util/symbol.h"
>>> #include "../../util/evsel.h"
>>> #include "../../util/evlist.h"
>>> #include <inttypes.h>
>>> -#include <pthread.h>
>>> #include <linux/kernel.h>
>>> #include <linux/string.h>
>>> #include <linux/zalloc.h>
>>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>>> index 700335cde618..25ded88801a3 100644
>>> --- a/tools/perf/ui/setup.c
>>> +++ b/tools/perf/ui/setup.c
>>> @@ -1,5 +1,4 @@
>>> // SPDX-License-Identifier: GPL-2.0
>>> -#include <pthread.h>
>>> #include <dlfcn.h>
>>> #include <unistd.h>
>>>
>>> @@ -8,7 +7,7 @@
>>> #include "../util/hist.h"
>>> #include "ui.h"
>>>
>>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
>>> +struct mutex ui__lock;
>>> void *perf_gtk_handle;
>>> int use_browser = -1;
>>>
>>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>>>
>>> void setup_browser(bool fallback_to_pager)
>>> {
>>> + mutex_init(&ui__lock);
>>> if (use_browser < 2 && (!isatty(1) || dump_trace))
>>> use_browser = 0;
>>>
>>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>>> default:
>>> break;
>>> }
>>> + mutex_destroy(&ui__lock);
>>
>> Looks like exit_browser() can be called even when setup_browser()
>> was never called.
>>
>> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
>> pthread won't notice.
>
> Memory sanitizer will notice some cases of this and so I didn't want
> to code defensively around exit being called ahead of setup.
I am not sure you understood.
As I wrote, exit_browser() can be called even when setup_browser()
was never called, so it is not defensive programming, it is necessary
programming that you only get away without doing because
PTHREAD_MUTEX_INITIALIZER is all zeros.
>
> Thanks,
> Ian
>
>>> }
>>> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
>>> index 298d6af82fdd..db4952f5990b 100644
>>> --- a/tools/perf/ui/tui/helpline.c
>>> +++ b/tools/perf/ui/tui/helpline.c
>>> @@ -2,7 +2,6 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> -#include <pthread.h>
>>> #include <linux/kernel.h>
>>> #include <linux/string.h>
>>>
>>> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>>> int ret;
>>> static int backlog;
>>>
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> ret = vscnprintf(ui_helpline__last_msg + backlog,
>>> sizeof(ui_helpline__last_msg) - backlog, format, ap);
>>> backlog += ret;
>>> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>>> SLsmg_refresh();
>>> backlog = 0;
>>> }
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>>
>>> return ret;
>>> }
>>> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
>>> index 3d74af5a7ece..71b6c8d9474f 100644
>>> --- a/tools/perf/ui/tui/progress.c
>>> +++ b/tools/perf/ui/tui/progress.c
>>> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
>>> }
>>>
>>> ui__refresh_dimensions(false);
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> y = SLtt_Screen_Rows / 2 - 2;
>>> SLsmg_set_color(0);
>>> SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
>>> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
>>> bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
>>> SLsmg_fill_region(y, 1, 1, bar, ' ');
>>> SLsmg_refresh();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>>
>>> static void tui_progress__finish(void)
>>> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
>>> return;
>>>
>>> ui__refresh_dimensions(false);
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> y = SLtt_Screen_Rows / 2 - 2;
>>> SLsmg_set_color(0);
>>> SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
>>> SLsmg_refresh();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>>
>>> static struct ui_progress_ops tui_progress__ops = {
>>> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
>>> index b1be59b4e2a4..a3b8c397c24d 100644
>>> --- a/tools/perf/ui/tui/setup.c
>>> +++ b/tools/perf/ui/tui/setup.c
>>> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
>>> {
>>> if (force || ui__need_resize) {
>>> ui__need_resize = 0;
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> SLtt_get_screen_size();
>>> SLsmg_reinit_smg();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>> }
>>>
>>> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
>>> "Press any key...", 0);
>>>
>>> SLtt_set_cursor_visibility(1);
>>> - if (!pthread_mutex_trylock(&ui__lock)) {
>>> + if (mutex_trylock(&ui__lock)) {
>>> SLsmg_refresh();
>>> SLsmg_reset_smg();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>> SLang_reset_tty();
>>> perf_error__unregister(&perf_tui_eops);
>>> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
>>> index 0f562e2cb1e8..3c5174854ac8 100644
>>> --- a/tools/perf/ui/tui/util.c
>>> +++ b/tools/perf/ui/tui/util.c
>>> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>>> t = sep + 1;
>>> }
>>>
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>>
>>> max_len += 2;
>>> nr_lines += 8;
>>> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>>> SLsmg_write_nstring((char *)exit_msg, max_len);
>>> SLsmg_refresh();
>>>
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>>
>>> x += 2;
>>> len = 0;
>>> key = ui__getch(delay_secs);
>>> while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>>
>>> if (key == K_BKSPC) {
>>> if (len == 0) {
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> goto next_key;
>>> }
>>> SLsmg_gotorc(y, x + --len);
>>> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>>> }
>>> SLsmg_refresh();
>>>
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>>
>>> /* XXX more graceful overflow handling needed */
>>> if (len == sizeof(buf) - 1) {
>>> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
>>>
>>> void ui__info_window(const char *title, const char *text)
>>> {
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> __ui__info_window(title, text, NULL);
>>> SLsmg_refresh();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> }
>>>
>>> int ui__question_window(const char *title, const char *text,
>>> const char *exit_msg, int delay_secs)
>>> {
>>> - pthread_mutex_lock(&ui__lock);
>>> + mutex_lock(&ui__lock);
>>> __ui__info_window(title, text, exit_msg);
>>> SLsmg_refresh();
>>> - pthread_mutex_unlock(&ui__lock);
>>> + mutex_unlock(&ui__lock);
>>> return ui__getch(delay_secs);
>>> }
>>>
>>> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
>>> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
>>> --- a/tools/perf/ui/ui.h
>>> +++ b/tools/perf/ui/ui.h
>>> @@ -2,11 +2,11 @@
>>> #ifndef _PERF_UI_H_
>>> #define _PERF_UI_H_ 1
>>>
>>> -#include <pthread.h>
>>> +#include "../util/mutex.h"
>>> #include <stdbool.h>
>>> #include <linux/compiler.h>
>>>
>>> -extern pthread_mutex_t ui__lock;
>>> +extern struct mutex ui__lock;
>>> extern void *perf_gtk_handle;
>>>
>>> extern int use_browser;
>>
On Fri, Aug 26, 2022 at 10:22 AM Adrian Hunter <[email protected]> wrote:
>
> On 26/08/22 19:02, Ian Rogers wrote:
> > On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <[email protected]> wrote:
> >>
> >> On 24/08/22 18:38, Ian Rogers wrote:
> >>> Switch to the use of mutex wrappers that provide better error checking.
> >>>
> >>> Signed-off-by: Ian Rogers <[email protected]>
> >>> ---
> >>> tools/perf/ui/browser.c | 20 ++++++++++----------
> >>> tools/perf/ui/browsers/annotate.c | 2 +-
> >>> tools/perf/ui/setup.c | 5 +++--
> >>> tools/perf/ui/tui/helpline.c | 5 ++---
> >>> tools/perf/ui/tui/progress.c | 8 ++++----
> >>> tools/perf/ui/tui/setup.c | 8 ++++----
> >>> tools/perf/ui/tui/util.c | 18 +++++++++---------
> >>> tools/perf/ui/ui.h | 4 ++--
> >>> 8 files changed, 35 insertions(+), 35 deletions(-)
> >>>
> >>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> >>> index fa5bd5c20e96..78fb01d6ad63 100644
> >>> --- a/tools/perf/ui/browser.c
> >>> +++ b/tools/perf/ui/browser.c
> >>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
> >>>
> >>> void ui_browser__show_title(struct ui_browser *browser, const char *title)
> >>> {
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> __ui_browser__show_title(browser, title);
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>>
> >>> int ui_browser__show(struct ui_browser *browser, const char *title,
> >>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >>>
> >>> browser->refresh_dimensions(browser);
> >>>
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> __ui_browser__show_title(browser, title);
> >>>
> >>> browser->title = title;
> >>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >>> va_end(ap);
> >>> if (err > 0)
> >>> ui_helpline__push(browser->helpline);
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> return err ? 0 : -1;
> >>> }
> >>>
> >>> void ui_browser__hide(struct ui_browser *browser)
> >>> {
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> ui_helpline__pop();
> >>> zfree(&browser->helpline);
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>>
> >>> static void ui_browser__scrollbar_set(struct ui_browser *browser)
> >>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
> >>>
> >>> int ui_browser__refresh(struct ui_browser *browser)
> >>> {
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> __ui_browser__refresh(browser);
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>>
> >>> return 0;
> >>> }
> >>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> >>> while (1) {
> >>> off_t offset;
> >>>
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> err = __ui_browser__refresh(browser);
> >>> SLsmg_refresh();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> if (err < 0)
> >>> break;
> >>>
> >>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> >>> index 44ba900828f6..b8747e8dd9ea 100644
> >>> --- a/tools/perf/ui/browsers/annotate.c
> >>> +++ b/tools/perf/ui/browsers/annotate.c
> >>> @@ -8,11 +8,11 @@
> >>> #include "../../util/hist.h"
> >>> #include "../../util/sort.h"
> >>> #include "../../util/map.h"
> >>> +#include "../../util/mutex.h"
> >>> #include "../../util/symbol.h"
> >>> #include "../../util/evsel.h"
> >>> #include "../../util/evlist.h"
> >>> #include <inttypes.h>
> >>> -#include <pthread.h>
> >>> #include <linux/kernel.h>
> >>> #include <linux/string.h>
> >>> #include <linux/zalloc.h>
> >>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> >>> index 700335cde618..25ded88801a3 100644
> >>> --- a/tools/perf/ui/setup.c
> >>> +++ b/tools/perf/ui/setup.c
> >>> @@ -1,5 +1,4 @@
> >>> // SPDX-License-Identifier: GPL-2.0
> >>> -#include <pthread.h>
> >>> #include <dlfcn.h>
> >>> #include <unistd.h>
> >>>
> >>> @@ -8,7 +7,7 @@
> >>> #include "../util/hist.h"
> >>> #include "ui.h"
> >>>
> >>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> >>> +struct mutex ui__lock;
> >>> void *perf_gtk_handle;
> >>> int use_browser = -1;
> >>>
> >>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> >>>
> >>> void setup_browser(bool fallback_to_pager)
> >>> {
> >>> + mutex_init(&ui__lock);
> >>> if (use_browser < 2 && (!isatty(1) || dump_trace))
> >>> use_browser = 0;
> >>>
> >>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> >>> default:
> >>> break;
> >>> }
> >>> + mutex_destroy(&ui__lock);
> >>
> >> Looks like exit_browser() can be called even when setup_browser()
> >> was never called.
> >>
> >> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
> >> pthread won't notice.
> >
> > Memory sanitizer will notice some cases of this and so I didn't want
> > to code defensively around exit being called ahead of setup.
>
> I am not sure you understood.
>
> As I wrote, exit_browser() can be called even when setup_browser()
> was never called, so it is not defensive programming, it is necessary
> programming that you only get away without doing because
> PTHREAD_MUTEX_INITIALIZER is all zeros.
Why are we here:
1) there is a memory leak
2) I fix the memory and trigger a use after free
3) I invent a reference count checker, use it to fix the memory leak,
use after free and missing locks - the patch for this in 10s of lines
long
4) when adding the lock fixes I defensively add error checking to the
mutex involved - mainly because I was scared I could introduce a
deadlock
5) I get asked to generalize this
6) GSoC contributor picks up and puts this down
7) I pull together the contributor's work
8) I get asked to turn a search and replace 4 patch fix into an unwieldy patch
9) I worry about the sanity of the change and add lock checking from clang
10) I end up trying to fix perf-sched who for some reason thought it
was perfectly valid to have threads blocked on mutexes that were
deallocated on the stack.
11) the UI code was written with a view that exiting something not
setup somehow made sense
I am drawing a line at fixing perf sched and the UI code. We can drop
this patch and keep things as a pthread_mutex_t, similarly for
perf-sched. I have gone about as far as I'm prepared to for the sake
of a 10s of line memory leak fix. Some private thoughts are, it would
be useful if review comments could be constructive, hey do this not
that, and not simply commenting on change or trying to shoehorn vast
amounts of tech debt clean up onto simple fixes.
Thanks,
Ian
> >
> > Thanks,
> > Ian
> >
> >>> }
> >>> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> >>> index 298d6af82fdd..db4952f5990b 100644
> >>> --- a/tools/perf/ui/tui/helpline.c
> >>> +++ b/tools/perf/ui/tui/helpline.c
> >>> @@ -2,7 +2,6 @@
> >>> #include <stdio.h>
> >>> #include <stdlib.h>
> >>> #include <string.h>
> >>> -#include <pthread.h>
> >>> #include <linux/kernel.h>
> >>> #include <linux/string.h>
> >>>
> >>> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >>> int ret;
> >>> static int backlog;
> >>>
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> ret = vscnprintf(ui_helpline__last_msg + backlog,
> >>> sizeof(ui_helpline__last_msg) - backlog, format, ap);
> >>> backlog += ret;
> >>> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >>> SLsmg_refresh();
> >>> backlog = 0;
> >>> }
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>>
> >>> return ret;
> >>> }
> >>> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> >>> index 3d74af5a7ece..71b6c8d9474f 100644
> >>> --- a/tools/perf/ui/tui/progress.c
> >>> +++ b/tools/perf/ui/tui/progress.c
> >>> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> >>> }
> >>>
> >>> ui__refresh_dimensions(false);
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> y = SLtt_Screen_Rows / 2 - 2;
> >>> SLsmg_set_color(0);
> >>> SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> >>> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> >>> bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> >>> SLsmg_fill_region(y, 1, 1, bar, ' ');
> >>> SLsmg_refresh();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>>
> >>> static void tui_progress__finish(void)
> >>> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> >>> return;
> >>>
> >>> ui__refresh_dimensions(false);
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> y = SLtt_Screen_Rows / 2 - 2;
> >>> SLsmg_set_color(0);
> >>> SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> >>> SLsmg_refresh();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>>
> >>> static struct ui_progress_ops tui_progress__ops = {
> >>> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> >>> index b1be59b4e2a4..a3b8c397c24d 100644
> >>> --- a/tools/perf/ui/tui/setup.c
> >>> +++ b/tools/perf/ui/tui/setup.c
> >>> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> >>> {
> >>> if (force || ui__need_resize) {
> >>> ui__need_resize = 0;
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> SLtt_get_screen_size();
> >>> SLsmg_reinit_smg();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>> }
> >>>
> >>> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> >>> "Press any key...", 0);
> >>>
> >>> SLtt_set_cursor_visibility(1);
> >>> - if (!pthread_mutex_trylock(&ui__lock)) {
> >>> + if (mutex_trylock(&ui__lock)) {
> >>> SLsmg_refresh();
> >>> SLsmg_reset_smg();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>> SLang_reset_tty();
> >>> perf_error__unregister(&perf_tui_eops);
> >>> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> >>> index 0f562e2cb1e8..3c5174854ac8 100644
> >>> --- a/tools/perf/ui/tui/util.c
> >>> +++ b/tools/perf/ui/tui/util.c
> >>> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >>> t = sep + 1;
> >>> }
> >>>
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>>
> >>> max_len += 2;
> >>> nr_lines += 8;
> >>> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >>> SLsmg_write_nstring((char *)exit_msg, max_len);
> >>> SLsmg_refresh();
> >>>
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>>
> >>> x += 2;
> >>> len = 0;
> >>> key = ui__getch(delay_secs);
> >>> while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>>
> >>> if (key == K_BKSPC) {
> >>> if (len == 0) {
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> goto next_key;
> >>> }
> >>> SLsmg_gotorc(y, x + --len);
> >>> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >>> }
> >>> SLsmg_refresh();
> >>>
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>>
> >>> /* XXX more graceful overflow handling needed */
> >>> if (len == sizeof(buf) - 1) {
> >>> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
> >>>
> >>> void ui__info_window(const char *title, const char *text)
> >>> {
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> __ui__info_window(title, text, NULL);
> >>> SLsmg_refresh();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> }
> >>>
> >>> int ui__question_window(const char *title, const char *text,
> >>> const char *exit_msg, int delay_secs)
> >>> {
> >>> - pthread_mutex_lock(&ui__lock);
> >>> + mutex_lock(&ui__lock);
> >>> __ui__info_window(title, text, exit_msg);
> >>> SLsmg_refresh();
> >>> - pthread_mutex_unlock(&ui__lock);
> >>> + mutex_unlock(&ui__lock);
> >>> return ui__getch(delay_secs);
> >>> }
> >>>
> >>> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> >>> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> >>> --- a/tools/perf/ui/ui.h
> >>> +++ b/tools/perf/ui/ui.h
> >>> @@ -2,11 +2,11 @@
> >>> #ifndef _PERF_UI_H_
> >>> #define _PERF_UI_H_ 1
> >>>
> >>> -#include <pthread.h>
> >>> +#include "../util/mutex.h"
> >>> #include <stdbool.h>
> >>> #include <linux/compiler.h>
> >>>
> >>> -extern pthread_mutex_t ui__lock;
> >>> +extern struct mutex ui__lock;
> >>> extern void *perf_gtk_handle;
> >>>
> >>> extern int use_browser;
> >>
>
On Fri, Aug 26, 2022 at 10:34 AM Adrian Hunter <[email protected]> wrote:
>
> On 26/08/22 19:05, Ian Rogers wrote:
> > On Fri, Aug 26, 2022 at 3:37 AM Adrian Hunter <[email protected]> wrote:
> >>
> >> On 24/08/22 18:38, Ian Rogers wrote:
> >>> Switch to the use of mutex wrappers that provide better error checking.
> >>>
> >>> Signed-off-by: Ian Rogers <[email protected]>
> >>> ---
> >>> tools/perf/util/dso.c | 12 ++++++------
> >>
> >> Some not done yet
> >>
> >> $ grep -i pthread_mut tools/perf/util/dso.c
> >> static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
> >> pthread_mutex_lock(&dso__data_open_lock);
> >> pthread_mutex_unlock(&dso__data_open_lock);
> >> if (pthread_mutex_lock(&dso__data_open_lock) < 0)
> >> pthread_mutex_unlock(&dso__data_open_lock);
> >> pthread_mutex_unlock(&dso__data_open_lock);
> >> pthread_mutex_lock(&dso__data_open_lock);
> >> pthread_mutex_unlock(&dso__data_open_lock);
> >> pthread_mutex_lock(&dso__data_open_lock);
> >> pthread_mutex_unlock(&dso__data_open_lock);
> >
> > Yes, these are all solely dso__data_open_lock that lacks any clear
> > init/exit code to place the initialization/destruction hooks onto. I
> > don't plan to alter these in this patch set.
>
> Perhaps that could be explained in the change log.
>
> But why not just add init / exit code. Could be called out
> of main(), or maybe use __attribute__((constructor)) /
> __attribute__((destructor))
Because the lock is global and not part of the dso.
Thanks,
Ian
On Fri, Aug 26, 2022 at 10:41 AM Adrian Hunter <[email protected]> wrote:
>
> On 26/08/22 19:06, Ian Rogers wrote:
> > On Fri, Aug 26, 2022 at 5:12 AM Adrian Hunter <[email protected]> wrote:
> >>
> >> On 24/08/22 18:38, Ian Rogers wrote:
> >>> Add annotations to describe lock behavior. Add unlocks so that mutexes
> >>> aren't conditionally held on exit from perf_sched__replay. Add an exit
> >>> variable so that thread_func can terminate, rather than leaving the
> >>> threads blocked on mutexes.
> >>>
> >>> Signed-off-by: Ian Rogers <[email protected]>
> >>> ---
> >>> tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
> >>> 1 file changed, 29 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> >>> index 7e4006d6b8bc..b483ff0d432e 100644
> >>> --- a/tools/perf/builtin-sched.c
> >>> +++ b/tools/perf/builtin-sched.c
> >>> @@ -246,6 +246,7 @@ struct perf_sched {
> >>> const char *time_str;
> >>> struct perf_time_interval ptime;
> >>> struct perf_time_interval hist_time;
> >>> + volatile bool thread_funcs_exit;
> >>> };
> >>>
> >>> /* per thread run time data */
> >>> @@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
> >>> prctl(PR_SET_NAME, comm2);
> >>> if (fd < 0)
> >>> return NULL;
> >>> -again:
> >>> - ret = sem_post(&this_task->ready_for_work);
> >>> - BUG_ON(ret);
> >>> - mutex_lock(&sched->start_work_mutex);
> >>> - mutex_unlock(&sched->start_work_mutex);
> >>>
> >>> - cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> >>> + while (!sched->thread_funcs_exit) {
> >>> + ret = sem_post(&this_task->ready_for_work);
> >>> + BUG_ON(ret);
> >>> + mutex_lock(&sched->start_work_mutex);
> >>> + mutex_unlock(&sched->start_work_mutex);
> >>>
> >>> - for (i = 0; i < this_task->nr_events; i++) {
> >>> - this_task->curr_event = i;
> >>> - perf_sched__process_event(sched, this_task->atoms[i]);
> >>> - }
> >>> + cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> >>>
> >>> - cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> >>> - this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> >>> - ret = sem_post(&this_task->work_done_sem);
> >>> - BUG_ON(ret);
> >>> + for (i = 0; i < this_task->nr_events; i++) {
> >>> + this_task->curr_event = i;
> >>> + perf_sched__process_event(sched, this_task->atoms[i]);
> >>> + }
> >>>
> >>> - mutex_lock(&sched->work_done_wait_mutex);
> >>> - mutex_unlock(&sched->work_done_wait_mutex);
> >>> + cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> >>> + this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> >>> + ret = sem_post(&this_task->work_done_sem);
> >>> + BUG_ON(ret);
> >>>
> >>> - goto again;
> >>> + mutex_lock(&sched->work_done_wait_mutex);
> >>> + mutex_unlock(&sched->work_done_wait_mutex);
> >>> + }
> >>> + return NULL;
> >>> }
> >>>
> >>> static void create_tasks(struct perf_sched *sched)
> >>> + EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
> >>> + EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
> >>> {
> >>> struct task_desc *task;
> >>> pthread_attr_t attr;
> >>> @@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
> >>> }
> >>>
> >>> static void wait_for_tasks(struct perf_sched *sched)
> >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> >>> {
> >>> u64 cpu_usage_0, cpu_usage_1;
> >>> struct task_desc *task;
> >>> @@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
> >>> }
> >>>
> >>> static void run_one_test(struct perf_sched *sched)
> >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> >>> {
> >>> u64 T0, T1, delta, avg_delta, fluct;
> >>>
> >>> @@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
> >>> print_task_traces(sched);
> >>> add_cross_task_wakeups(sched);
> >>>
> >>> + sched->thread_funcs_exit = false;
> >>> create_tasks(sched);
> >>> printf("------------------------------------------------------------\n");
> >>> for (i = 0; i < sched->replay_repeat; i++)
> >>> run_one_test(sched);
> >>>
> >>> + sched->thread_funcs_exit = true;
> >>> + mutex_unlock(&sched->start_work_mutex);
> >>> + mutex_unlock(&sched->work_done_wait_mutex);
> >>
> >> I think you still need to wait for the threads to exit before
> >> destroying the mutexes.
> >
> > This is a pre-existing issue and beyond the scope of this patch set.
>
> You added the mutex_destroy functions in patch 8, so it is still
> fallout from that.
In the previous code the threads were blocked on mutexes that were
stack allocated and the stack memory went away. You are correct to say
that to those locks I added an init and destroy call. The lifetime of
the mutex was wrong previously and remains wrong in this change.
Thanks,
Ian
> >
> > Thanks,
> > Ian
> >
> >>> return 0;
> >>> }
> >>>
> >>
>
On 26/08/22 19:05, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 3:37 AM Adrian Hunter <[email protected]> wrote:
>>
>> On 24/08/22 18:38, Ian Rogers wrote:
>>> Switch to the use of mutex wrappers that provide better error checking.
>>>
>>> Signed-off-by: Ian Rogers <[email protected]>
>>> ---
>>> tools/perf/util/dso.c | 12 ++++++------
>>
>> Some not done yet
>>
>> $ grep -i pthread_mut tools/perf/util/dso.c
>> static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
>> pthread_mutex_lock(&dso__data_open_lock);
>> pthread_mutex_unlock(&dso__data_open_lock);
>> if (pthread_mutex_lock(&dso__data_open_lock) < 0)
>> pthread_mutex_unlock(&dso__data_open_lock);
>> pthread_mutex_unlock(&dso__data_open_lock);
>> pthread_mutex_lock(&dso__data_open_lock);
>> pthread_mutex_unlock(&dso__data_open_lock);
>> pthread_mutex_lock(&dso__data_open_lock);
>> pthread_mutex_unlock(&dso__data_open_lock);
>
> Yes, these are all solely dso__data_open_lock that lacks any clear
> init/exit code to place the initialization/destruction hooks onto. I
> don't plan to alter these in this patch set.
Perhaps that could be explained in the change log.
But why not just add init / exit code. Could be called out
of main(), or maybe use __attribute__((constructor)) /
__attribute__((destructor))
On 26/08/22 19:06, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 5:12 AM Adrian Hunter <[email protected]> wrote:
>>
>> On 24/08/22 18:38, Ian Rogers wrote:
>>> Add annotations to describe lock behavior. Add unlocks so that mutexes
>>> aren't conditionally held on exit from perf_sched__replay. Add an exit
>>> variable so that thread_func can terminate, rather than leaving the
>>> threads blocked on mutexes.
>>>
>>> Signed-off-by: Ian Rogers <[email protected]>
>>> ---
>>> tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
>>> 1 file changed, 29 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>>> index 7e4006d6b8bc..b483ff0d432e 100644
>>> --- a/tools/perf/builtin-sched.c
>>> +++ b/tools/perf/builtin-sched.c
>>> @@ -246,6 +246,7 @@ struct perf_sched {
>>> const char *time_str;
>>> struct perf_time_interval ptime;
>>> struct perf_time_interval hist_time;
>>> + volatile bool thread_funcs_exit;
>>> };
>>>
>>> /* per thread run time data */
>>> @@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
>>> prctl(PR_SET_NAME, comm2);
>>> if (fd < 0)
>>> return NULL;
>>> -again:
>>> - ret = sem_post(&this_task->ready_for_work);
>>> - BUG_ON(ret);
>>> - mutex_lock(&sched->start_work_mutex);
>>> - mutex_unlock(&sched->start_work_mutex);
>>>
>>> - cpu_usage_0 = get_cpu_usage_nsec_self(fd);
>>> + while (!sched->thread_funcs_exit) {
>>> + ret = sem_post(&this_task->ready_for_work);
>>> + BUG_ON(ret);
>>> + mutex_lock(&sched->start_work_mutex);
>>> + mutex_unlock(&sched->start_work_mutex);
>>>
>>> - for (i = 0; i < this_task->nr_events; i++) {
>>> - this_task->curr_event = i;
>>> - perf_sched__process_event(sched, this_task->atoms[i]);
>>> - }
>>> + cpu_usage_0 = get_cpu_usage_nsec_self(fd);
>>>
>>> - cpu_usage_1 = get_cpu_usage_nsec_self(fd);
>>> - this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
>>> - ret = sem_post(&this_task->work_done_sem);
>>> - BUG_ON(ret);
>>> + for (i = 0; i < this_task->nr_events; i++) {
>>> + this_task->curr_event = i;
>>> + perf_sched__process_event(sched, this_task->atoms[i]);
>>> + }
>>>
>>> - mutex_lock(&sched->work_done_wait_mutex);
>>> - mutex_unlock(&sched->work_done_wait_mutex);
>>> + cpu_usage_1 = get_cpu_usage_nsec_self(fd);
>>> + this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
>>> + ret = sem_post(&this_task->work_done_sem);
>>> + BUG_ON(ret);
>>>
>>> - goto again;
>>> + mutex_lock(&sched->work_done_wait_mutex);
>>> + mutex_unlock(&sched->work_done_wait_mutex);
>>> + }
>>> + return NULL;
>>> }
>>>
>>> static void create_tasks(struct perf_sched *sched)
>>> + EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
>>> + EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
>>> {
>>> struct task_desc *task;
>>> pthread_attr_t attr;
>>> @@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
>>> }
>>>
>>> static void wait_for_tasks(struct perf_sched *sched)
>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
>>> {
>>> u64 cpu_usage_0, cpu_usage_1;
>>> struct task_desc *task;
>>> @@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
>>> }
>>>
>>> static void run_one_test(struct perf_sched *sched)
>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
>>> {
>>> u64 T0, T1, delta, avg_delta, fluct;
>>>
>>> @@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
>>> print_task_traces(sched);
>>> add_cross_task_wakeups(sched);
>>>
>>> + sched->thread_funcs_exit = false;
>>> create_tasks(sched);
>>> printf("------------------------------------------------------------\n");
>>> for (i = 0; i < sched->replay_repeat; i++)
>>> run_one_test(sched);
>>>
>>> + sched->thread_funcs_exit = true;
>>> + mutex_unlock(&sched->start_work_mutex);
>>> + mutex_unlock(&sched->work_done_wait_mutex);
>>
>> I think you still need to wait for the threads to exit before
>> destroying the mutexes.
>
> This is a pre-existing issue and beyond the scope of this patch set.
You added the mutex_destroy functions in patch 8, so it is still
fallout from that.
>
> Thanks,
> Ian
>
>>> return 0;
>>> }
>>>
>>
On Fri, Aug 26, 2022 at 10:48 AM Ian Rogers <[email protected]> wrote:
>
> On Fri, Aug 26, 2022 at 10:41 AM Adrian Hunter <[email protected]> wrote:
> >
> > On 26/08/22 19:06, Ian Rogers wrote:
> > > On Fri, Aug 26, 2022 at 5:12 AM Adrian Hunter <[email protected]> wrote:
> > >>
> > >> On 24/08/22 18:38, Ian Rogers wrote:
> > >>> Add annotations to describe lock behavior. Add unlocks so that mutexes
> > >>> aren't conditionally held on exit from perf_sched__replay. Add an exit
> > >>> variable so that thread_func can terminate, rather than leaving the
> > >>> threads blocked on mutexes.
> > >>>
> > >>> Signed-off-by: Ian Rogers <[email protected]>
> > >>> ---
> > >>> tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
> > >>> 1 file changed, 29 insertions(+), 17 deletions(-)
> > >>>
> > >>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > >>> index 7e4006d6b8bc..b483ff0d432e 100644
> > >>> --- a/tools/perf/builtin-sched.c
> > >>> +++ b/tools/perf/builtin-sched.c
> > >>> @@ -246,6 +246,7 @@ struct perf_sched {
> > >>> const char *time_str;
> > >>> struct perf_time_interval ptime;
> > >>> struct perf_time_interval hist_time;
> > >>> + volatile bool thread_funcs_exit;
> > >>> };
> > >>>
> > >>> /* per thread run time data */
> > >>> @@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
> > >>> prctl(PR_SET_NAME, comm2);
> > >>> if (fd < 0)
> > >>> return NULL;
> > >>> -again:
> > >>> - ret = sem_post(&this_task->ready_for_work);
> > >>> - BUG_ON(ret);
> > >>> - mutex_lock(&sched->start_work_mutex);
> > >>> - mutex_unlock(&sched->start_work_mutex);
> > >>>
> > >>> - cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> > >>> + while (!sched->thread_funcs_exit) {
> > >>> + ret = sem_post(&this_task->ready_for_work);
> > >>> + BUG_ON(ret);
> > >>> + mutex_lock(&sched->start_work_mutex);
> > >>> + mutex_unlock(&sched->start_work_mutex);
> > >>>
> > >>> - for (i = 0; i < this_task->nr_events; i++) {
> > >>> - this_task->curr_event = i;
> > >>> - perf_sched__process_event(sched, this_task->atoms[i]);
> > >>> - }
> > >>> + cpu_usage_0 = get_cpu_usage_nsec_self(fd);
> > >>>
> > >>> - cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> > >>> - this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> > >>> - ret = sem_post(&this_task->work_done_sem);
> > >>> - BUG_ON(ret);
> > >>> + for (i = 0; i < this_task->nr_events; i++) {
> > >>> + this_task->curr_event = i;
> > >>> + perf_sched__process_event(sched, this_task->atoms[i]);
> > >>> + }
> > >>>
> > >>> - mutex_lock(&sched->work_done_wait_mutex);
> > >>> - mutex_unlock(&sched->work_done_wait_mutex);
> > >>> + cpu_usage_1 = get_cpu_usage_nsec_self(fd);
> > >>> + this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
> > >>> + ret = sem_post(&this_task->work_done_sem);
> > >>> + BUG_ON(ret);
> > >>>
> > >>> - goto again;
> > >>> + mutex_lock(&sched->work_done_wait_mutex);
> > >>> + mutex_unlock(&sched->work_done_wait_mutex);
> > >>> + }
> > >>> + return NULL;
> > >>> }
> > >>>
> > >>> static void create_tasks(struct perf_sched *sched)
> > >>> + EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
> > >>> + EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
> > >>> {
> > >>> struct task_desc *task;
> > >>> pthread_attr_t attr;
> > >>> @@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
> > >>> }
> > >>>
> > >>> static void wait_for_tasks(struct perf_sched *sched)
> > >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> > >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> > >>> {
> > >>> u64 cpu_usage_0, cpu_usage_1;
> > >>> struct task_desc *task;
> > >>> @@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
> > >>> }
> > >>>
> > >>> static void run_one_test(struct perf_sched *sched)
> > >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
> > >>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
> > >>> {
> > >>> u64 T0, T1, delta, avg_delta, fluct;
> > >>>
> > >>> @@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
> > >>> print_task_traces(sched);
> > >>> add_cross_task_wakeups(sched);
> > >>>
> > >>> + sched->thread_funcs_exit = false;
> > >>> create_tasks(sched);
> > >>> printf("------------------------------------------------------------\n");
> > >>> for (i = 0; i < sched->replay_repeat; i++)
> > >>> run_one_test(sched);
> > >>>
> > >>> + sched->thread_funcs_exit = true;
> > >>> + mutex_unlock(&sched->start_work_mutex);
> > >>> + mutex_unlock(&sched->work_done_wait_mutex);
> > >>
> > >> I think you still need to wait for the threads to exit before
> > >> destroying the mutexes.
> > >
> > > This is a pre-existing issue and beyond the scope of this patch set.
> >
> > You added the mutex_destroy functions in patch 8, so it is still
> > fallout from that.
>
> In the previous code the threads were blocked on mutexes that were
> stack allocated and the stack memory went away. You are correct to say
> that to those locks I added an init and destroy call. The lifetime of
> the mutex was wrong previously and remains wrong in this change.
I think you fixed the lifetime issue with sched->thread_funcs_exit here.
All you need to do is calling pthread_join() after the mutex_unlock, no?
Thanks,
Namhyung
Em Fri, Aug 26, 2022 at 10:00:43AM -0700, Ian Rogers escreveu:
> On Fri, Aug 26, 2022 at 9:45 AM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > Em Wed, Aug 24, 2022 at 08:38:58AM -0700, Ian Rogers escreveu:
> > > Add thread safety annotations to struct mutex so that when compiled with
> > > clang's -Wthread-safety warnings are generated for erroneous lock
> > > patterns. NO_THREAD_SAFETY_ANALYSIS is needed for
> > > mutex_lock/mutex_unlock as the analysis doesn't under pthread calls.
> >
> > So even having the guards checking if the attribute is available it
> > seems at least clang 11.0 is needed, as the "lockable" arg was
> > introduced there:
> >
> > 37 42.61 fedora:32 : FAIL clang version 10.0.1 (Fedora 10.0.1-3.fc32)
> > In file included from /git/perf-6.0.0-rc2/tools/perf/util/../ui/ui.h:5:
> > /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:74:8: error: invalid capability name 'lockable'; capability name must be 'mutex' or 'role' [-Werror,-Wthread-safety-attributes]
> > struct LOCKABLE mutex {
> > ^
> > /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:35:44: note: expanded from macro 'LOCKABLE'
> > #define LOCKABLE __attribute__((capability("lockable")))
>
>
> capability("lockable") can just be lockable, the capability
> generalization came in a later clang release.
>
> That is change:
> #define LOCKABLE __attribute__((capability("lockable")))
> to:
> #define LOCKABLE __attribute__((lockable))
>
> and later clangs take the earlier name. Do you want a v5 for this 1 liner?
I did it and I'm now testing, thanks.
- Arnaldo
diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
index 48a2d87598f0d725..29b5494b213a3fc9 100644
--- a/tools/perf/util/mutex.h
+++ b/tools/perf/util/mutex.h
@@ -32,7 +32,7 @@
#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
/* Documents if a type is a lockable type. */
-#define LOCKABLE __attribute__((capability("lockable")))
+#define LOCKABLE __attribute__((lockable))
/* Documents functions that acquire a lock in the body of a function, and do not release it. */
#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
- Arnaldo
On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <[email protected]> wrote:
> Below seems adequate for now, at least logically, but maybe it
> would confuse clang thread-safety analysis?
I think it's not just about locks, the exit_browser should bail out early
if the setup code was not called.
Thanks,
Namhyung
>
> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> index 25ded88801a3..6d81be6a349e 100644
> --- a/tools/perf/ui/setup.c
> +++ b/tools/perf/ui/setup.c
> @@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> return 0;
> }
>
> +/*
> + * exit_browser() can get called without setup_browser() having been called
> + * first, so it is necessary to keep track of whether ui__lock has been
> + * initialized.
> + */
> +static bool ui__lock_initialized;
> +
> void setup_browser(bool fallback_to_pager)
> {
> mutex_init(&ui__lock);
> + ui__lock_initialized = true;
> if (use_browser < 2 && (!isatty(1) || dump_trace))
> use_browser = 0;
>
> @@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
> default:
> break;
> }
> - mutex_destroy(&ui__lock);
> + if (ui__lock_initialized)
> + mutex_destroy(&ui__lock);
> }
>
On 26/08/22 20:45, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 10:22 AM Adrian Hunter <[email protected]> wrote:
>>
>> On 26/08/22 19:02, Ian Rogers wrote:
>>> On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <[email protected]> wrote:
>>>>
>>>> On 24/08/22 18:38, Ian Rogers wrote:
>>>>> Switch to the use of mutex wrappers that provide better error checking.
>>>>>
>>>>> Signed-off-by: Ian Rogers <[email protected]>
>>>>> ---
>>>>> tools/perf/ui/browser.c | 20 ++++++++++----------
>>>>> tools/perf/ui/browsers/annotate.c | 2 +-
>>>>> tools/perf/ui/setup.c | 5 +++--
>>>>> tools/perf/ui/tui/helpline.c | 5 ++---
>>>>> tools/perf/ui/tui/progress.c | 8 ++++----
>>>>> tools/perf/ui/tui/setup.c | 8 ++++----
>>>>> tools/perf/ui/tui/util.c | 18 +++++++++---------
>>>>> tools/perf/ui/ui.h | 4 ++--
>>>>> 8 files changed, 35 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>>>>> index fa5bd5c20e96..78fb01d6ad63 100644
>>>>> --- a/tools/perf/ui/browser.c
>>>>> +++ b/tools/perf/ui/browser.c
>>>>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>>>
>>>>> void ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>>> {
>>>>> - pthread_mutex_lock(&ui__lock);
>>>>> + mutex_lock(&ui__lock);
>>>>> __ui_browser__show_title(browser, title);
>>>>> - pthread_mutex_unlock(&ui__lock);
>>>>> + mutex_unlock(&ui__lock);
>>>>> }
>>>>>
>>>>> int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>>
>>>>> browser->refresh_dimensions(browser);
>>>>>
>>>>> - pthread_mutex_lock(&ui__lock);
>>>>> + mutex_lock(&ui__lock);
>>>>> __ui_browser__show_title(browser, title);
>>>>>
>>>>> browser->title = title;
>>>>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>> va_end(ap);
>>>>> if (err > 0)
>>>>> ui_helpline__push(browser->helpline);
>>>>> - pthread_mutex_unlock(&ui__lock);
>>>>> + mutex_unlock(&ui__lock);
>>>>> return err ? 0 : -1;
>>>>> }
>>>>>
>>>>> void ui_browser__hide(struct ui_browser *browser)
>>>>> {
>>>>> - pthread_mutex_lock(&ui__lock);
>>>>> + mutex_lock(&ui__lock);
>>>>> ui_helpline__pop();
>>>>> zfree(&browser->helpline);
>>>>> - pthread_mutex_unlock(&ui__lock);
>>>>> + mutex_unlock(&ui__lock);
>>>>> }
>>>>>
>>>>> static void ui_browser__scrollbar_set(struct ui_browser *browser)
>>>>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>>>>>
>>>>> int ui_browser__refresh(struct ui_browser *browser)
>>>>> {
>>>>> - pthread_mutex_lock(&ui__lock);
>>>>> + mutex_lock(&ui__lock);
>>>>> __ui_browser__refresh(browser);
>>>>> - pthread_mutex_unlock(&ui__lock);
>>>>> + mutex_unlock(&ui__lock);
>>>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>>>>> while (1) {
>>>>> off_t offset;
>>>>>
>>>>> - pthread_mutex_lock(&ui__lock);
>>>>> + mutex_lock(&ui__lock);
>>>>> err = __ui_browser__refresh(browser);
>>>>> SLsmg_refresh();
>>>>> - pthread_mutex_unlock(&ui__lock);
>>>>> + mutex_unlock(&ui__lock);
>>>>> if (err < 0)
>>>>> break;
>>>>>
>>>>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>>>>> index 44ba900828f6..b8747e8dd9ea 100644
>>>>> --- a/tools/perf/ui/browsers/annotate.c
>>>>> +++ b/tools/perf/ui/browsers/annotate.c
>>>>> @@ -8,11 +8,11 @@
>>>>> #include "../../util/hist.h"
>>>>> #include "../../util/sort.h"
>>>>> #include "../../util/map.h"
>>>>> +#include "../../util/mutex.h"
>>>>> #include "../../util/symbol.h"
>>>>> #include "../../util/evsel.h"
>>>>> #include "../../util/evlist.h"
>>>>> #include <inttypes.h>
>>>>> -#include <pthread.h>
>>>>> #include <linux/kernel.h>
>>>>> #include <linux/string.h>
>>>>> #include <linux/zalloc.h>
>>>>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>>>>> index 700335cde618..25ded88801a3 100644
>>>>> --- a/tools/perf/ui/setup.c
>>>>> +++ b/tools/perf/ui/setup.c
>>>>> @@ -1,5 +1,4 @@
>>>>> // SPDX-License-Identifier: GPL-2.0
>>>>> -#include <pthread.h>
>>>>> #include <dlfcn.h>
>>>>> #include <unistd.h>
>>>>>
>>>>> @@ -8,7 +7,7 @@
>>>>> #include "../util/hist.h"
>>>>> #include "ui.h"
>>>>>
>>>>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
>>>>> +struct mutex ui__lock;
>>>>> void *perf_gtk_handle;
>>>>> int use_browser = -1;
>>>>>
>>>>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>>>>>
>>>>> void setup_browser(bool fallback_to_pager)
>>>>> {
>>>>> + mutex_init(&ui__lock);
>>>>> if (use_browser < 2 && (!isatty(1) || dump_trace))
>>>>> use_browser = 0;
>>>>>
>>>>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>>>>> default:
>>>>> break;
>>>>> }
>>>>> + mutex_destroy(&ui__lock);
>>>>
>>>> Looks like exit_browser() can be called even when setup_browser()
>>>> was never called.
>>>>
>>>> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
>>>> pthread won't notice.
>>>
>>> Memory sanitizer will notice some cases of this and so I didn't want
>>> to code defensively around exit being called ahead of setup.
>>
>> I am not sure you understood.
>>
>> As I wrote, exit_browser() can be called even when setup_browser()
>> was never called, so it is not defensive programming, it is necessary
>> programming that you only get away without doing because
>> PTHREAD_MUTEX_INITIALIZER is all zeros.
>
> Why are we here:
> 1) there is a memory leak
> 2) I fix the memory and trigger a use after free
> 3) I invent a reference count checker, use it to fix the memory leak,
> use after free and missing locks - the patch for this in 10s of lines
> long
> 4) when adding the lock fixes I defensively add error checking to the
> mutex involved - mainly because I was scared I could introduce a
> deadlock
> 5) I get asked to generalize this
> 6) GSoC contributor picks up and puts this down
> 7) I pull together the contributor's work
> 8) I get asked to turn a search and replace 4 patch fix into an unwieldy patch
> 9) I worry about the sanity of the change and add lock checking from clang
> 10) I end up trying to fix perf-sched who for some reason thought it
> was perfectly valid to have threads blocked on mutexes that were
> deallocated on the stack.
> 11) the UI code was written with a view that exiting something not
> setup somehow made sense
>
> I am drawing a line at fixing perf sched and the UI code. We can drop
> this patch and keep things as a pthread_mutex_t, similarly for
> perf-sched. I have gone about as far as I'm prepared to for the sake
> of a 10s of line memory leak fix. Some private thoughts are, it would
> be useful if review comments could be constructive, hey do this not
> that, and not simply commenting on change or trying to shoehorn vast
> amounts of tech debt clean up onto simple fixes.
If you want help, you only need ask.
Below seems adequate for now, at least logically, but maybe it
would confuse clang thread-safety analysis?
diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
index 25ded88801a3..6d81be6a349e 100644
--- a/tools/perf/ui/setup.c
+++ b/tools/perf/ui/setup.c
@@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
return 0;
}
+/*
+ * exit_browser() can get called without setup_browser() having been called
+ * first, so it is necessary to keep track of whether ui__lock has been
+ * initialized.
+ */
+static bool ui__lock_initialized;
+
void setup_browser(bool fallback_to_pager)
{
mutex_init(&ui__lock);
+ ui__lock_initialized = true;
if (use_browser < 2 && (!isatty(1) || dump_trace))
use_browser = 0;
@@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
default:
break;
}
- mutex_destroy(&ui__lock);
+ if (ui__lock_initialized)
+ mutex_destroy(&ui__lock);
}
On 26/08/22 21:26, Namhyung Kim wrote:
> On Fri, Aug 26, 2022 at 10:48 AM Ian Rogers <[email protected]> wrote:
>>
>> On Fri, Aug 26, 2022 at 10:41 AM Adrian Hunter <[email protected]> wrote:
>>>
>>> On 26/08/22 19:06, Ian Rogers wrote:
>>>> On Fri, Aug 26, 2022 at 5:12 AM Adrian Hunter <[email protected]> wrote:
>>>>>
>>>>> On 24/08/22 18:38, Ian Rogers wrote:
>>>>>> Add annotations to describe lock behavior. Add unlocks so that mutexes
>>>>>> aren't conditionally held on exit from perf_sched__replay. Add an exit
>>>>>> variable so that thread_func can terminate, rather than leaving the
>>>>>> threads blocked on mutexes.
>>>>>>
>>>>>> Signed-off-by: Ian Rogers <[email protected]>
>>>>>> ---
>>>>>> tools/perf/builtin-sched.c | 46 ++++++++++++++++++++++++--------------
>>>>>> 1 file changed, 29 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>>>>>> index 7e4006d6b8bc..b483ff0d432e 100644
>>>>>> --- a/tools/perf/builtin-sched.c
>>>>>> +++ b/tools/perf/builtin-sched.c
>>>>>> @@ -246,6 +246,7 @@ struct perf_sched {
>>>>>> const char *time_str;
>>>>>> struct perf_time_interval ptime;
>>>>>> struct perf_time_interval hist_time;
>>>>>> + volatile bool thread_funcs_exit;
>>>>>> };
>>>>>>
>>>>>> /* per thread run time data */
>>>>>> @@ -633,31 +634,34 @@ static void *thread_func(void *ctx)
>>>>>> prctl(PR_SET_NAME, comm2);
>>>>>> if (fd < 0)
>>>>>> return NULL;
>>>>>> -again:
>>>>>> - ret = sem_post(&this_task->ready_for_work);
>>>>>> - BUG_ON(ret);
>>>>>> - mutex_lock(&sched->start_work_mutex);
>>>>>> - mutex_unlock(&sched->start_work_mutex);
>>>>>>
>>>>>> - cpu_usage_0 = get_cpu_usage_nsec_self(fd);
>>>>>> + while (!sched->thread_funcs_exit) {
>>>>>> + ret = sem_post(&this_task->ready_for_work);
>>>>>> + BUG_ON(ret);
>>>>>> + mutex_lock(&sched->start_work_mutex);
>>>>>> + mutex_unlock(&sched->start_work_mutex);
>>>>>>
>>>>>> - for (i = 0; i < this_task->nr_events; i++) {
>>>>>> - this_task->curr_event = i;
>>>>>> - perf_sched__process_event(sched, this_task->atoms[i]);
>>>>>> - }
>>>>>> + cpu_usage_0 = get_cpu_usage_nsec_self(fd);
>>>>>>
>>>>>> - cpu_usage_1 = get_cpu_usage_nsec_self(fd);
>>>>>> - this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
>>>>>> - ret = sem_post(&this_task->work_done_sem);
>>>>>> - BUG_ON(ret);
>>>>>> + for (i = 0; i < this_task->nr_events; i++) {
>>>>>> + this_task->curr_event = i;
>>>>>> + perf_sched__process_event(sched, this_task->atoms[i]);
>>>>>> + }
>>>>>>
>>>>>> - mutex_lock(&sched->work_done_wait_mutex);
>>>>>> - mutex_unlock(&sched->work_done_wait_mutex);
>>>>>> + cpu_usage_1 = get_cpu_usage_nsec_self(fd);
>>>>>> + this_task->cpu_usage = cpu_usage_1 - cpu_usage_0;
>>>>>> + ret = sem_post(&this_task->work_done_sem);
>>>>>> + BUG_ON(ret);
>>>>>>
>>>>>> - goto again;
>>>>>> + mutex_lock(&sched->work_done_wait_mutex);
>>>>>> + mutex_unlock(&sched->work_done_wait_mutex);
>>>>>> + }
>>>>>> + return NULL;
>>>>>> }
>>>>>>
>>>>>> static void create_tasks(struct perf_sched *sched)
>>>>>> + EXCLUSIVE_LOCK_FUNCTION(sched->start_work_mutex)
>>>>>> + EXCLUSIVE_LOCK_FUNCTION(sched->work_done_wait_mutex)
>>>>>> {
>>>>>> struct task_desc *task;
>>>>>> pthread_attr_t attr;
>>>>>> @@ -687,6 +691,8 @@ static void create_tasks(struct perf_sched *sched)
>>>>>> }
>>>>>>
>>>>>> static void wait_for_tasks(struct perf_sched *sched)
>>>>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
>>>>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
>>>>>> {
>>>>>> u64 cpu_usage_0, cpu_usage_1;
>>>>>> struct task_desc *task;
>>>>>> @@ -738,6 +744,8 @@ static void wait_for_tasks(struct perf_sched *sched)
>>>>>> }
>>>>>>
>>>>>> static void run_one_test(struct perf_sched *sched)
>>>>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->work_done_wait_mutex)
>>>>>> + EXCLUSIVE_LOCKS_REQUIRED(sched->start_work_mutex)
>>>>>> {
>>>>>> u64 T0, T1, delta, avg_delta, fluct;
>>>>>>
>>>>>> @@ -3309,11 +3317,15 @@ static int perf_sched__replay(struct perf_sched *sched)
>>>>>> print_task_traces(sched);
>>>>>> add_cross_task_wakeups(sched);
>>>>>>
>>>>>> + sched->thread_funcs_exit = false;
>>>>>> create_tasks(sched);
>>>>>> printf("------------------------------------------------------------\n");
>>>>>> for (i = 0; i < sched->replay_repeat; i++)
>>>>>> run_one_test(sched);
>>>>>>
>>>>>> + sched->thread_funcs_exit = true;
>>>>>> + mutex_unlock(&sched->start_work_mutex);
>>>>>> + mutex_unlock(&sched->work_done_wait_mutex);
>>>>>
>>>>> I think you still need to wait for the threads to exit before
>>>>> destroying the mutexes.
>>>>
>>>> This is a pre-existing issue and beyond the scope of this patch set.
>>>
>>> You added the mutex_destroy functions in patch 8, so it is still
>>> fallout from that.
>>
>> In the previous code the threads were blocked on mutexes that were
>> stack allocated and the stack memory went away. You are correct to say
>> that to those locks I added an init and destroy call. The lifetime of
>> the mutex was wrong previously and remains wrong in this change.
>
> I think you fixed the lifetime issue with sched->thread_funcs_exit here.
> All you need to do is calling pthread_join() after the mutex_unlock, no?
Like this maybe:
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index b483ff0d432e..8090c1218855 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3326,6 +3326,13 @@ static int perf_sched__replay(struct perf_sched *sched)
sched->thread_funcs_exit = true;
mutex_unlock(&sched->start_work_mutex);
mutex_unlock(&sched->work_done_wait_mutex);
+ /* Get rid of threads so they won't be upset by mutex destruction */
+ for (i = 0; i < sched->nr_tasks; i++) {
+ int err = pthread_join(sched->tasks[i]->thread, NULL);
+
+ if (err)
+ pr_err("pthread_join() failed for task nr %lu, error %d\n", i, err);
+ }
return 0;
}
On 26/08/22 22:00, Namhyung Kim wrote:
> On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <[email protected]> wrote:
>> Below seems adequate for now, at least logically, but maybe it
>> would confuse clang thread-safety analysis?
>
> I think it's not just about locks, the exit_browser should bail out early
> if the setup code was not called.
In those cases, use_browser is 0 or -1 unless someone has inserted
an invalid perf config like "tui.script=on" or "gtk.script=on".
So currently, in cases where exit_browser() is called without
setup_browser(), it does nothing. Which means it is only the
unconditional mutex_destroy() that needs to be conditional.
>
> Thanks,
> Namhyung
>
>
>>
>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>> index 25ded88801a3..6d81be6a349e 100644
>> --- a/tools/perf/ui/setup.c
>> +++ b/tools/perf/ui/setup.c
>> @@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>> return 0;
>> }
>>
>> +/*
>> + * exit_browser() can get called without setup_browser() having been called
>> + * first, so it is necessary to keep track of whether ui__lock has been
>> + * initialized.
>> + */
>> +static bool ui__lock_initialized;
>> +
>> void setup_browser(bool fallback_to_pager)
>> {
>> mutex_init(&ui__lock);
>> + ui__lock_initialized = true;
>> if (use_browser < 2 && (!isatty(1) || dump_trace))
>> use_browser = 0;
>>
>> @@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
>> default:
>> break;
>> }
>> - mutex_destroy(&ui__lock);
>> + if (ui__lock_initialized)
>> + mutex_destroy(&ui__lock);
>> }
>>
On Fri, Aug 26, 2022 at 12:21 PM Adrian Hunter <[email protected]> wrote:
>
> On 26/08/22 22:00, Namhyung Kim wrote:
> > On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <[email protected]> wrote:
> >> Below seems adequate for now, at least logically, but maybe it
> >> would confuse clang thread-safety analysis?
> >
> > I think it's not just about locks, the exit_browser should bail out early
> > if the setup code was not called.
>
> In those cases, use_browser is 0 or -1 unless someone has inserted
> an invalid perf config like "tui.script=on" or "gtk.script=on".
> So currently, in cases where exit_browser() is called without
> setup_browser(), it does nothing. Which means it is only the
> unconditional mutex_destroy() that needs to be conditional.
Yeah there's a possibility that it can be called with > 0 use_browser
on some broken config or something. So I think it's safer and better
for future changes.
Thanks,
Namhyung
On Fri, Aug 26, 2022 at 1:40 PM Namhyung Kim <[email protected]> wrote:
>
> On Fri, Aug 26, 2022 at 12:21 PM Adrian Hunter <[email protected]> wrote:
> >
> > On 26/08/22 22:00, Namhyung Kim wrote:
> > > On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <[email protected]> wrote:
> > >> Below seems adequate for now, at least logically, but maybe it
> > >> would confuse clang thread-safety analysis?
> > >
> > > I think it's not just about locks, the exit_browser should bail out early
> > > if the setup code was not called.
> >
> > In those cases, use_browser is 0 or -1 unless someone has inserted
> > an invalid perf config like "tui.script=on" or "gtk.script=on".
> > So currently, in cases where exit_browser() is called without
> > setup_browser(), it does nothing. Which means it is only the
> > unconditional mutex_destroy() that needs to be conditional.
>
> Yeah there's a possibility that it can be called with > 0 use_browser
> on some broken config or something. So I think it's safer and better
> for future changes.
I'd thought about a:
static bool ui__lock_initialized;
but the issue is shouldn't it be atomic? Maybe we should guard it with
a lock? Then we are back where we started. Having a clean init/exit
invariant would be best but such a change has the potential to be
large and out of scope here.
Thanks,
Ian
> Thanks,
> Namhyung
On Fri, Aug 26, 2022 at 12:36 PM Adrian Hunter <[email protected]> wrote:
>
> On 26/08/22 21:26, Namhyung Kim wrote:
> > On Fri, Aug 26, 2022 at 10:48 AM Ian Rogers <[email protected]> wrote:
> >> In the previous code the threads were blocked on mutexes that were
> >> stack allocated and the stack memory went away. You are correct to say
> >> that to those locks I added an init and destroy call. The lifetime of
> >> the mutex was wrong previously and remains wrong in this change.
> >
> > I think you fixed the lifetime issue with sched->thread_funcs_exit here.
> > All you need to do is calling pthread_join() after the mutex_unlock, no?
>
> Like this maybe:
Yeah, but at this point we might want to factor it out as a function like
destroy_tasks().
Thanks,
Namhyung
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index b483ff0d432e..8090c1218855 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3326,6 +3326,13 @@ static int perf_sched__replay(struct perf_sched *sched)
> sched->thread_funcs_exit = true;
> mutex_unlock(&sched->start_work_mutex);
> mutex_unlock(&sched->work_done_wait_mutex);
> + /* Get rid of threads so they won't be upset by mutex destruction */
> + for (i = 0; i < sched->nr_tasks; i++) {
> + int err = pthread_join(sched->tasks[i]->thread, NULL);
> +
> + if (err)
> + pr_err("pthread_join() failed for task nr %lu, error %d\n", i, err);
> + }
> return 0;
> }
On 26/08/22 23:52, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 1:40 PM Namhyung Kim <[email protected]> wrote:
>>
>> On Fri, Aug 26, 2022 at 12:21 PM Adrian Hunter <[email protected]> wrote:
>>>
>>> On 26/08/22 22:00, Namhyung Kim wrote:
>>>> On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <[email protected]> wrote:
>>>>> Below seems adequate for now, at least logically, but maybe it
>>>>> would confuse clang thread-safety analysis?
>>>>
>>>> I think it's not just about locks, the exit_browser should bail out early
>>>> if the setup code was not called.
>>>
>>> In those cases, use_browser is 0 or -1 unless someone has inserted
>>> an invalid perf config like "tui.script=on" or "gtk.script=on".
>>> So currently, in cases where exit_browser() is called without
>>> setup_browser(), it does nothing. Which means it is only the
>>> unconditional mutex_destroy() that needs to be conditional.
>>
>> Yeah there's a possibility that it can be called with > 0 use_browser
>> on some broken config or something. So I think it's safer and better
>> for future changes.
>
> I'd thought about a:
> static bool ui__lock_initialized;
> but the issue is shouldn't it be atomic?
No, it is only accessed from the main thread.