[PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis

Building the Linux kernel with Clang and LLVM
 help / color / mirror / Atom feed

* [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis
@ 2025-05-19 22:46 Ian Rogers
  2025-05-19 22:46 ` [PATCH v1 2/3] perf rwsem: Add thread-safety annotations Ian Rogers
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Ian Rogers @ 2025-05-19 22:46 UTC (permalink / raw)
  To: langfei, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Kan Liang, Nathan Chancellor,
	Nick Desaulniers, Bill Wendling, Justin Stitt, James Clark,
	Chaitanya S Prakash, Athira Rajeev, Stephen Brennan, Howard Chu,
	linux-perf-users, linux-kernel, llvm

The pattern:
```
if (x) {
   lock(...)
}
block1;
if (x) {
   unlock(...)
}
```
defeats clang's -Wthread-safety analysis where it complains of locks
held on one path and not another. Add helper functions for "block1"
then restructure as:
```
if (x) {
   lock(...);
   block1();
   unlock(...);
} else {
   block1();
}
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dso.c | 45 +++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 8619b6eea62d..057fcf4225ac 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1349,6 +1349,16 @@ struct dso *machine__findnew_kernel(struct machine *machine, const char *name,
 	return dso;
 }
 
+static void __dso__set_long_name_id(struct dso *dso, const char *name, bool name_allocated)
+{
+	if (dso__long_name_allocated(dso))
+		free((char *)dso__long_name(dso));
+
+	RC_CHK_ACCESS(dso)->long_name = name;
+	RC_CHK_ACCESS(dso)->long_name_len = strlen(name);
+	dso__set_long_name_allocated(dso, name_allocated);
+}
+
 static void dso__set_long_name_id(struct dso *dso, const char *name, bool name_allocated)
 {
 	struct dsos *dsos = dso__dsos(dso);
@@ -1362,18 +1372,11 @@ static void dso__set_long_name_id(struct dso *dso, const char *name, bool name_a
 		 * renaming the dso.
 		 */
 		down_write(&dsos->lock);
-	}
-
-	if (dso__long_name_allocated(dso))
-		free((char *)dso__long_name(dso));
-
-	RC_CHK_ACCESS(dso)->long_name = name;
-	RC_CHK_ACCESS(dso)->long_name_len = strlen(name);
-	dso__set_long_name_allocated(dso, name_allocated);
-
-	if (dsos) {
+		__dso__set_long_name_id(dso, name, name_allocated);
 		dsos->sorted = false;
 		up_write(&dsos->lock);
+	} else {
+		__dso__set_long_name_id(dso, name, name_allocated);
 	}
 }
 
@@ -1451,6 +1454,16 @@ void dso__set_long_name(struct dso *dso, const char *name, bool name_allocated)
 	dso__set_long_name_id(dso, name, name_allocated);
 }
 
+static void __dso__set_short_name(struct dso *dso, const char *name, bool name_allocated)
+{
+	if (dso__short_name_allocated(dso))
+		free((char *)dso__short_name(dso));
+
+	RC_CHK_ACCESS(dso)->short_name		  = name;
+	RC_CHK_ACCESS(dso)->short_name_len	  = strlen(name);
+	dso__set_short_name_allocated(dso, name_allocated);
+}
+
 void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated)
 {
 	struct dsos *dsos = dso__dsos(dso);
@@ -1464,17 +1477,11 @@ void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated)
 		 * renaming the dso.
 		 */
 		down_write(&dsos->lock);
-	}
-	if (dso__short_name_allocated(dso))
-		free((char *)dso__short_name(dso));
-
-	RC_CHK_ACCESS(dso)->short_name		  = name;
-	RC_CHK_ACCESS(dso)->short_name_len	  = strlen(name);
-	dso__set_short_name_allocated(dso, name_allocated);
-
-	if (dsos) {
+		__dso__set_short_name(dso, name, name_allocated);
 		dsos->sorted = false;
 		up_write(&dsos->lock);
+	} else {
+		__dso__set_short_name(dso, name, name_allocated);
 	}
 }
 
