[libcamera-devel] [RFC PATCH 2/6] libcamera: base: Add mutex classes with thread safety annotations
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 24 04:19:44 CET 2021
Hi Hiro,
On Wed, Nov 24, 2021 at 03:25:29AM +0900, Hirokazu Honda wrote:
> On Mon, Nov 22, 2021 at 11:23 AM Laurent Pinchart wrote:
> > On Fri, Nov 12, 2021 at 04:20:01PM +0900, Hirokazu Honda wrote:
> > > On Fri, Nov 12, 2021 at 7:53 AM Laurent Pinchart wrote:
> > > > On Thu, Nov 11, 2021 at 10:08:35PM +0530, Umang Jain wrote:
> > > > > On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> > > > > > Mutex2 and MutexLocker2 are annotated by clang thread safety
> > > > >
> > > > > Not sure I like the naming here. but I don't have any better suggestion
> > > > > as of now, still contemplating on that.. maybe AMutex and AMutexLocker
> > > > > to denote they are annotated?
> > > >
> > > > I'm also concerned by this. Can't we replace the current Mutex and
> > > > MutexLocker with annotated versions, without having two implementations
> > > > ?
> > >
> > > The problem is condition_variable. It takes MutexLocker
> > > (std::unique_lock) on wait.
> > > However, the new MutexLocker is not std::unique_lock. I avoid this
> > > problem by MutexLocker2::get().
> > > I found [1] MutexLocker derives std::unique_lock, but not sure if it is ok.
> > > [1] https://stackoverflow.com/questions/40468897/clang-thread-safety-with-stdcondition-variable
> >
> > I understand that, but can't we then drop the
> >
> > using Mutex = std::mutex;
> > using MutexLocker = std::unique_lock<std::mutex>;
> >
> > and rename Mutex2 and MutexLocker2 to Mutex and MutexLocker ? Having two
> > options is really awful.
>
> Do you mean to replace Mutex and MutexLocker in the code with
> std::mutex and std::unique_lcok<std::mutex> before this patch?
No, otherwise we wouldn't be able to add thread safety annotation, would
we ?
I don't want to have two different Mutex and MutexLocker classes, one
with manual annotations, and one without. We need a single class that
all the libcamera codebase can use, without having to care about
implementation details.
> > > > > > annotations. So we can add annotation to code where the classes are used.
> > > > > > In the future, they will replace Mutex and MutexLocker.
> > > > >
> > > > > Ok so I see a patch following, that does this replacement. I went into
> > > > > the territory of thinking if this annotations can be used full libcamera
> > > > > codebase. Can it be done (not as part of the series) but as a separate
> > > > > series. I am not sure how much useful it would be since annotation is
> > > > > clang-only. Let's see.
> > > > >
> > > > > So, the series really:
> > > > >
> > > > > In the future, they will replace Mutex and MutexLocker in the
> > > > > android HAL layer.
> > > > >
> > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > ---
> > > > > > include/libcamera/base/meson.build | 1 +
> > > > > > include/libcamera/base/mutex.h | 66 ++++++++++++++++++++++++++++++
> > > > >
> > > > > Please refer to my comment on 1/6 about putting this in
> > > > > include/libcamera/internal, maybe that's a better place?
> > > >
> > > > I think base makes sense, as we already define Mutex and MutexLocker
> > > > there.
> > > >
> > > > > > include/libcamera/base/thread.h | 5 +--
> > > > > > 3 files changed, 68 insertions(+), 4 deletions(-)
> > > > > > create mode 100644 include/libcamera/base/mutex.h
> > > > > >
> > > > > > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> > > > > > index 1a71ce5a..37c4435a 100644
> > > > > > --- a/include/libcamera/base/meson.build
> > > > > > +++ b/include/libcamera/base/meson.build
> > > > > > @@ -13,6 +13,7 @@ libcamera_base_headers = files([
> > > > > > 'flags.h',
> > > > > > 'log.h',
> > > > > > 'message.h',
> > > > > > + 'mutex.h',
> > > > > > 'object.h',
> > > > > > 'private.h',
> > > > > > 'semaphore.h',
> > > > > > diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> > > > > > new file mode 100644
> > > > > > index 00000000..d130988e
> > > > > > --- /dev/null
> > > > > > +++ b/include/libcamera/base/mutex.h
> > > > > > @@ -0,0 +1,66 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > + *
> > > > > > + * thread.h - Thread support
> > > > >
> > > > >
> > > > > oops it should be mutex.h
> > > > >
> > > > >
> > > > > Patch itself looks good to me.
> > > > >
> > > > > > + */
> > > > > > +#ifndef __LIBCAMERA_BASE_MUTEX_H__
> > > > > > +#define __LIBCAMERA_BASE_MUTEX_H__
> > > > > > +
> > > > > > +#include <mutex>
> > > > > > +
> > > > > > +#include <libcamera/base/thread_annotations.h>
> > > > > > +
> > > > > > +namespace libcamera {
> > > > > > +
> > > > > > +using Mutex = std::mutex;
> > > > > > +using MutexLocker = std::unique_lock<std::mutex>;
> > > > > > +
> > > > > > +class CAPABILITY("mutex") Mutex2 final
> > > >
> > > > Documentation is missing.
> > > >
> > > > Thinking further about this class, given that libc++ annotates
> > > > std::mutex, do we a custom annotated class ? It would only be used when
> > > > compiling with clang and libstd++, which seems a bit of a cornercase.
> > >
> > > Yeah, once libc++ annotats std::unique_lock, I would let Mutex be
> > > std::unique_lock if libc++ and otherwise this custom class.
> >
> > I know we need to annotate std::unique_lock, so we need to wrap it in
> > MutexLocker, but with libc++ do we need a custom-annotated Mutex class,
> > or could we simply use
> >
> > using Mutex = std::mutex;
> >
> > then ?
>
> I think you're right.
> So we should have like
> #if defined(USE_LIBCPP)
> using Mutex = std::mutex;
> #else
> class Mutex {
> ...
> };
> #endif
>
> Is this what you demand?
That's what I had in mind, yes.
> > > > > > +{
> > > > > > +public:
> > > >
> > > > The std::mutex constructor is constexpr, I think we should do the same
> > > > here to replicate the same API.
> > > >
> > > > > > + void lock() ACQUIRE()
> > > > > > + {
> > > > > > + mutex_.lock();
> > > > > > + }
> > > > > > +
> > > > > > + void unlock() RELEASE()
> > > > > > + {
> > > > > > + mutex_.unlock();
> > > > > > + }
> > > >
> > > > We also need try_lock().
> > > >
> > > > > > +
> > > > > > + std::mutex &get() { return mutex_; }
> > > >
> > > > Missing blank line.
> > > >
> > > > This function is only used by MutexLocker2. I think a friend declaration
> > > > would be better here, to avoid exposing get().
> > > >
> > > > > > +private:
> > > > > > + std::mutex mutex_;
> > > > > > +};
> > > > > > +
> > > > > > +class SCOPED_CAPABILITY MutexLocker2 final
> > > > > > +{
> > > > > > +public:
> > > > > > + MutexLocker2(Mutex2 &mutex) ACQUIRE(mutex)
> > > > > > + : lock_(mutex.get())
> > > > > > + {
> > > > > > + }
> > > > > > +
> > > > > > + ~MutexLocker2() RELEASE()
> > > > > > + {
> > > > > > + }
> > > > > > +
> > > > > > + void lock() ACQUIRE()
> > > > > > + {
> > > > > > + lock_.lock();
> > > > > > + }
> > > > > > +
> > > > > > + void unlock() RELEASE()
> > > > > > + {
> > > > > > + lock_.unlock();
> > > > > > + }
> > > >
> > > > There are more functions in the std::unique_lock API (including more
> > > > constructors), should we implement them too ?
> > > >
> > > > > > +
> > > > > > + std::unique_lock<std::mutex> &get() { return lock_; }
> > > >
> > > > Missing blank line.
> > > >
> > > > This function is only used to support std::condition_variable::wait(),
> > > > wouldn't it be better to implement a ConditionVariable class that would
> > > > take a MutexLocker argument in wait() ?
> > >
> > > I wondered about it. If we don't like MutexLocker derives
> > > ConditionVariable, then I would try it.
> >
> > It's the get() that bothers me a bit.
>
> Sorry, what do you mean?
> Shall we have our own ConditionVariable or let MutexLocker derive
> std::unique_lock?
One of those two :-) As we define Mutex and MutexLocker types, it could
make sense to add a ConditionVariable type too. However, if that would
work for the libcamera internals, it would limit interoperability with
other APIs that use the C++ standard classes. Having MutexLocker inherit
from std::unique_lock would solve that problem, but I don't know if that
could cause issues. std:: classes are usually not meant to be derived
from, and someone who would cast MutexLocker to std::unique_lock would
be able to call the functions the class offers without going through
annotations, which I assume could lead to warnings.
I'm beginning to wonder if the last of thread safety annotations in
libc++ for std::unique_lock could be caused by the fact that thread
safety analysis doesn't offer the required features to be really
usable. There have been attempts to fix this before (see
https://lists.llvm.org/pipermail/cfe-dev/2016-November/051453.html for
instance), but nothing got merged, which isn't a good sign.
> > > > > > +private:
> > > > > > + std::unique_lock<std::mutex> lock_;
> > > > > > +};
> > > > > > +
> > > > > > +} /* namespace libcamera */
> > > > > > +
> > > > > > +#endif /* __LIBCAMERA_BASE_MUTEX_H__ */
> > > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > > > > > index e0ca0aea..ae630563 100644
> > > > > > --- a/include/libcamera/base/thread.h
> > > > > > +++ b/include/libcamera/base/thread.h
> > > > > > @@ -8,13 +8,13 @@
> > > > > > #define __LIBCAMERA_BASE_THREAD_H__
> > > > > >
> > > > > > #include <memory>
> > > > > > -#include <mutex>
> > > > > > #include <sys/types.h>
> > > > > > #include <thread>
> > > > > >
> > > > > > #include <libcamera/base/private.h>
> > > > > >
> > > > > > #include <libcamera/base/message.h>
> > > > > > +#include <libcamera/base/mutex.h>
> > > > > > #include <libcamera/base/signal.h>
> > > > > > #include <libcamera/base/utils.h>
> > > > > >
> > > > > > @@ -26,9 +26,6 @@ class Object;
> > > > > > class ThreadData;
> > > > > > class ThreadMain;
> > > > > >
> > > > > > -using Mutex = std::mutex;
> > > > > > -using MutexLocker = std::unique_lock<std::mutex>;
> > > > > > -
> > > > > > class Thread
> > > > > > {
> > > > > > public:
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list