Re: [PATCH v1 1/4] perf mutex: Wrapped usage of pthread_mutex_t

[Namhyung Kim <namhyung@kernel.org>]

Hi,

On Wed, Jul 27, 2022 at 12:35 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Jul 27, 2022 at 04:49:51PM +0530, gpavithrasha@gmail.com escreveu:
> > From: pavithra <gpavithrasha@gmail.com>

Please set the author name to be a full name.

> >
> > Added a new header file mutex.h that wraps the
> > usage of pthread_mutex_t and updated lock in dso.h.

What is the point of the wrapping?  I think it's to add
error checks.  Then you need to describe it here and/or
in the file comment.

>
> Hi,
>
>         You should create a first patch with just the new mutex.c and
> mutex.h files, then you go on to change the users of
> pthread_mutex_lock/unlock to the wrappers.
>
>         Also please add the license on the first line of the new
> mutex.[ch] files, see Documentation/process/license-rules.rst.
>
>         tldr; probably what you want is to have this single line in
> those the two new files (mutex.[ch]):
>
> // SPDX-License-Identifier: GPL-2.0
>
>
>
> > Signed-off-by: pavithra <gpavithrasha@gmail.com>
> > ---

[SNIP]
> > diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> > new file mode 100644
> > index 000000000000..b7264a1438c4
> > --- /dev/null
> > +++ b/tools/perf/util/mutex.c
> > @@ -0,0 +1,32 @@
> > +#include <mutex.h>
> > +#include <pthread.h>
> > +
> > +//to avoid the warning : implicit declaration of BUG_ON,
> > +//we add the following 2 headers.

We usually omit this kind of information.  But if you really
think it's necessary, you can add a single line comment like:

#include <linux/kernel.h>  /* BUG_ON */

> > +#include <linux/compiler.h>
> > +#include <linux/kernel.h>
> > +
> > +void mutex_init(struct mutex *mtx)
> > +{
> > +pthread_mutexattr_t lock_attr;

No indentation?

> > +pthread_mutexattr_init(&lock_attr);
> > +pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_ERRORCHECK);
> > +BUG_ON(pthread_mutex_init(&mtx->lock, &lock_attr));
> > +//on success, returns 0.

I believe this belongs to the above line, but it can just be omitted.

> > +pthread_mutexattr_destroy(&lock_attr);
> > +}
> > +
> > +void mutex_destroy(struct mutex *mtx)
> > +{
> > +BUG_ON(pthread_mutex_destroy(&mtx->lock));     //on success, returns 0.
> > +}
> > +
> > +void mutex_lock(struct mutex *mtx)
> > +{
> > +BUG_ON(pthread_mutex_lock(&mtx->lock) != 0);

Maybe this form is better to indicate it returns 0 on success.

Thanks,
Namhyung


> > +}
> > +
> > +void mutex_unlock(struct mutex *mtx)
> > +{
> > +BUG_ON(pthread_mutex_unlock(&mtx->lock) != 0);
> > +}
> > diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h
> > new file mode 100644
> > index 000000000000..ab2ebb98b24a
> > --- /dev/null
> > +++ b/tools/perf/util/mutex.h
> > @@ -0,0 +1,15 @@
> > +#ifndef __PERF_MUTEX_H
> > +#define _PERF_MUTEX_H
> > +
> > +#include <pthread.h>
> > +
> > +struct mutex {
> > +pthread_mutex_t lock;
> > +};
> > +
> > +void mutex_lock(struct mutex *mtx);
> > +void mutex_unlock(struct mutex *mtx);
> > +void mutex_init(struct mutex *mtx);
> > +void mutex_destroy(struct mutex *mtx);
> > +
> > +#endif /* _PERF_MUTEX_H */
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index a8f80e427674..342be12cfa1e 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1629,7 +1629,7 @@ int dso__load(struct dso *dso, struct map *map)
> >       }
> >
> >       nsinfo__mountns_enter(dso->nsinfo, &nsc);
> > -     pthread_mutex_lock(&dso->lock);
> > +     mutex_lock(&dso->lock);
> >
> >       /* check again under the dso->lock */
> >       if (dso__loaded(dso)) {
> > @@ -1778,7 +1778,7 @@ int dso__load(struct dso *dso, struct map *map)
> >               ret = 0;
> >  out:
> >       dso__set_loaded(dso);
> > -     pthread_mutex_unlock(&dso->lock);
> > +     mutex_unlock(&dso->lock);
> >       nsinfo__mountns_exit(&nsc);
> >
> >       return ret;
> > --
> > 2.25.1
> >
>
> --
>
> - Arnaldo