[libcamera-devel] [RFC PATCH 2/6] libcamera: base: Add mutex classes with thread safety annotations
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 11 23:52:46 CET 2021
Hello,
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
?
> > 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.
> > +{
> > +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() ?
> > +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