[v4,01/53] perf comm: Use regular mutex

  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(-)
  

Hi Ian,

On Thu, Nov 2, 2023 at 10:58 AM Ian Rogers <irogers@google.com> wrote:
>
> The rwsem is only after used for writing so switch to a mutex that has
> better error checking.

Hmm.. ok.  It doesn't make sense to use rwsem without readers.

>
> Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")

But I'm not sure this is a fix.  Other than that,

> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  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.42.0.869.gea05f2083d-goog
>
  

On Sun, Nov 5, 2023 at 9:32 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Thu, Nov 2, 2023 at 10:58 AM Ian Rogers <irogers@google.com> wrote:
> >
> > The rwsem is only after used for writing so switch to a mutex that has
> > better error checking.
>
> Hmm.. ok.  It doesn't make sense to use rwsem without readers.
>
> >
> > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
>
> But I'm not sure this is a fix.  Other than that,

Thanks Namhyung, it fixes the case that you enable RWS_ERRORCHECK in
rwsem.h as the rwsem static initialization is wrong for a mutex.

Ian

> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
>
> Thanks,
> Namhyung
>
>
> > ---
> >  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.42.0.869.gea05f2083d-goog
> >
  

On Sun, Nov 5, 2023 at 1:35 PM Ian Rogers <irogers@google.com> wrote:
>
> On Sun, Nov 5, 2023 at 9:32 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Thu, Nov 2, 2023 at 10:58 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > The rwsem is only after used for writing so switch to a mutex that has
> > > better error checking.
> >
> > Hmm.. ok.  It doesn't make sense to use rwsem without readers.
> >
> > >
> > > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
> >
> > But I'm not sure this is a fix.  Other than that,
>
> Thanks Namhyung, it fixes the case that you enable RWS_ERRORCHECK in
> rwsem.h as the rwsem static initialization is wrong for a mutex.

Sounds like we need a separate fix.  Maybe you need to
add a static initializer macro depending on the config.

Thanks,
Namhyung
  

On Sun, Nov 5, 2023 at 7:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Sun, Nov 5, 2023 at 1:35 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Sun, Nov 5, 2023 at 9:32 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Thu, Nov 2, 2023 at 10:58 AM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > The rwsem is only after used for writing so switch to a mutex that has
> > > > better error checking.
> > >
> > > Hmm.. ok.  It doesn't make sense to use rwsem without readers.
> > >
> > > >
> > > > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
> > >
> > > But I'm not sure this is a fix.  Other than that,
> >
> > Thanks Namhyung, it fixes the case that you enable RWS_ERRORCHECK in
> > rwsem.h as the rwsem static initialization is wrong for a mutex.
>
> Sounds like we need a separate fix.  Maybe you need to
> add a static initializer macro depending on the config.

Agreed, but the only use would be here and switching this case to a
mutex gives extra error checking such as the mutex being taken
recursively. Given that, I prefer the existing change and the static
initializer for rwsem can be a follow up when needed.

Thanks,
Ian

> Thanks,
> Namhyung
  

Em Sun, Nov 05, 2023 at 09:31:47AM -0800, Namhyung Kim escreveu:
> Hi Ian,
> 
> On Thu, Nov 2, 2023 at 10:58 AM Ian Rogers <irogers@google.com> wrote:
> >
> > The rwsem is only after used for writing so switch to a mutex that has
> > better error checking.
> 
> Hmm.. ok.  It doesn't make sense to use rwsem without readers.

Well, the only reader is a findnew method, that will primarily read,
but possibly write if it doesn't find it there, so converting to a
regular mutex seems sensible.

- Arnaldo
 
> > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex")
> 
> But I'm not sure this is a fix.  Other than that,

Yeah, agreed, will remove the fixes.
 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,

- Arnaldo
  

Em Mon, Nov 27, 2023 at 06:53:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sun, Nov 05, 2023 at 09:31:47AM -0800, Namhyung Kim escreveu:
> > Hi Ian,
> > 
> > On Thu, Nov 2, 2023 at 10:58 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > The rwsem is only after used for writing so switch to a mutex that has
> > > better error checking.
> > 
> > Hmm.. ok.  It doesn't make sense to use rwsem without readers.
> 
> Well, the only reader is a findnew method, that will primarily read,
> but possibly write if it doesn't find it there, so converting to a
> regular mutex seems sensible.

To be fixed tomorrow:

   3    32.71 alpine:3.15                   : FAIL gcc version 10.3.1 20211027 (Alpine 10.3.1_git20211027)
    util/comm.c:20:46: error: 'PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP' undeclared here (not in a function)
       20 | static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
          |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [/git/perf-6.6.0-rc1/tools/build/Makefile.build:158: util] Error 2
   4    32.17 alpine:3.16                   : FAIL gcc version 11.2.1 20220219 (Alpine 11.2.1_git20220219)
    util/comm.c:20:46: error: 'PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP' undeclared here (not in a function)
       20 | static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
          |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [/git/perf-6.6.0-rc1/tools/build/Makefile.build:158: util] Error 2
   5    25.82 alpine:3.17                   : FAIL gcc version 12.2.1 20220924 (Alpine 12.2.1_git20220924-r4)
    util/comm.c:20:46: error: 'PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP' undeclared here (not in a function)
       20 | static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
          |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [/git/perf-6.6.0-rc1/tools/build/Makefile.build:158: util] Error 2
   6    26.64 alpine:3.18                   : FAIL gcc version 12.2.1 20220924 (Alpine 12.2.1_git20220924-r10)
    util/comm.c:20:46: error: 'PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP' undeclared here (not in a function)
       20 | static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
          |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [/git/perf-6.6.0-rc1/tools/build/Makefile.build:158: util] Error 2
   7    29.66 alpine:edge                   : FAIL gcc version 13.1.1 20230722 (Alpine 13.1.1_git20230722)
    util/comm.c:20:46: error: 'PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP' undeclared here (not in a function)
       20 | static struct mutex comm_str_lock = {.lock = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,};
          |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [/git/perf-6.6.0-rc1/tools/build/Makefile.build:158: util] Error 2


 I.e. doesn't play well with musl libc.

 - Arnaldo