The rwsem is only after used for writing so switch to a mutex that has
better error checking.

Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/comm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index afb8d4fd2644..4ae7bc2aa9a6 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -17,7 +17,7 @@ struct comm_str {
 
 /* Should perhaps be moved to struct machine */
 static struct rb_root comm_str_root;
-static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
+static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
 
 static struct comm_str *comm_str__get(struct comm_str *cs)
 {
@@ -30,9 +30,9 @@ static struct comm_str *comm_str__get(struct comm_str *cs)
 static void comm_str__put(struct comm_str *cs)
 {
 	if (cs && refcount_dec_and_test(&cs->refcnt)) {
-		down_write(&comm_str_lock);
+		mutex_lock(&comm_str_lock);
 		rb_erase(&cs->rb_node, &comm_str_root);
-		up_write(&comm_str_lock);
+		mutex_unlock(&comm_str_lock);
 		zfree(&cs->str);
 		free(cs);
 	}
@@ -98,9 +98,9 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
 {
 	struct comm_str *cs;
 
-	down_write(&comm_str_lock);
+	mutex_lock(&comm_str_lock);
 	cs = __comm_str__findnew(str, root);
-	up_write(&comm_str_lock);
+	mutex_unlock(&comm_str_lock);
 
 	return cs;
 }
-- 
2.43.0.rc1.413.gea7ed67945-goog

On Mon, Nov 27, 2023 at 2:09 PM Ian Rogers <irogers@google.com> wrote:
>
> The rwsem is only after used for writing so switch to a mutex that has
> better error checking.
>
> Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")

I think we talked about fixing this separately, no?


> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/comm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> index afb8d4fd2644..4ae7bc2aa9a6 100644
> --- a/tools/perf/util/comm.c
> +++ b/tools/perf/util/comm.c
> @@ -17,7 +17,7 @@ struct comm_str {
>
>  /* Should perhaps be moved to struct machine */
>  static struct rb_root comm_str_root;
> -static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> +static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};

IIUC it has a problem with musl libc.  Actually I think it's better to
hide the field and the pthread initializer under some macro like
MUTEX_INITIALIZER or DEFINE_MUTEX() like in the kernel.

Thanks,
Namhyung

>
>  static struct comm_str *comm_str__get(struct comm_str *cs)
>  {
> @@ -30,9 +30,9 @@ static struct comm_str *comm_str__get(struct comm_str *cs)
>  static void comm_str__put(struct comm_str *cs)
>  {
>         if (cs && refcount_dec_and_test(&cs->refcnt)) {
> -               down_write(&comm_str_lock);
> +               mutex_lock(&comm_str_lock);
>                 rb_erase(&cs->rb_node, &comm_str_root);
> -               up_write(&comm_str_lock);
> +               mutex_unlock(&comm_str_lock);
>                 zfree(&cs->str);
>                 free(cs);
>         }
> @@ -98,9 +98,9 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
>  {
>         struct comm_str *cs;
>
> -       down_write(&comm_str_lock);
> +       mutex_lock(&comm_str_lock);
>         cs = __comm_str__findnew(str, root);
> -       up_write(&comm_str_lock);
> +       mutex_unlock(&comm_str_lock);
>
>         return cs;
>  }
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>

On Wed, Nov 29, 2023 at 4:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 2:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > The rwsem is only after used for writing so switch to a mutex that has
> > better error checking.
> >
> > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
>
> I think we talked about fixing this separately, no?

Sorry, I'm unclear on an action to do. Currently changing the
RWS_ERRORCHECK in tools/perf/util/rwsem.h will break the build without
this change.

> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/comm.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> > index afb8d4fd2644..4ae7bc2aa9a6 100644
> > --- a/tools/perf/util/comm.c
> > +++ b/tools/perf/util/comm.c
> > @@ -17,7 +17,7 @@ struct comm_str {
> >
> >  /* Should perhaps be moved to struct machine */
> >  static struct rb_root comm_str_root;
> > -static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > +static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
>
> IIUC it has a problem with musl libc.  Actually I think it's better to
> hide the field and the pthread initializer under some macro like
> MUTEX_INITIALIZER or DEFINE_MUTEX() like in the kernel.

Will there be enough use to justify this? I think ideally we'd not be
having global locks needing global initializers as we run into
problems like we see in metrics needing to mix counting and sampling.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> >  static struct comm_str *comm_str__get(struct comm_str *cs)
> >  {
> > @@ -30,9 +30,9 @@ static struct comm_str *comm_str__get(struct comm_str *cs)
> >  static void comm_str__put(struct comm_str *cs)
> >  {
> >         if (cs && refcount_dec_and_test(&cs->refcnt)) {
> > -               down_write(&comm_str_lock);
> > +               mutex_lock(&comm_str_lock);
> >                 rb_erase(&cs->rb_node, &comm_str_root);
> > -               up_write(&comm_str_lock);
> > +               mutex_unlock(&comm_str_lock);
> >                 zfree(&cs->str);
> >                 free(cs);
> >         }
> > @@ -98,9 +98,9 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
> >  {
> >         struct comm_str *cs;
> >
> > -       down_write(&comm_str_lock);
> > +       mutex_lock(&comm_str_lock);
> >         cs = __comm_str__findnew(str, root);
> > -       up_write(&comm_str_lock);
> > +       mutex_unlock(&comm_str_lock);
> >
> >         return cs;
> >  }
> > --
> > 2.43.0.rc1.413.gea7ed67945-goog
> >

On Thu, Nov 30, 2023 at 10:28 AM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Nov 29, 2023 at 4:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Nov 27, 2023 at 2:09 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > The rwsem is only after used for writing so switch to a mutex that has
> > > better error checking.
> > >
> > > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
> >
> > I think we talked about fixing this separately, no?
>
> Sorry, I'm unclear on an action to do. Currently changing the
> RWS_ERRORCHECK in tools/perf/util/rwsem.h will break the build without
> this change.

Can it be like this?

#ifdef RWS_ERRORCHECK
#define RWSEM_INITIALIZER  { .lock = PTHREAD_MUTEX_INITIALIZER, }
#else
#define RWSEM_INITIALIZER  { .lock = PTHREAD_RWLOCK_INITIALIZER, }
#endif

>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/comm.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> > > index afb8d4fd2644..4ae7bc2aa9a6 100644
> > > --- a/tools/perf/util/comm.c
> > > +++ b/tools/perf/util/comm.c
> > > @@ -17,7 +17,7 @@ struct comm_str {
> > >
> > >  /* Should perhaps be moved to struct machine */
> > >  static struct rb_root comm_str_root;
> > > -static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > > +static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
> >
> > IIUC it has a problem with musl libc.  Actually I think it's better to
> > hide the field and the pthread initializer under some macro like
> > MUTEX_INITIALIZER or DEFINE_MUTEX() like in the kernel.
>
> Will there be enough use to justify this? I think ideally we'd not be
> having global locks needing global initializers as we run into
> problems like we see in metrics needing to mix counting and sampling.

I don't know but there might be a reason to use global locks.
Then we need to support the initialization and it'd be better
to make it easier to handle internal changes like this.

Thanks,
Namhyung

On Sat, Dec 2, 2023 at 3:55 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Nov 30, 2023 at 10:28 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Nov 29, 2023 at 4:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Mon, Nov 27, 2023 at 2:09 PM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > The rwsem is only after used for writing so switch to a mutex that has
> > > > better error checking.
> > > >
> > > > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
> > >
> > > I think we talked about fixing this separately, no?
> >
> > Sorry, I'm unclear on an action to do. Currently changing the
> > RWS_ERRORCHECK in tools/perf/util/rwsem.h will break the build without
> > this change.
>
> Can it be like this?
>
> #ifdef RWS_ERRORCHECK
> #define RWSEM_INITIALIZER  { .lock = PTHREAD_MUTEX_INITIALIZER, }
> #else
> #define RWSEM_INITIALIZER  { .lock = PTHREAD_RWLOCK_INITIALIZER, }
> #endif
>
> >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/util/comm.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> > > > index afb8d4fd2644..4ae7bc2aa9a6 100644
> > > > --- a/tools/perf/util/comm.c
> > > > +++ b/tools/perf/util/comm.c
> > > > @@ -17,7 +17,7 @@ struct comm_str {
> > > >
> > > >  /* Should perhaps be moved to struct machine */
> > > >  static struct rb_root comm_str_root;
> > > > -static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > > > +static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
> > >
> > > IIUC it has a problem with musl libc.  Actually I think it's better to
> > > hide the field and the pthread initializer under some macro like
> > > MUTEX_INITIALIZER or DEFINE_MUTEX() like in the kernel.
> >
> > Will there be enough use to justify this? I think ideally we'd not be
> > having global locks needing global initializers as we run into
> > problems like we see in metrics needing to mix counting and sampling.
>
> I don't know but there might be a reason to use global locks.
> Then we need to support the initialization and it'd be better
> to make it easier to handle internal changes like this.

Right. So you are suggesting I make a macro for initialization but
when this change is applied it will remove the only user of the macro.
The macro would clearly be redundant which is why I didn't do a
separate fix for that before doing this change - to use a mutex as the
rwsem here is only ever used as a write lock. If we're looking to
improve rwsem I don't think adding unused macros is the best thing,
for example, we could remove references to perf_singlethreaded which
is an idea that has had its day.

Thanks,
Ian

> Thanks,
> Namhyung

On Wed, Dec 6, 2023 at 4:05 PM Ian Rogers <irogers@google.com> wrote:
>
> On Sat, Dec 2, 2023 at 3:55 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Nov 30, 2023 at 10:28 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Wed, Nov 29, 2023 at 4:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > On Mon, Nov 27, 2023 at 2:09 PM Ian Rogers <irogers@google.com> wrote:
> > > > >
> > > > > The rwsem is only after used for writing so switch to a mutex that has
> > > > > better error checking.
> > > > >
> > > > > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
> > > >
> > > > I think we talked about fixing this separately, no?
> > >
> > > Sorry, I'm unclear on an action to do. Currently changing the
> > > RWS_ERRORCHECK in tools/perf/util/rwsem.h will break the build without
> > > this change.
> >
> > Can it be like this?
> >
> > #ifdef RWS_ERRORCHECK
> > #define RWSEM_INITIALIZER  { .lock = PTHREAD_MUTEX_INITIALIZER, }
> > #else
> > #define RWSEM_INITIALIZER  { .lock = PTHREAD_RWLOCK_INITIALIZER, }
> > #endif
> >
> > >
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > ---
> > > > >  tools/perf/util/comm.c | 10 +++++-----
> > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> > > > > index afb8d4fd2644..4ae7bc2aa9a6 100644
> > > > > --- a/tools/perf/util/comm.c
> > > > > +++ b/tools/perf/util/comm.c
> > > > > @@ -17,7 +17,7 @@ struct comm_str {
> > > > >
> > > > >  /* Should perhaps be moved to struct machine */
> > > > >  static struct rb_root comm_str_root;
> > > > > -static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> > > > > +static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
> > > >
> > > > IIUC it has a problem with musl libc.  Actually I think it's better to
> > > > hide the field and the pthread initializer under some macro like
> > > > MUTEX_INITIALIZER or DEFINE_MUTEX() like in the kernel.
> > >
> > > Will there be enough use to justify this? I think ideally we'd not be
> > > having global locks needing global initializers as we run into
> > > problems like we see in metrics needing to mix counting and sampling.
> >
> > I don't know but there might be a reason to use global locks.
> > Then we need to support the initialization and it'd be better
> > to make it easier to handle internal changes like this.
>
> Right. So you are suggesting I make a macro for initialization but
> when this change is applied it will remove the only user of the macro.
> The macro would clearly be redundant which is why I didn't do a
> separate fix for that before doing this change - to use a mutex as the
> rwsem here is only ever used as a write lock. If we're looking to
> improve rwsem I don't think adding unused macros is the best thing,
> for example, we could remove references to perf_singlethreaded which
> is an idea that has had its day.

Note, RWS_ERRORCHECK being enabled still is broken without this.

Thanks,
Ian

> Thanks,
> Ian
>
> > Thanks,
> > Namhyung