[PATCH v1 1/4] perf mutex: Wrapped usage of pthread_mutex_t

[PATCH v1 1/4] perf mutex: Wrapped usage of pthread_mutex_t

[gpavithrasha]

From: pavithra <gpavithrasha@gmail.com>

Added a new header file mutex.h that wraps the
usage of pthread_mutex_t and updated lock in dso.h.

Signed-off-by: pavithra <gpavithrasha@gmail.com>
---
 tools/perf/util/Build    |  1 +
 tools/perf/util/dso.c    | 33 ++++++++++++++++-----------------
 tools/perf/util/dso.h    |  3 ++-
 tools/perf/util/mutex.c  | 32 ++++++++++++++++++++++++++++++++
 tools/perf/util/mutex.h  | 15 +++++++++++++++
 tools/perf/util/symbol.c |  4 ++--
 6 files changed, 68 insertions(+), 20 deletions(-)
 create mode 100644 tools/perf/util/mutex.c
 create mode 100644 tools/perf/util/mutex.h

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 8dcfca1a882f..559c20d94c36 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -120,6 +120,7 @@ perf-y += time-utils.o
 perf-y += expr-bison.o
 perf-y += branch.o
 perf-y += mem2node.o
+perf-y += mutex.o
 
 perf-$(CONFIG_LIBBPF) += bpf-loader.o
 perf-$(CONFIG_LIBBPF) += bpf_map.o
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index e11ddf86f2b3..605c31585d48 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -426,7 +426,7 @@ void dso__set_module_info(struct dso *dso, struct kmod_path *m,
  */
 static LIST_HEAD(dso__data_open);
 static long dso__data_open_cnt;
-static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
+static struct mutex dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
 
 static void dso__list_add(struct dso *dso)
 {
@@ -633,9 +633,9 @@ static void check_data_close(void)
  */
 void dso__data_close(struct dso *dso)
 {
-	pthread_mutex_lock(&dso__data_open_lock);
+	mutex_lock(&dso__data_open_lock);
 	close_dso(dso);
-	pthread_mutex_unlock(&dso__data_open_lock);
+	mutex_unlock(&dso__data_open_lock);
 }
 
 static void try_to_open_dso(struct dso *dso, struct machine *machine)
@@ -684,20 +684,19 @@ int dso__data_get_fd(struct dso *dso, struct machine *machine)
 	if (dso->data.status == DSO_DATA_STATUS_ERROR)
 		return -1;
 
-	if (pthread_mutex_lock(&dso__data_open_lock) < 0)
-		return -1;
+	mutex_lock(&dso__data_open_lock);
 
 	try_to_open_dso(dso, machine);
 
 	if (dso->data.fd < 0)
-		pthread_mutex_unlock(&dso__data_open_lock);
+		mutex_unlock(&dso__data_open_lock);
 
 	return dso->data.fd;
 }
 
 void dso__data_put_fd(struct dso *dso __maybe_unused)
 {
-	pthread_mutex_unlock(&dso__data_open_lock);
+	mutex_unlock(&dso__data_open_lock);
 }
 
 bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
@@ -756,7 +755,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;
 
@@ -765,7 +764,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)
@@ -802,7 +801,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;
 
@@ -823,7 +822,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;
 }
 
@@ -843,7 +842,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
 {
 	ssize_t ret;
 
-	pthread_mutex_lock(&dso__data_open_lock);
+	mutex_lock(&dso__data_open_lock);
 
 	/*
 	 * dso->data.fd might be closed if other thread opened another
@@ -859,7 +858,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
 
 	ret = pread(dso->data.fd, data, DSO__DATA_CACHE_SIZE, offset);
 out:
-	pthread_mutex_unlock(&dso__data_open_lock);
+	mutex_unlock(&dso__data_open_lock);
 	return ret;
 }
 
@@ -953,7 +952,7 @@ static int file_size(struct dso *dso, struct machine *machine)
 	struct stat st;
 	char sbuf[STRERR_BUFSIZE];
 
-	pthread_mutex_lock(&dso__data_open_lock);
+	mutex_lock(&dso__data_open_lock);
 
 	/*
 	 * dso->data.fd might be closed if other thread opened another
@@ -977,7 +976,7 @@ static int file_size(struct dso *dso, struct machine *machine)
 	dso->data.file_size = st.st_size;
 
 out:
-	pthread_mutex_unlock(&dso__data_open_lock);
+	mutex_unlock(&dso__data_open_lock);
 	return ret;
 }
 
@@ -1192,7 +1191,7 @@ struct dso *dso__new(const char *name)
 		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);
 	}
 
@@ -1226,7 +1225,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 e4dddb76770d..e08b2ab48314 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -11,6 +11,7 @@
 #include <stdio.h>
 #include <linux/bitops.h>
 #include "build-id.h"
+#include "mutex.h"
 
 struct machine;
 struct map;
@@ -132,7 +133,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/mutex.c b/tools/perf/util/mutex.c
new file mode 100644
index 000000000000..b7264a1438c4
--- /dev/null
+++ b/tools/perf/util/mutex.c
@@ -0,0 +1,32 @@
+#include <mutex.h>
+#include <pthread.h>
+
+//to avoid the warning : implicit declaration of BUG_ON,
+//we add the following 2 headers.
+#include <linux/compiler.h>
+#include <linux/kernel.h>
+
+void mutex_init(struct mutex *mtx)
+{
+pthread_mutexattr_t lock_attr;
+pthread_mutexattr_init(&lock_attr);
+pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
+BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
+//on success, returns 0.
+pthread_mutexattr_destroy(&lock_attr);
+}
+
+void mutex_destroy(struct mutex *mtx)
+{
+BUG_ON(pthread_mutex_destroy(&mtx->lock));     //on success, returns 0.
+}
+
+void mutex_lock(struct mutex *mtx)
+{
+BUG_ON(pthread_mutex_lock(&mtx->lock) != 0);
+}
+
+void mutex_unlock(struct mutex *mtx)
+{
+BUG_ON(pthread_mutex_unlock(&mtx->lock) != 0);
+}
diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
new file mode 100644
index 000000000000..ab2ebb98b24a
--- /dev/null
+++ b/tools/perf/util/mutex.h
@@ -0,0 +1,15 @@
+#ifndef __PERF_MUTEX_H
+#define _PERF_MUTEX_H
+
+#include <pthread.h>
+
+struct mutex {
+pthread_mutex_t lock;
+};
+
+void mutex_lock(struct mutex *mtx);
+void mutex_unlock(struct mutex *mtx);
+void mutex_init(struct mutex *mtx);
+void mutex_destroy(struct mutex *mtx);
+
+#endif /* _PERF_MUTEX_H */
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a8f80e427674..342be12cfa1e 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1629,7 +1629,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)) {
@@ -1778,7 +1778,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.25.1

[PATCH v1 2/4] perf mutex with nsinfo: Updated pthread_mutex_t usage

[gpavithrasha]

From: pavithra <gpavithrasha@gmail.com>

Updated usage of pthread_mutex_t with nsinfo
with the new wrapped lock(struct) in mutex.h
(remove data races).

Signed-off-by: pavithra <gpavithrasha@gmail.com>
---
 tools/perf/builtin-inject.c   | 6 +++---
 tools/perf/util/map.c         | 2 ++
 tools/perf/util/probe-event.c | 6 +++++-
 tools/perf/util/symbol.c      | 2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 372ecb3e2c06..81eaed8da207 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -388,10 +388,10 @@ static int perf_event__repipe_id_index(struct perf_session *session,
 static int dso__read_build_id(struct dso *dso)
 {
 	if (dso->has_build_id)
-		return 0;
-
+		return 0;	
+		
 	if (filename__read_build_id(dso->long_name, dso->build_id,
-				    sizeof(dso->build_id)) > 0) {
+	                           sizeof(dso->build_id)) > 0) {
 		dso->has_build_id = true;
 		return 0;
 	}
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 5b83ed1ebbd6..2ef5fe0cc53c 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -214,8 +214,10 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
 			if (!(prot & PROT_EXEC))
 				dso__set_loaded(dso);
 		}
+		mutex_lock(&dso->lock);
 		dso->nsinfo = nsi;
 		dso__put(dso);
+		mutex_unlock(&dso->lock);
 	}
 	return map;
 out_delete:
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 91cab5f669d2..e527f2612ba4 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -38,6 +38,7 @@
 #include "session.h"
 #include "string2.h"
 #include "strbuf.h"