-- 
2.49.0.1101.gccaa498523-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v1 2/3] perf rwsem: Add thread-safety annotations
  2025-05-19 22:46 [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Ian Rogers
@ 2025-05-19 22:46 ` Ian Rogers
  2025-05-19 22:46 ` [PATCH v1 3/3] perf thread: Ensure comm_lock held for comm_list Ian Rogers
  2025-05-20 21:17 ` [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Arnaldo Carvalho de Melo
  2 siblings, 0 replies; 4+ messages in thread
From: Ian Rogers @ 2025-05-19 22:46 UTC (permalink / raw)
  To: langfei, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Kan Liang, Nathan Chancellor,
	Nick Desaulniers, Bill Wendling, Justin Stitt, James Clark,
	Chaitanya S Prakash, Athira Rajeev, Stephen Brennan, Howard Chu,
	linux-perf-users, linux-kernel, llvm

Add annotations used by clang's -Wthread-safety. Fix dsos compilation
errors caused by a lock of annotations.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dsos.c  |  3 +++
 tools/perf/util/mutex.h | 11 +++++++++++
 tools/perf/util/rwsem.c |  4 ++++
 tools/perf/util/rwsem.h | 10 +++++-----
 4 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index e0998e2a7c4e..4d213017d202 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -157,6 +157,7 @@ static struct dso *__dsos__find_by_longname_id(struct dsos *dsos,
 					       const char *name,
 					       const struct dso_id *id,
 					       bool write_locked)
+	SHARED_LOCKS_REQUIRED(dsos->lock)
 {
 	struct dsos__key key = {
 		.long_name = name,
@@ -262,6 +263,7 @@ static int dsos__find_id_cb(struct dso *dso, void *data)
 
 static struct dso *__dsos__find_id(struct dsos *dsos, const char *name, const struct dso_id *id,
 				   bool cmp_short, bool write_locked)
+	SHARED_LOCKS_REQUIRED(dsos->lock)
 {
 	struct dso *res;
 
@@ -338,6 +340,7 @@ static struct dso *__dsos__addnew_id(struct dsos *dsos, const char *name, const
 }
 
 static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, const struct dso_id *id)
+	SHARED_LOCKS_REQUIRED(dsos->lock)
 {
 	struct dso *dso = __dsos__find_id(dsos, name, id, false, /*write_locked=*/true);
 
diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
index 62d258c71ded..38458f00846f 100644
--- a/tools/perf/util/mutex.h
+++ b/tools/perf/util/mutex.h
@@ -42,6 +42,12 @@
 /* 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 acquire a shared (reader) lock in the body of a
+ * function, and do not release it.
+ */
+#define SHARED_LOCK_FUNCTION(...)  __attribute__((shared_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.
@@ -55,6 +61,9 @@
 /* Documents a function that expects a mutex to be held prior to entry. */
 #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
 
+/* Documents a function that expects a shared (reader) lock to be held prior to entry. */
+#define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_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))
 
@@ -66,9 +75,11 @@
 #define LOCKS_EXCLUDED(...)
 #define LOCK_RETURNED(x)
 #define EXCLUSIVE_LOCK_FUNCTION(...)
+#define SHARED_LOCK_FUNCTION(...)
 #define UNLOCK_FUNCTION(...)
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...)
 #define EXCLUSIVE_LOCKS_REQUIRED(...)
+#define SHARED_LOCKS_REQUIRED(...)
 #define NO_THREAD_SAFETY_ANALYSIS
 
 #endif
diff --git a/tools/perf/util/rwsem.c b/tools/perf/util/rwsem.c
index 5109167f27f7..9d26832398db 100644
--- a/tools/perf/util/rwsem.c
+++ b/tools/perf/util/rwsem.c
@@ -27,6 +27,7 @@ int exit_rwsem(struct rw_semaphore *sem)
 }
 
 int down_read(struct rw_semaphore *sem)
+	NO_THREAD_SAFETY_ANALYSIS
 {
 #if RWS_ERRORCHECK
 	mutex_lock(&sem->mtx);
@@ -37,6 +38,7 @@ int down_read(struct rw_semaphore *sem)
 }
 
 int up_read(struct rw_semaphore *sem)
+	NO_THREAD_SAFETY_ANALYSIS
 {
 #if RWS_ERRORCHECK
 	mutex_unlock(&sem->mtx);
@@ -47,6 +49,7 @@ int up_read(struct rw_semaphore *sem)
 }
 
 int down_write(struct rw_semaphore *sem)
+	NO_THREAD_SAFETY_ANALYSIS
 {
 #if RWS_ERRORCHECK
 	mutex_lock(&sem->mtx);
@@ -57,6 +60,7 @@ int down_write(struct rw_semaphore *sem)
 }
 
 int up_write(struct rw_semaphore *sem)
+	NO_THREAD_SAFETY_ANALYSIS
 {
 #if RWS_ERRORCHECK
 	mutex_unlock(&sem->mtx);
diff --git a/tools/perf/util/rwsem.h b/tools/perf/util/rwsem.h
index ef5cbc31d967..b102d8143181 100644
--- a/tools/perf/util/rwsem.h
+++ b/tools/perf/util/rwsem.h
@@ -10,7 +10,7 @@
  */
 #define RWS_ERRORCHECK 0
 
-struct rw_semaphore {
+struct LOCKABLE rw_semaphore {
 #if RWS_ERRORCHECK
 	struct mutex mtx;
 #else
@@ -21,10 +21,10 @@ struct rw_semaphore {
 int init_rwsem(struct rw_semaphore *sem);
 int exit_rwsem(struct rw_semaphore *sem);
 
-int down_read(struct rw_semaphore *sem);
-int up_read(struct rw_semaphore *sem);
+int down_read(struct rw_semaphore *sem) SHARED_LOCK_FUNCTION(sem);
+int up_read(struct rw_semaphore *sem) UNLOCK_FUNCTION(sem);
 
-int down_write(struct rw_semaphore *sem);
-int up_write(struct rw_semaphore *sem);
+int down_write(struct rw_semaphore *sem) EXCLUSIVE_LOCK_FUNCTION(sem);
+int up_write(struct rw_semaphore *sem) UNLOCK_FUNCTION(sem);
 
 #endif /* _PERF_RWSEM_H */
-- 
2.49.0.1101.gccaa498523-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v1 3/3] perf thread: Ensure comm_lock held for comm_list
  2025-05-19 22:46 [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Ian Rogers
  2025-05-19 22:46 ` [PATCH v1 2/3] perf rwsem: Add thread-safety annotations Ian Rogers
@ 2025-05-19 22:46 ` Ian Rogers
  2025-05-20 21:17 ` [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Arnaldo Carvalho de Melo
  2 siblings, 0 replies; 4+ messages in thread
From: Ian Rogers @ 2025-05-19 22:46 UTC (permalink / raw)
  To: langfei, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Kan Liang, Nathan Chancellor,
	Nick Desaulniers, Bill Wendling, Justin Stitt, James Clark,
	Chaitanya S Prakash, Athira Rajeev, Stephen Brennan, Howard Chu,
	linux-perf-users, linux-kernel, llvm

Add thread safety annotations for comm_list and add locking for two
instances where the list is accessed without the lock held (in
contradiction to ____thread__set_comm).

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/comm.c   |  2 ++
 tools/perf/util/thread.c | 17 +++++++++++++----
 tools/perf/util/thread.h |  9 +++++----
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 8aa456d7c2cd..9880247a2c33 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -24,6 +24,7 @@ static struct comm_strs {
 static void comm_strs__remove_if_last(struct comm_str *cs);
 
 static void comm_strs__init(void)
+	NO_THREAD_SAFETY_ANALYSIS /* Inherently single threaded due to pthread_once. */
 {
 	init_rwsem(&_comm_strs.lock);
 	_comm_strs.capacity = 16;
@@ -119,6 +120,7 @@ static void comm_strs__remove_if_last(struct comm_str *cs)
 }
 
 static struct comm_str *__comm_strs__find(struct comm_strs *comm_strs, const char *str)
+	SHARED_LOCKS_REQUIRED(comm_strs->lock)
 {
 	struct comm_str **result;
 
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 415c0e5d1e75..c202b98b36c2 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -41,6 +41,7 @@ int thread__init_maps(struct thread *thread, struct machine *machine)
 }
 
 struct thread *thread__new(pid_t pid, pid_t tid)
+	NO_THREAD_SAFETY_ANALYSIS /* Allocation/creation is inherently single threaded. */
 {
 	RC_STRUCT(thread) *_thread = zalloc(sizeof(*_thread));
 	struct thread *thread;
@@ -202,22 +203,29 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp,
 
 struct comm *thread__comm(struct thread *thread)
 {
-	if (list_empty(thread__comm_list(thread)))
-		return NULL;
+	struct comm *res = NULL;
 
-	return list_first_entry(thread__comm_list(thread), struct comm, list);
+	down_read(thread__comm_lock(thread));
+	if (!list_empty(thread__comm_list(thread)))
+		res = list_first_entry(thread__comm_list(thread), struct comm, list);
+	up_read(thread__comm_lock(thread));
+	return res;
 }
 
 struct comm *thread__exec_comm(struct thread *thread)
 {
 	struct comm *comm, *last = NULL, *second_last = NULL;
 
+	down_read(thread__comm_lock(thread));
 	list_for_each_entry(comm, thread__comm_list(thread), list) {
-		if (comm->exec)
+		if (comm->exec) {
+			up_read(thread__comm_lock(thread));
 			return comm;
+		}
 		second_last = last;
 		last = comm;
 	}
+	up_read(thread__comm_lock(thread));
 
 	/*
 	 * 'last' with no start time might be the parent's comm of a synthesized
@@ -233,6 +241,7 @@ struct comm *thread__exec_comm(struct thread *thread)
 
 static int ____thread__set_comm(struct thread *thread, const char *str,
 				u64 timestamp, bool exec)
+	EXCLUSIVE_LOCKS_REQUIRED(thread__comm_lock(thread))
 {
 	struct comm *new, *curr = thread__comm(thread);
 
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index cd574a896418..56e08c8ae005 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -236,14 +236,15 @@ static inline struct rw_semaphore *thread__namespaces_lock(struct thread *thread
 	return &RC_CHK_ACCESS(thread)->namespaces_lock;
 }
 
-static inline struct list_head *thread__comm_list(struct thread *thread)
+static inline struct rw_semaphore *thread__comm_lock(struct thread *thread)
 {
-	return &RC_CHK_ACCESS(thread)->comm_list;
+	return &RC_CHK_ACCESS(thread)->comm_lock;
 }
 
-static inline struct rw_semaphore *thread__comm_lock(struct thread *thread)
+static inline struct list_head *thread__comm_list(struct thread *thread)
+	SHARED_LOCKS_REQUIRED(thread__comm_lock(thread))
 {
-	return &RC_CHK_ACCESS(thread)->comm_lock;
+	return &RC_CHK_ACCESS(thread)->comm_list;
 }
 
 static inline u64 thread__db_id(const struct thread *thread)
-- 
2.49.0.1101.gccaa498523-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis
  2025-05-19 22:46 [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Ian Rogers
  2025-05-19 22:46 ` [PATCH v1 2/3] perf rwsem: Add thread-safety annotations Ian Rogers
  2025-05-19 22:46 ` [PATCH v1 3/3] perf thread: Ensure comm_lock held for comm_list Ian Rogers
@ 2025-05-20 21:17 ` Arnaldo Carvalho de Melo
  2 siblings, 0 replies; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-05-20 21:17 UTC (permalink / raw)
  To: Ian Rogers
  Cc: langfei, Peter Zijlstra, Ingo Molnar, Namhyung Kim, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	James Clark, Chaitanya S Prakash, Athira Rajeev, Stephen Brennan,
	Howard Chu, linux-perf-users, linux-kernel, llvm

On Mon, May 19, 2025 at 03:46:43PM -0700, Ian Rogers wrote:
> The pattern:
> ```
> if (x) {
>    lock(...)
> }
> block1;
> if (x) {
>    unlock(...)
> }
> ```
> defeats clang's -Wthread-safety analysis where it complains of locks
> held on one path and not another. Add helper functions for "block1"
> then restructure as:
> ```
> if (x) {
>    lock(...);
>    block1();
>    unlock(...);
> } else {
>    block1();
> }
> ```

Got the three in the tmp.perf-tools-next branch, will try and test it
further and then push it out to perf-tools-next.

Thanks,

- Arnalod

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-05-20 21:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-19 22:46 [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Ian Rogers
2025-05-19 22:46 ` [PATCH v1 2/3] perf rwsem: Add thread-safety annotations Ian Rogers
2025-05-19 22:46 ` [PATCH v1 3/3] perf thread: Ensure comm_lock held for comm_list Ian Rogers
2025-05-20 21:17 ` [PATCH v1 1/3] perf dso: Minor refactor to allow Wthread-safety analysis Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox