Re: [PATCH 1/1] Revert "perf thread: Ensure comm_lock held for comm_list"

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Hi Ian,

	This one had hit perf-tools-next, so I'm reverting it till we
figure out, since I'm trying to finish processing patches real soon now
for this window, to give it some time to soak in linux-next.

	I noticed that sometimes when trying to exit 'perf top' it just
sat there, so doing a bisect I ended up on this one, had no time to
properly investigate it.

- Arnaldo

On Wed, May 28, 2025 at 12:57:11PM -0300, Arnaldo Carvalho de Melo wrote:
> This reverts commit 8f454c95817d15ee529d58389612ea4b34f5ffb3.
> 
> 'perf top' is freezing on exit sometimes, bisected to this one, revert.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Cc: Bill Wendling <morbo@google.com>
> Cc: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
> Cc: Fei Lang <langfei@huawei.com>
> Cc: Howard Chu <howardchu95@gmail.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: James Clark <james.clark@linaro.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Justin Stitt <justinstitt@google.com>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <nick.desaulniers+lkml@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephen Brennan <stephen.s.brennan@oracle.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/comm.c   |  2 --
>  tools/perf/util/thread.c | 17 ++++-------------
>  tools/perf/util/thread.h |  9 ++++-----
>  3 files changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> index 9880247a2c3364cb..8aa456d7c2cd2d74 100644
> --- a/tools/perf/util/comm.c
> +++ b/tools/perf/util/comm.c
> @@ -24,7 +24,6 @@ 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;
> @@ -120,7 +119,6 @@ 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 c202b98b36c29215..415c0e5d1e751a47 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -41,7 +41,6 @@ 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;
> @@ -203,29 +202,22 @@ int thread__set_namespaces(struct thread *thread, u64 timestamp,
>  
>  struct comm *thread__comm(struct thread *thread)
>  {
> -	struct comm *res = NULL;
> +	if (list_empty(thread__comm_list(thread)))
> +		return NULL;
>  
> -	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;
> +	return list_first_entry(thread__comm_list(thread), struct comm, list);
>  }
>  
>  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) {
> -			up_read(thread__comm_lock(thread));
> +		if (comm->exec)
>  			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
> @@ -241,7 +233,6 @@ 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 56e08c8ae005e82b..cd574a896418ac94 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -236,15 +236,14 @@ static inline struct rw_semaphore *thread__namespaces_lock(struct thread *thread
>  	return &RC_CHK_ACCESS(thread)->namespaces_lock;
>  }
>  
> -static inline struct rw_semaphore *thread__comm_lock(struct thread *thread)
> +static inline struct list_head *thread__comm_list(struct thread *thread)
>  {
> -	return &RC_CHK_ACCESS(thread)->comm_lock;
> +	return &RC_CHK_ACCESS(thread)->comm_list;
>  }
>  
> -static inline struct list_head *thread__comm_list(struct thread *thread)
> -	SHARED_LOCKS_REQUIRED(thread__comm_lock(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 u64 thread__db_id(const struct thread *thread)
> -- 
> 2.49.0
>