+#include "mutex.h"
 
 #include <subcmd/pager.h>
 #include <linux/ctype.h>
@@ -171,8 +172,11 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
 		struct map *map;
 
 		map = dso__new_map(target);
-		if (map && map->dso)
+		if (map && map->dso) {
+			mutex_lock(&map->dso->lock);
 			map->dso->nsinfo = nsinfo__get(nsi);
+			mutex_unlock(&map->dso->lock);
+		}	
 		return map;
 	} else {
 		return kernel_get_module_map(target);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 342be12cfa1e..4b711b13f915 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1619,7 +1619,7 @@ int dso__load(struct dso *dso, struct map *map)
 	struct nscookie nsc;
 	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,
-- 
2.25.1

[PATCH v1 3/4] perf mutex and cond: Updated files mutex.h & mutex.c

[gpavithrasha]

From: pavithra <gpavithrasha@gmail.com>

Added new struct and corresponding
functions to wrap usage of pthread_cond_t.
Added a new function for mutex_trylock-similar to
mutex_lock.

Signed-off-by: pavithra <gpavithrasha@gmail.com>
---
 tools/perf/util/mutex.c | 37 ++++++++++++++++++++++++++++++++-----
 tools/perf/util/mutex.h | 13 +++++++++++--
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
index b7264a1438c4..9dc37a3f374f 100644
--- a/tools/perf/util/mutex.c
+++ b/tools/perf/util/mutex.c
@@ -1,8 +1,8 @@
 #include <mutex.h>
 #include <pthread.h>
 
-//to avoid the warning : implicit declaration of BUG_ON,
-//we add the following 2 headers.
+/*to avoid the warning : implicit declaration of BUG_ON*/
+/*we add the following 2 headers*/
 #include <linux/compiler.h>
 #include <linux/kernel.h>
 
@@ -11,14 +11,15 @@ void mutex_init(struct mutex *mtx)
 pthread_mutexattr_t lock_attr;
 pthread_mutexattr_init(&lock_attr);
 pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
-BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
-//on success, returns 0.
+/*pthread_mutex_init:on success, returns 0*/
+BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));  
 pthread_mutexattr_destroy(&lock_attr);
 }
 
 void mutex_destroy(struct mutex *mtx)
 {
-BUG_ON(pthread_mutex_destroy(&mtx->lock));     //on success, returns 0.
+/*pthread_mutex_destroy:on success, returns 0*/
+BUG_ON(pthread_mutex_destroy(&mtx->lock));     
 }
 
 void mutex_lock(struct mutex *mtx)
@@ -30,3 +31,29 @@ void mutex_unlock(struct mutex *mtx)
 {
 BUG_ON(pthread_mutex_unlock(&mtx->lock) != 0);
 }
+
+bool mutex_trylock(struct mutex *mtx)
+{
+return pthread_mutex_trylock(&mtx->lock)!=0;
+}
+
+void cond_wait(struct cond *cnd, struct mutex *mtx)
+{
+BUG_ON(pthread_cond_wait(&cnd->cond, &mtx->lock) != 0);
+}
+
+void cond_signal(struct cond *cnd)
+{
+BUG_ON(pthread_cond_signal(&cnd->cond) != 0);
+}
+
+void cond_init(struct cond *cnd)
+{
+pthread_condattr_t attr;
+
+pthread_condattr_init(&attr);
+
+/*pthread_cond_init:on success, returns 0*/
+BUG_ON(pthread_cond_init(&cnd->cond, &attr));
+pthread_condattr_destroy(&attr);
+}
diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
index ab2ebb98b24a..f1b4aaa151be 100644
--- a/tools/perf/util/mutex.h
+++ b/tools/perf/util/mutex.h
@@ -1,15 +1,24 @@
 #ifndef __PERF_MUTEX_H
