* [PATCH 1/1] Revert "perf thread: Ensure comm_lock held for comm_list" @ 2025-05-28 15:57 Arnaldo Carvalho de Melo 2025-05-28 15:58 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-05-28 15:57 UTC (permalink / raw) To: Ian Rogers Cc: Namhyung Kim, Adrian Hunter, Alexander Shishkin, Athira Rajeev, Bill Wendling, Chaitanya S Prakash, Fei Lang, Howard Chu, Ingo Molnar, James Clark, Jiri Olsa, Justin Stitt, Kan Liang, Mark Rutland, Nathan Chancellor, Nick Desaulniers, Peter Zijlstra, Stephen Brennan, Linux Kernel Mailing List, linux-perf-users 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] Revert "perf thread: Ensure comm_lock held for comm_list" 2025-05-28 15:57 [PATCH 1/1] Revert "perf thread: Ensure comm_lock held for comm_list" Arnaldo Carvalho de Melo @ 2025-05-28 15:58 ` Arnaldo Carvalho de Melo 2025-05-28 17:55 ` Ian Rogers 0 siblings, 1 reply; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-05-28 15:58 UTC (permalink / raw) To: Ian Rogers Cc: Namhyung Kim, Adrian Hunter, Alexander Shishkin, Athira Rajeev, Bill Wendling, Chaitanya S Prakash, Fei Lang, Howard Chu, Ingo Molnar, James Clark, Jiri Olsa, Justin Stitt, Kan Liang, Mark Rutland, Nathan Chancellor, Nick Desaulniers, Peter Zijlstra, Stephen Brennan, Linux Kernel Mailing List, linux-perf-users 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] Revert "perf thread: Ensure comm_lock held for comm_list" 2025-05-28 15:58 ` Arnaldo Carvalho de Melo @ 2025-05-28 17:55 ` Ian Rogers 0 siblings, 0 replies; 3+ messages in thread From: Ian Rogers @ 2025-05-28 17:55 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Namhyung Kim, Adrian Hunter, Alexander Shishkin, Athira Rajeev, Bill Wendling, Chaitanya S Prakash, Fei Lang, Howard Chu, Ingo Molnar, James Clark, Jiri Olsa, Justin Stitt, Kan Liang, Mark Rutland, Nathan Chancellor, Nick Desaulniers, Peter Zijlstra, Stephen Brennan, Linux Kernel Mailing List, linux-perf-users On Wed, May 28, 2025 at 8:58 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > 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. Hi Arnaldo, I believe it is fixed by: https://lore.kernel.org/lkml/20250528032637.198960-8-irogers@google.com/ I was actually trying to repro a DEBUG=1 issue I'd seen in `perf top` with this assert failing: https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/maps.c?h=perf-tools-next#n80 ``` assert(RC_CHK_EQUAL(map__kmap(map)->kmaps, maps)); ``` but no dice on that - I suspect it has something to do with modules or BPF or something. Anyway, with the patch above `perf top` for me I wasn't able to make it crash even when being aggressive and running with both address and memory sanitizer. Thanks, Ian > - 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 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-28 17:55 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-28 15:57 [PATCH 1/1] Revert "perf thread: Ensure comm_lock held for comm_list" Arnaldo Carvalho de Melo 2025-05-28 15:58 ` Arnaldo Carvalho de Melo 2025-05-28 17:55 ` Ian Rogers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).