* [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).