-#define _PERF_MUTEX_H
+#define __PERF_MUTEX_H
 
 #include <pthread.h>
+#include <stdbool.h>
 
 struct mutex {
 pthread_mutex_t lock;
 };
 
+struct cond {
+pthread_cond_t cond;
+};
+
 void mutex_lock(struct mutex *mtx);
 void mutex_unlock(struct mutex *mtx);
+bool mutex_trylock(struct mutex *mtx);
 void mutex_init(struct mutex *mtx);
 void mutex_destroy(struct mutex *mtx);
 
-#endif /* _PERF_MUTEX_H */
+void cond_wait(struct cond *cnd, struct mutex *mtx);
+void cond_signal(struct cond *cnd);
+void cond_init(struct cond *cnd);
+#endif /* __PERF_MUTEX_H */
-- 
2.25.1

[PATCH v1 4/4] perf mutex and cond: Updated every occurrence of pthread_mutex and pthread_cond

[gpavithrasha]

From: pavithra <gpavithrasha@gmail.com>

Structs using pthread_mutex_t and
pthread_cond_t were updated and all the
usage of them were replaced with the new
wrapped functions as per the declaration &
definition in mutex.h and mutex.c files. Header
files included in each file were modified so as
to avoid redefinition errors.

Signed-off-by: pavithra <gpavithrasha@gmail.com>
---
 tools/perf/builtin-inject.c       |  1 -
 tools/perf/builtin-top.c          | 42 +++++++++++++++----------------
 tools/perf/ui/browsers/annotate.c |  4 +--
 tools/perf/util/annotate.c        |  8 +++---
 tools/perf/util/annotate.h        |  3 ++-
 tools/perf/util/hist.c            |  6 ++---
 tools/perf/util/hist.h            |  3 ++-
 tools/perf/util/symbol.c          |  2 +-
 tools/perf/util/top.h             |  5 ++--
 9 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 81eaed8da207..570472c597c3 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -395,7 +395,6 @@ static int dso__read_build_id(struct dso *dso)
 		dso->has_build_id = true;
 		return 0;
 	}
-
 	return -1;
 }
 
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 1f60124eb19b..e3ca3ba8fcfb 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -132,10 +132,10 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 	}
 
 	notes = symbol__annotation(sym);
-	pthread_mutex_lock(&notes->lock);
+	mutex_lock(&notes->lock);
 
 	if (!symbol__hists(sym, top->evlist->core.nr_entries)) {
-		pthread_mutex_unlock(&notes->lock);
+		mutex_unlock(&notes->lock);
 		pr_err("Not enough memory for annotating '%s' symbol!\n",
 		       sym->name);
 		sleep(1);
@@ -151,7 +151,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(&notes->lock);
+	mutex_unlock(&notes->lock);
 	return err;
 }
 
@@ -204,19 +204,19 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 
 	notes = symbol__annotation(sym);
 
-	if (pthread_mutex_trylock(&notes->lock))
-		return;
+	if(mutex_trylock(&notes->lock))
+	return;
 
 	err = hist_entry__inc_addr_samples(he, sample, evsel, ip);
 
-	pthread_mutex_unlock(&notes->lock);
+	mutex_unlock(&notes->lock);
 
 	if (unlikely(err)) {
 		/*
 		 * 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);
@@ -226,7 +226,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);
 	}
 }
 
@@ -246,7 +246,7 @@ static void perf_top__show_details(struct perf_top *top)
 	symbol = he->ms.sym;
 	notes = symbol__annotation(symbol);
 
-	pthread_mutex_lock(&notes->lock);
+	mutex_lock(&notes->lock);
 
 	symbol__calc_percent(symbol, evsel);
 
@@ -267,7 +267,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(&notes->lock);
+	mutex_unlock(&notes->lock);
 }
 
 static void perf_top__resort_hists(struct perf_top *t)
@@ -824,13 +824,13 @@ 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);
 
 		err = hist_entry_iter__add(&iter, &al, top->max_stack, top);
 		if (err < 0)
 			pr_err("Problem incrementing symbol period, skipping event\n");
 
-		pthread_mutex_unlock(&hists->lock);
+		mutex_unlock(&hists->lock);
 	}
 
 	addr_location__put(&al);
@@ -886,10 +886,10 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
 		perf_mmap__consume(md);
 
 		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);
 		}
 	}
 
@@ -1094,10 +1094,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");
@@ -1211,8 +1211,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)
@@ -1330,7 +1330,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/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 82207db8f97c..af87e3fd3b86 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -309,7 +309,7 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
 
 	browser->entries = RB_ROOT;
 
-	pthread_mutex_lock(&notes->lock);
+	mutex_lock(&notes->lock);
 
 	symbol__calc_percent(sym, evsel);
 
@@ -421,7 +421,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
 	}
 
 	notes = symbol__annotation(dl->ops.target.sym);
-	pthread_mutex_lock(&notes->lock);
+	mutex_lock(&notes->lock);
 
 	if (!symbol__hists(dl->ops.target.sym, evsel->evlist->core.nr_entries)) {
 		pthread_mutex_unlock(&notes->lock);
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e830eadfca2a..35b377b382b2 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -803,7 +803,7 @@ void symbol__annotate_zero_histograms(struct symbol *sym)
 {
 	struct annotation *notes = symbol__annotation(sym);
 
-	pthread_mutex_lock(&notes->lock);
+	mutex_lock(&notes->lock);
 	if (notes->src != NULL) {
 		memset(notes->src->histograms, 0,
 		       notes->src->nr_histograms * notes->src->sizeof_sym_hist);
@@ -811,7 +811,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(&notes->lock);
+	mutex_unlock(&notes->lock);
 }
 
 static int __symbol__account_cycles(struct cyc_hist *ch,
@@ -1063,7 +1063,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
 	notes->hit_insn = 0;
 	notes->cover_insn = 0;
 
-	pthread_mutex_lock(&notes->lock);
+	mutex_lock(&notes->lock);
 	for (offset = size - 1; offset >= 0; --offset) {
 		struct cyc_hist *ch;
 
@@ -1082,7 +1082,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
 			notes->have_cycles = true;
 		}
 	}
-	pthread_mutex_unlock(&notes->lock);
+	mutex_unlock(&notes->lock);
 }
 
 int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index d94be9140e31..942776db8e41 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -11,6 +11,7 @@
 #include <pthread.h>
 #include <asm/bug.h>
 #include "symbol_conf.h"
+#include "mutex.h"
 
 struct hist_browser_timer;
 struct hist_entry;
@@ -268,7 +269,7 @@ struct annotated_source {
 };
 
 struct annotation {
-	pthread_mutex_t		lock;
+	struct mutex lock;
 	u64			max_coverage;
 	u64			start;
 	u64			hit_cycles;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 679a1d75090c..07d51c029375 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1565,13 +1565,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;
 }
@@ -2731,7 +2731,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 6a186b668303..214d908c1be3 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -8,6 +8,7 @@
 #include "evsel.h"
 #include "color.h"
 #include "events_stats.h"
+#include "util/mutex.h"
 
 struct hist_entry;
 struct hist_entry_ops;
@@ -88,7 +89,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 events_stats	stats;
 	u64			event_stream;
 	u16			col_len[HISTC_NR_COLS];
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 4b711b13f915..3ab882cce094 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -278,7 +278,7 @@ struct symbol *symbol__new(u64 start, u64 len, u8 binding, u8 type, const char *
 	if (symbol_conf.priv_size) {
 		if (symbol_conf.init_annotation) {
 			struct annotation *notes = (void *)sym;
-			pthread_mutex_init(&notes->lock, NULL);
+			mutex_init(&notes->lock);
 		}
 		sym = ((void *)sym) + symbol_conf.priv_size;
 	}
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index f117d4f4821e..4fd3eb48c073 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>
@@ -49,8 +50,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.25.1

Re: [PATCH v1 1/4] perf mutex: Wrapped usage of pthread_mutex_t

[Arnaldo Carvalho de Melo]

Em Wed, Jul 27, 2022 at 04:49:51PM +0530, gpavithrasha@gmail.com escreveu:
> From: pavithra <gpavithrasha@gmail.com>
> 
> Added a new header file mutex.h that wraps the
> usage of pthread_mutex_t and updated lock in dso.h.

Hi,

	You should create a first patch with just the new mutex.c and
mutex.h files, then you go on to change the users of
pthread_mutex_lock/unlock to the wrappers.

	Also please add the license on the first line of the new
mutex.[ch] files, see Documentation/process/license-rules.rst.

	tldr; probably what you want is to have this single line in
those the two new files (mutex.[ch]):

// SPDX-License-Identifier: GPL-2.0


 
> Signed-off-by: pavithra <gpavithrasha@gmail.com>
> ---
>  tools/perf/util/Build    |  1 +
>  tools/perf/util/dso.c    | 33 ++++++++++++++++-----------------
>  tools/perf/util/dso.h    |  3 ++-
>  tools/perf/util/mutex.c  | 32 ++++++++++++++++++++++++++++++++
>  tools/perf/util/mutex.h  | 15 +++++++++++++++
>  tools/perf/util/symbol.c |  4 ++--
>  6 files changed, 68 insertions(+), 20 deletions(-)
>  create mode 100644 tools/perf/util/mutex.c
>  create mode 100644 tools/perf/util/mutex.h
> 
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 8dcfca1a882f..559c20d94c36 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -120,6 +120,7 @@ perf-y += time-utils.o
>  perf-y += expr-bison.o
>  perf-y += branch.o
>  perf-y += mem2node.o
> +perf-y += mutex.o
>  
>  perf-$(CONFIG_LIBBPF) += bpf-loader.o
>  perf-$(CONFIG_LIBBPF) += bpf_map.o
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index e11ddf86f2b3..605c31585d48 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -426,7 +426,7 @@ void dso__set_module_info(struct dso *dso, struct kmod_path *m,
>   */
>  static LIST_HEAD(dso__data_open);
>  static long dso__data_open_cnt;
> -static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;
> +static struct mutex dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;

Please find in include/linux/mutex.h the replacement for
PTHREAD_MUTEX_INITIALIZER, since we're trying to mimic the kernel mutex
api.

>  
>  static void dso__list_add(struct dso *dso)
>  {
> @@ -633,9 +633,9 @@ static void check_data_close(void)
>   */
>  void dso__data_close(struct dso *dso)
>  {
> -	pthread_mutex_lock(&dso__data_open_lock);
> +	mutex_lock(&dso__data_open_lock);
>  	close_dso(dso);
> -	pthread_mutex_unlock(&dso__data_open_lock);
> +	mutex_unlock(&dso__data_open_lock);
>  }
>  
>  static void try_to_open_dso(struct dso *dso, struct machine *machine)
> @@ -684,20 +684,19 @@ int dso__data_get_fd(struct dso *dso, struct machine *machine)
>  	if (dso->data.status == DSO_DATA_STATUS_ERROR)
>  		return -1;
>  
> -	if (pthread_mutex_lock(&dso__data_open_lock) < 0)
> -		return -1;
> +	mutex_lock(&dso__data_open_lock);
>  
>  	try_to_open_dso(dso, machine);
>  
>  	if (dso->data.fd < 0)
> -		pthread_mutex_unlock(&dso__data_open_lock);
> +		mutex_unlock(&dso__data_open_lock);
>  
>  	return dso->data.fd;
>  }
>  
>  void dso__data_put_fd(struct dso *dso __maybe_unused)
>  {
> -	pthread_mutex_unlock(&dso__data_open_lock);
> +	mutex_unlock(&dso__data_open_lock);
>  }
>  
>  bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
> @@ -756,7 +755,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;
>  
> @@ -765,7 +764,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)
> @@ -802,7 +801,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;
>  
> @@ -823,7 +822,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;
>  }
>  
> @@ -843,7 +842,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
>  {
>  	ssize_t ret;
>  
> -	pthread_mutex_lock(&dso__data_open_lock);
> +	mutex_lock(&dso__data_open_lock);
>  
>  	/*
>  	 * dso->data.fd might be closed if other thread opened another
> @@ -859,7 +858,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
>  
>  	ret = pread(dso->data.fd, data, DSO__DATA_CACHE_SIZE, offset);
>  out:
> -	pthread_mutex_unlock(&dso__data_open_lock);
> +	mutex_unlock(&dso__data_open_lock);
>  	return ret;
>  }
>  
> @@ -953,7 +952,7 @@ static int file_size(struct dso *dso, struct machine *machine)
>  	struct stat st;
>  	char sbuf[STRERR_BUFSIZE];
>  
> -	pthread_mutex_lock(&dso__data_open_lock);
> +	mutex_lock(&dso__data_open_lock);
>  
>  	/*
>  	 * dso->data.fd might be closed if other thread opened another
> @@ -977,7 +976,7 @@ static int file_size(struct dso *dso, struct machine *machine)
>  	dso->data.file_size = st.st_size;
>  
>  out:
> -	pthread_mutex_unlock(&dso__data_open_lock);
> +	mutex_unlock(&dso__data_open_lock);
>  	return ret;
>  }
>  
> @@ -1192,7 +1191,7 @@ struct dso *dso__new(const char *name)
>  		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);
>  	}
>  
> @@ -1226,7 +1225,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 e4dddb76770d..e08b2ab48314 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -11,6 +11,7 @@
>  #include <stdio.h>
>  #include <linux/bitops.h>
>  #include "build-id.h"
> +#include "mutex.h"
>  
>  struct machine;
>  struct map;
> @@ -132,7 +133,7 @@ struct dso_cache {
>  struct auxtrace_cache;
>  
>  struct dso {
> -	pthread_mutex_t	 lock;
> +	struct mutex lock;

There was a previous alignment of the member names, when you see
something like this in code you're changing, please keep the style.

>  	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/mutex.c b/tools/perf/util/mutex.c
> new file mode 100644
> index 000000000000..b7264a1438c4
> --- /dev/null
> +++ b/tools/perf/util/mutex.c
> @@ -0,0 +1,32 @@
> +#include <mutex.h>
> +#include <pthread.h>
> +
> +//to avoid the warning : implicit declaration of BUG_ON,
> +//we add the following 2 headers.
> +#include <linux/compiler.h>
> +#include <linux/kernel.h>
> +
> +void mutex_init(struct mutex *mtx)
> +{
> +pthread_mutexattr_t lock_attr;
> +pthread_mutexattr_init(&lock_attr);
> +pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
> +BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
> +//on success, returns 0.
> +pthread_mutexattr_destroy(&lock_attr);
> +}
> +
> +void mutex_destroy(struct mutex *mtx)
> +{
> +BUG_ON(pthread_mutex_destroy(&mtx->lock));     //on success, returns 0.
> +}
> +
> +void mutex_lock(struct mutex *mtx)
> +{
> +BUG_ON(pthread_mutex_lock(&mtx->lock) != 0);
> +}
> +
> +void mutex_unlock(struct mutex *mtx)
> +{
> +BUG_ON(pthread_mutex_unlock(&mtx->lock) != 0);
> +}
> diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> new file mode 100644
> index 000000000000..ab2ebb98b24a
> --- /dev/null
> +++ b/tools/perf/util/mutex.h
> @@ -0,0 +1,15 @@
> +#ifndef __PERF_MUTEX_H
> +#define _PERF_MUTEX_H
> +
> +#include <pthread.h>
> +
> +struct mutex {
> +pthread_mutex_t lock;
> +};
> +
> +void mutex_lock(struct mutex *mtx);
> +void mutex_unlock(struct mutex *mtx);
> +void mutex_init(struct mutex *mtx);
> +void mutex_destroy(struct mutex *mtx);
> +
> +#endif /* _PERF_MUTEX_H */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a8f80e427674..342be12cfa1e 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1629,7 +1629,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)) {
> @@ -1778,7 +1778,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.25.1
> 

-- 

- Arnaldo

Re: [PATCH v1 1/4] perf mutex: Wrapped usage of pthread_mutex_t

[Namhyung Kim]

Hi,

On Wed, Jul 27, 2022 at 12:35 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Jul 27, 2022 at 04:49:51PM +0530, gpavithrasha@gmail.com escreveu:
> > From: pavithra <gpavithrasha@gmail.com>

Please set the author name to be a full name.

> >
> > Added a new header file mutex.h that wraps the
> > usage of pthread_mutex_t and updated lock in dso.h.

What is the point of the wrapping?  I think it's to add
error checks.  Then you need to describe it here and/or
in the file comment.

>
> Hi,
>
>         You should create a first patch with just the new mutex.c and
> mutex.h files, then you go on to change the users of
> pthread_mutex_lock/unlock to the wrappers.
>
>         Also please add the license on the first line of the new
> mutex.[ch] files, see Documentation/process/license-rules.rst.
>
>         tldr; probably what you want is to have this single line in
> those the two new files (mutex.[ch]):
>
> // SPDX-License-Identifier: GPL-2.0
>
>
>
> > Signed-off-by: pavithra <gpavithrasha@gmail.com>
> > ---

[SNIP]
> > diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> > new file mode 100644
> > index 000000000000..b7264a1438c4
> > --- /dev/null
> > +++ b/tools/perf/util/mutex.c
> > @@ -0,0 +1,32 @@
> > +#include <mutex.h>
> > +#include <pthread.h>
> > +
> > +//to avoid the warning : implicit declaration of BUG_ON,
> > +//we add the following 2 headers.

We usually omit this kind of information.  But if you really
think it's necessary, you can add a single line comment like:

#include <linux/kernel.h>  /* BUG_ON */

> > +#include <linux/compiler.h>
> > +#include <linux/kernel.h>
> > +
> > +void mutex_init(struct mutex *mtx)
> > +{
> > +pthread_mutexattr_t lock_attr;

No indentation?

> > +pthread_mutexattr_init(&lock_attr);
> > +pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
> > +BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
> > +//on success, returns 0.

I believe this belongs to the above line, but it can just be omitted.

> > +pthread_mutexattr_destroy(&lock_attr);
> > +}
> > +
> > +void mutex_destroy(struct mutex *mtx)
> > +{
> > +BUG_ON(pthread_mutex_destroy(&mtx->lock));     //on success, returns 0.
> > +}
> > +
> > +void mutex_lock(struct mutex *mtx)
> > +{
> > +BUG_ON(pthread_mutex_lock(&mtx->lock) != 0);

Maybe this form is better to indicate it returns 0 on success.

Thanks,
Namhyung


> > +}
> > +
> > +void mutex_unlock(struct mutex *mtx)
> > +{
> > +BUG_ON(pthread_mutex_unlock(&mtx->lock) != 0);
> > +}
> > diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> > new file mode 100644
> > index 000000000000..ab2ebb98b24a
> > --- /dev/null
> > +++ b/tools/perf/util/mutex.h
> > @@ -0,0 +1,15 @@
> > +#ifndef __PERF_MUTEX_H
> > +#define _PERF_MUTEX_H
> > +
> > +#include <pthread.h>
> > +
> > +struct mutex {
> > +pthread_mutex_t lock;
> > +};
> > +
> > +void mutex_lock(struct mutex *mtx);
> > +void mutex_unlock(struct mutex *mtx);
> > +void mutex_init(struct mutex *mtx);
> > +void mutex_destroy(struct mutex *mtx);
> > +
> > +#endif /* _PERF_MUTEX_H */
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index a8f80e427674..342be12cfa1e 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1629,7 +1629,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)) {
> > @@ -1778,7 +1778,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.25.1
> >
>
> --
>
> - Arnaldo

Re: [PATCH v1 2/4] perf mutex with nsinfo: Updated pthread_mutex_t usage

[Namhyung Kim]

On Wed, Jul 27, 2022 at 4:20 AM <gpavithrasha@gmail.com> wrote:
>
> From: pavithra <gpavithrasha@gmail.com>
>
> Updated usage of pthread_mutex_t with nsinfo
> with the new wrapped lock(struct) in mutex.h
> (remove data races).

It doesn't look like you updated the usage with new wrapping.
Instead you introduced new usage.  Then I think you need to
describe why it's needed and what problems it solves.

>
> Signed-off-by: pavithra <gpavithrasha@gmail.com>
> ---
>  tools/perf/builtin-inject.c   | 6 +++---
>  tools/perf/util/map.c         | 2 ++
>  tools/perf/util/probe-event.c | 6 +++++-
>  tools/perf/util/symbol.c      | 2 +-
>  4 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 372ecb3e2c06..81eaed8da207 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -388,10 +388,10 @@ static int perf_event__repipe_id_index(struct perf_session *session,
>  static int dso__read_build_id(struct dso *dso)
>  {
>         if (dso->has_build_id)
> -               return 0;
> -
> +               return 0;
> +
>         if (filename__read_build_id(dso->long_name, dso->build_id,
> -                                   sizeof(dso->build_id)) > 0) {
> +                                  sizeof(dso->build_id)) > 0) {

Unrelated whitespace changes.. is it really needed?

Also I'm not sure you are working on acme/perf/core branch.

>                 dso->has_build_id = true;
>                 return 0;
>         }
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 5b83ed1ebbd6..2ef5fe0cc53c 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -214,8 +214,10 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>                         if (!(prot & PROT_EXEC))
>                                 dso__set_loaded(dso);
>                 }
> +               mutex_lock(&dso->lock);
>                 dso->nsinfo = nsi;
>                 dso__put(dso);
> +               mutex_unlock(&dso->lock);
>         }
>         return map;
>  out_delete:
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 91cab5f669d2..e527f2612ba4 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -38,6 +38,7 @@
>  #include "session.h"
>  #include "string2.h"
>  #include "strbuf.h"
> +#include "mutex.h"
>
>  #include <subcmd/pager.h>
>  #include <linux/ctype.h>
> @@ -171,8 +172,11 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
>                 struct map *map;
>
>                 map = dso__new_map(target);
> -               if (map && map->dso)
> +               if (map && map->dso) {
> +                       mutex_lock(&map->dso->lock);
>                         map->dso->nsinfo = nsinfo__get(nsi);
> +                       mutex_unlock(&map->dso->lock);
> +               }
>                 return map;
>         } else {
>                 return kernel_get_module_map(target);
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 342be12cfa1e..4b711b13f915 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1619,7 +1619,7 @@ int dso__load(struct dso *dso, struct map *map)
>         struct nscookie nsc;
>         char newmapname[PATH_MAX];
>         const char *map_path = dso->long_name;
> -
> +       mutex_lock(&dso->lock);

No matching unlock?

Thanks,
Namhyung


>         perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0;
>         if (perfmap) {
>                 if (dso->nsinfo && (dso__find_perf_map(newmapname,
> --
> 2.25.1
>

Re: [PATCH v1 3/4] perf mutex and cond: Updated files mutex.h & mutex.c

[Namhyung Kim]

On Wed, Jul 27, 2022 at 4:20 AM <gpavithrasha@gmail.com> wrote:
>
> From: pavithra <gpavithrasha@gmail.com>
>
> Added new struct and corresponding
> functions to wrap usage of pthread_cond_t.
> Added a new function for mutex_trylock-similar to
> mutex_lock.
>
> Signed-off-by: pavithra <gpavithrasha@gmail.com>
> ---
>  tools/perf/util/mutex.c | 37 ++++++++++++++++++++++++++++++++-----
>  tools/perf/util/mutex.h | 13 +++++++++++--
>  2 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> index b7264a1438c4..9dc37a3f374f 100644
> --- a/tools/perf/util/mutex.c
> +++ b/tools/perf/util/mutex.c
> @@ -1,8 +1,8 @@
>  #include <mutex.h>
>  #include <pthread.h>
>
> -//to avoid the warning : implicit declaration of BUG_ON,
> -//we add the following 2 headers.
> +/*to avoid the warning : implicit declaration of BUG_ON*/
> +/*we add the following 2 headers*/
>  #include <linux/compiler.h>
>  #include <linux/kernel.h>
>
> @@ -11,14 +11,15 @@ void mutex_init(struct mutex *mtx)
>  pthread_mutexattr_t lock_attr;
>  pthread_mutexattr_init(&lock_attr);
>  pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
> -BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
> -//on success, returns 0.
> +/*pthread_mutex_init:on success, returns 0*/
> +BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
>  pthread_mutexattr_destroy(&lock_attr);
>  }
>
>  void mutex_destroy(struct mutex *mtx)
>  {
> -BUG_ON(pthread_mutex_destroy(&mtx->lock));     //on success, returns 0.
> +/*pthread_mutex_destroy:on success, returns 0*/
> +BUG_ON(pthread_mutex_destroy(&mtx->lock));

Above changes should belong to the patch 1.

>  }
>
>  void mutex_lock(struct mutex *mtx)
> @@ -30,3 +31,29 @@ void mutex_unlock(struct mutex *mtx)
>  {
>  BUG_ON(pthread_mutex_unlock(&mtx->lock) != 0);
>  }
> +
> +bool mutex_trylock(struct mutex *mtx)
> +{
> +return pthread_mutex_trylock(&mtx->lock)!=0;
> +}
> +
> +void cond_wait(struct cond *cnd, struct mutex *mtx)
> +{
> +BUG_ON(pthread_cond_wait(&cnd->cond, &mtx->lock) != 0);
> +}
> +
> +void cond_signal(struct cond *cnd)
> +{
> +BUG_ON(pthread_cond_signal(&cnd->cond) != 0);
> +}
> +
> +void cond_init(struct cond *cnd)
> +{
> +pthread_condattr_t attr;
> +
> +pthread_condattr_init(&attr);
> +
> +/*pthread_cond_init:on success, returns 0*/
> +BUG_ON(pthread_cond_init(&cnd->cond, &attr));

Please be consistent with != 0.

Thanks,
Namhyung


> +pthread_condattr_destroy(&attr);
> +}
> diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> index ab2ebb98b24a..f1b4aaa151be 100644
> --- a/tools/perf/util/mutex.h
> +++ b/tools/perf/util/mutex.h
> @@ -1,15 +1,24 @@
>  #ifndef __PERF_MUTEX_H
> -#define _PERF_MUTEX_H
> +#define __PERF_MUTEX_H
>
>  #include <pthread.h>
> +#include <stdbool.h>
>
>  struct mutex {
>  pthread_mutex_t lock;
>  };
>
> +struct cond {
> +pthread_cond_t cond;
> +};
> +
>  void mutex_lock(struct mutex *mtx);
>  void mutex_unlock(struct mutex *mtx);
> +bool mutex_trylock(struct mutex *mtx);
>  void mutex_init(struct mutex *mtx);
>  void mutex_destroy(struct mutex *mtx);
>
> -#endif /* _PERF_MUTEX_H */
> +void cond_wait(struct cond *cnd, struct mutex *mtx);
> +void cond_signal(struct cond *cnd);
> +void cond_init(struct cond *cnd);
> +#endif /* __PERF_MUTEX_H */
> --
> 2.25.1
>

Re: [PATCH v1 4/4] perf mutex and cond: Updated every occurrence of pthread_mutex and pthread_cond

[Namhyung Kim]

On Wed, Jul 27, 2022 at 4:20 AM <gpavithrasha@gmail.com> wrote:
>
> From: pavithra <gpavithrasha@gmail.com>
>
> Structs using pthread_mutex_t and
> pthread_cond_t were updated and all the
> usage of them were replaced with the new
> wrapped functions as per the declaration &
> definition in mutex.h and mutex.c files. Header
> files included in each file were modified so as
> to avoid redefinition errors.

You seem to use a very narrow line width setting.
I think the recommended setup for git commit message
is around 72.

Anyway, to minimize potential bad impact I'd suggest
to break the conversion into separate commits.
One for perf top, another for annotate, and for hist.

>
> Signed-off-by: pavithra <gpavithrasha@gmail.com>
> ---
>  tools/perf/builtin-inject.c       |  1 -
>  tools/perf/builtin-top.c          | 42 +++++++++++++++----------------
>  tools/perf/ui/browsers/annotate.c |  4 +--
>  tools/perf/util/annotate.c        |  8 +++---
>  tools/perf/util/annotate.h        |  3 ++-
>  tools/perf/util/hist.c            |  6 ++---
>  tools/perf/util/hist.h            |  3 ++-
>  tools/perf/util/symbol.c          |  2 +-
>  tools/perf/util/top.h             |  5 ++--
>  9 files changed, 38 insertions(+), 36 deletions(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 81eaed8da207..570472c597c3 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -395,7 +395,6 @@ static int dso__read_build_id(struct dso *dso)
>                 dso->has_build_id = true;
>                 return 0;
>         }
> -

Please try not to touch other code unnecessarily.

>         return -1;
>  }
>
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 1f60124eb19b..e3ca3ba8fcfb 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -132,10 +132,10 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
>         }
>
>         notes = symbol__annotation(sym);
> -       pthread_mutex_lock(&notes->lock);
> +       mutex_lock(&notes->lock);
>
>         if (!symbol__hists(sym, top->evlist->core.nr_entries)) {
> -               pthread_mutex_unlock(&notes->lock);
> +               mutex_unlock(&notes->lock);
>                 pr_err("Not enough memory for annotating '%s' symbol!\n",
>                        sym->name);
>                 sleep(1);
> @@ -151,7 +151,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(&notes->lock);
> +       mutex_unlock(&notes->lock);
>         return err;
>  }
>
> @@ -204,19 +204,19 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>
>         notes = symbol__annotation(sym);
>
> -       if (pthread_mutex_trylock(&notes->lock))
> -               return;
> +       if(mutex_trylock(&notes->lock))
> +       return;
>
>         err = hist_entry__inc_addr_samples(he, sample, evsel, ip);
>
> -       pthread_mutex_unlock(&notes->lock);
> +       mutex_unlock(&notes->lock);
>
>         if (unlikely(err)) {
>                 /*
>                  * 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);
> @@ -226,7 +226,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);
>         }
>  }
>
> @@ -246,7 +246,7 @@ static void perf_top__show_details(struct perf_top *top)
>         symbol = he->ms.sym;
>         notes = symbol__annotation(symbol);
>
> -       pthread_mutex_lock(&notes->lock);
> +       mutex_lock(&notes->lock);
>
>         symbol__calc_percent(symbol, evsel);
>
> @@ -267,7 +267,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(&notes->lock);
> +       mutex_unlock(&notes->lock);
>  }
>
>  static void perf_top__resort_hists(struct perf_top *t)
> @@ -824,13 +824,13 @@ 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);
>
>                 err = hist_entry_iter__add(&iter, &al, top->max_stack, top);
>                 if (err < 0)
>                         pr_err("Problem incrementing symbol period, skipping event\n");
>
> -               pthread_mutex_unlock(&hists->lock);
> +               mutex_unlock(&hists->lock);
>         }
>
>         addr_location__put(&al);
> @@ -886,10 +886,10 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
>                 perf_mmap__consume(md);
>
>                 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);
>                 }
>         }
>
> @@ -1094,10 +1094,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");
> @@ -1211,8 +1211,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)
> @@ -1330,7 +1330,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/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 82207db8f97c..af87e3fd3b86 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -309,7 +309,7 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
>
>         browser->entries = RB_ROOT;
>
> -       pthread_mutex_lock(&notes->lock);
> +       mutex_lock(&notes->lock);
>
>         symbol__calc_percent(sym, evsel);
>
> @@ -421,7 +421,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
>         }
>
>         notes = symbol__annotation(dl->ops.target.sym);
> -       pthread_mutex_lock(&notes->lock);
> +       mutex_lock(&notes->lock);
>
>         if (!symbol__hists(dl->ops.target.sym, evsel->evlist->core.nr_entries)) {
>                 pthread_mutex_unlock(&notes->lock);
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index e830eadfca2a..35b377b382b2 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -803,7 +803,7 @@ void symbol__annotate_zero_histograms(struct symbol *sym)
>  {
>         struct annotation *notes = symbol__annotation(sym);
>
> -       pthread_mutex_lock(&notes->lock);
> +       mutex_lock(&notes->lock);
>         if (notes->src != NULL) {
>                 memset(notes->src->histograms, 0,
>                        notes->src->nr_histograms * notes->src->sizeof_sym_hist);
> @@ -811,7 +811,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(&notes->lock);
> +       mutex_unlock(&notes->lock);
>  }
>
>  static int __symbol__account_cycles(struct cyc_hist *ch,
> @@ -1063,7 +1063,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
>         notes->hit_insn = 0;
>         notes->cover_insn = 0;
>
> -       pthread_mutex_lock(&notes->lock);
> +       mutex_lock(&notes->lock);
>         for (offset = size - 1; offset >= 0; --offset) {
>                 struct cyc_hist *ch;
>
> @@ -1082,7 +1082,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
>                         notes->have_cycles = true;
>                 }
>         }
> -       pthread_mutex_unlock(&notes->lock);
> +       mutex_unlock(&notes->lock);
>  }
>
>  int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index d94be9140e31..942776db8e41 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -11,6 +11,7 @@
>  #include <pthread.h>
>  #include <asm/bug.h>
>  #include "symbol_conf.h"
> +#include "mutex.h"
>
>  struct hist_browser_timer;
>  struct hist_entry;
> @@ -268,7 +269,7 @@ struct annotated_source {
>  };
>
>  struct annotation {
> -       pthread_mutex_t         lock;
> +       struct mutex lock;
>         u64                     max_coverage;
>         u64                     start;
>         u64                     hit_cycles;
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 679a1d75090c..07d51c029375 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1565,13 +1565,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;
>  }
> @@ -2731,7 +2731,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 6a186b668303..214d908c1be3 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -8,6 +8,7 @@
>  #include "evsel.h"
>  #include "color.h"
>  #include "events_stats.h"
> +#include "util/mutex.h"
>
>  struct hist_entry;
>  struct hist_entry_ops;
> @@ -88,7 +89,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;

As Arnaldo mentioned, please keep the existing
indentation.

>         struct events_stats     stats;
>         u64                     event_stream;
>         u16                     col_len[HISTC_NR_COLS];
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 4b711b13f915..3ab882cce094 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -278,7 +278,7 @@ struct symbol *symbol__new(u64 start, u64 len, u8 binding, u8 type, const char *
>         if (symbol_conf.priv_size) {
>                 if (symbol_conf.init_annotation) {
>                         struct annotation *notes = (void *)sym;
> -                       pthread_mutex_init(&notes->lock, NULL);
> +                       mutex_init(&notes->lock);
>                 }
>                 sym = ((void *)sym) + symbol_conf.priv_size;
>         }
> diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
> index f117d4f4821e..4fd3eb48c073 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>
> @@ -49,8 +50,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;

Ditto.

Thanks,
Namhyung


>         } qe;
>  };
>
> --
> 2.25.1
>