[libcamera-devel] [PATCH v3 05/12] libcamera: base: Add mutex classes with thread safety annotations
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 1 00:30:12 CET 2021
Hi Hiro,
Thank you for the patch.
On Wed, Dec 01, 2021 at 12:55:53AM +0900, Hirokazu Honda wrote:
> This replaces Mutex and MutexLocker with our own defined classes.
> The classes are annotated by clang thread safety annotations.
> So we can add annotation to code where the classes are used.
>
> v4l2 code needs to be annotated, which violates Mutex capability.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> include/libcamera/base/meson.build | 1 +
> include/libcamera/base/mutex.h | 131 +++++++++++++++++++++++++++++
> include/libcamera/base/thread.h | 7 +-
> src/libcamera/base/meson.build | 1 +
> src/libcamera/base/mutex.cpp | 121 ++++++++++++++++++++++++++
> src/libcamera/base/thread.cpp | 15 ----
> src/v4l2/v4l2_camera_proxy.h | 5 +-
> 7 files changed, 258 insertions(+), 23 deletions(-)
> create mode 100644 include/libcamera/base/mutex.h
> create mode 100644 src/libcamera/base/mutex.cpp
>
> 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..2792551c
> --- /dev/null
> +++ b/include/libcamera/base/mutex.h
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * thread.h - Thread support
Copy and paste ? :-)
> + */
> +
> +#pragma once
> +
> +#include <condition_variable>
> +#include <mutex>
> +
> +#include <libcamera/base/thread_annotations.h>
> +
> +namespace libcamera {
> +
> +class ConditionVariable;
> +
> +#ifndef __DOXYGEN__
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker;
> +#else
> +class MutexLocker;
> +#endif /* __DOXYGEN__ */
You can drop this forward declaration if you used "friend class
MutexLocker" below. Same for ConditionVariable.
> +
> +/* \todo using Mutex = std::mutex if lib++ is used. */
> +
> +#ifndef __DOXYGEN__
> +class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
> +#else
> +class Mutex final
> +#endif /* __DOXYGEN__ */
The conditional compilation could be removed there if we added
diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
index 3d20e3ca460f..aaa09ecb1db7 100644
--- a/Documentation/Doxyfile.in
+++ b/Documentation/Doxyfile.in
@@ -2041,7 +2041,8 @@ INCLUDE_FILE_PATTERNS = *.h
PREDEFINED = __DOXYGEN__ \
__cplusplus \
- __attribute__(x)=
+ __attribute__(x)= \
+ LIBCAMERA_TSA_CAPABILITY(x)=
# If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
# tag can be used to specify a list of macro names that should be expanded. The
I'm not sure if that's better though. It would be nicer to specify
EXPAND_AS_DEFINED = LIBCAMERA_TSA_*
(or a similar syntax), but that's not supported by Doxygen :-( It
shouldn't be hard to implement, but I don't know if it would be accepted
upstream.
I've also tried setting EXPAND_ONLY_PREDEF to NO, and it didn't help. I
may be missing something there.
> +{
> +public:
> + constexpr Mutex()
> + {
> + }
> +
> + void lock() LIBCAMERA_TSA_ACQUIRE()
> + {
> + mutex_.lock();
> + }
> +
> + void unlock() LIBCAMERA_TSA_RELEASE()
> + {
> + mutex_.unlock();
> + }
> +
> +private:
> + friend MutexLocker;
> +
> + std::mutex mutex_;
> +};
> +
> +#ifndef __DOXYGEN__
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
> +#else
> +class MutexLocker final
> +#endif /* __DOXYGEN__ */
> +{
> +public:
> + explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
> + : lock_(mutex.mutex_)
> + {
> + }
> +
> + MutexLocker(Mutex &mutex, std::defer_lock_t t) noexcept LIBCAMERA_TSA_EXCLUDES(mutex)
> + : lock_(mutex.mutex_, t)
> + {
> + }
> +
> + ~MutexLocker() LIBCAMERA_TSA_RELEASE()
> + {
> + }
> +
> + void lock() LIBCAMERA_TSA_ACQUIRE()
> + {
> + lock_.lock();
> + }
> +
> + bool try_lock() LIBCAMERA_TSA_TRY_ACQUIRE(true)
> + {
> + return lock_.try_lock();
> + }
> +
> + void unlock() LIBCAMERA_TSA_RELEASE()
> + {
> + lock_.unlock();
> + }
> +
> +private:
> + friend ConditionVariable;
> +
> + std::unique_lock<std::mutex> lock_;
> +};
> +
> +class ConditionVariable final
> +{
> +public:
> + ConditionVariable()
> + {
> + }
> +
> + void notify_one() noexcept
> + {
> + cv_.notify_one();
> + }
> +
> + void notify_all() noexcept
> + {
> + cv_.notify_all();
> + }
> +
> + template<class Predicate>
> + void wait(MutexLocker &locker, Predicate stopWaiting)
> + {
> + cv_.wait(locker.lock_, stopWaiting);
> + }
> +
> + template<class Rep, class Period, class Predicate>
> + bool wait_for(MutexLocker &locker,
> + const std::chrono::duration<Rep, Period> &relTime,
> + Predicate stopWaiting)
> + {
> + return cv_.wait_for(locker.lock_, relTime, stopWaiting);
> + }
> +
> +private:
> + std::condition_variable cv_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index 1ebf8363..44678c34 100644
> --- a/include/libcamera/base/thread.h
> +++ b/include/libcamera/base/thread.h
> @@ -7,15 +7,14 @@
>
> #pragma once
>
> -#include <condition_variable>
> #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>
>
> @@ -27,10 +26,6 @@ class Object;
> class ThreadData;
> class ThreadMain;
>
> -using ConditionVariable = std::condition_variable;
> -using Mutex = std::mutex;
> -using MutexLocker = std::unique_lock<std::mutex>;
> -
> class Thread
> {
> public:
> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> index 05fed7ac..b93b8505 100644
> --- a/src/libcamera/base/meson.build
> +++ b/src/libcamera/base/meson.build
> @@ -11,6 +11,7 @@ libcamera_base_sources = files([
> 'flags.cpp',
> 'log.cpp',
> 'message.cpp',
> + 'mutex.cpp',
> 'object.cpp',
> 'semaphore.cpp',
> 'signal.cpp',
> diff --git a/src/libcamera/base/mutex.cpp b/src/libcamera/base/mutex.cpp
> new file mode 100644
> index 00000000..327b90c0
> --- /dev/null
> +++ b/src/libcamera/base/mutex.cpp
> @@ -0,0 +1,121 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * mutex.cpp - Mutex classes with clang thread safety annotation
> + */
> +
> +#include <libcamera/base/mutex.h>
> +
> +/**
> + * \file base/mutex.h
> + * \brief Mutex classes with clang thread safety annotation
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Mutex
> + * \brief std::mutex wrapper with clang thread safety annotation
> + */
Let's expand this a little bit.
/**
* \class Mutex
* \brief std::mutex wrapper with clang thread safety annotation
*
* The Mutex class wraps a std::mutex instance to add clang thread safety
* annotation support. The class exposes the same interface as std::mutex and
* can be used as a transparent replacement. It integrates with the
* MutexLocker and ConditionVariable classes.
*
* See https://en.cppreference.com/w/cpp/thread/mutex for the complete API
* documentation.
*/
Same below.
> +
> +/**
> + * \fn Mutex::Mutex()
> + * \brief Construct a Mutex instance
> + */
I wonder, should we drop the documentation for individual members ?
Otherwise we'll end up replicating the C++ library documentation. To
avoid doxygen warnings, we could have, in the .h file,
#ifndef __DOXYGEN__
class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
{
public:
...
};
class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker
{
public:
...
};
class ConditionVariable final
{
public:
...
};
#else
class Mutex final
{
};
class MutexLocker final
{
};
class ConditionVariable final
{
};
#endif /* __DOXYGEN__ */
> +
> +/**
> + * \fn Mutex::lock()
> + * \brief Locks std::mutex
> + */
> +
> +/**
> + * \fn Mutex::unlock()
> + * \brief Unlocks std::mutex
> + */
> +
> +/**
> + * \class MutexLocker
> + * \brief std::unique_lock wrapper with clang thread safety annotation
> + */
> +
> +/**
> + * \fn MutexLocker::MutexLocker(Mutex &mutex)
> + * \brief Constructs MutexLocker with \a mutex as as the associated mutex and
> + * locks \a mutex
> + * \param[in] mutex Mutex to be locked/unlocked by MutexLocker
> + *
> + * This blocks until \a mutex is acquired.
> + */
> +
> +/**
> + * \fn MutexLocker::MutexLocker(Mutex &mutex, std::defer_lock_t t)
> + * \brief Constructs MutexLocker with \a mutex as as the associated mutex but
> + * does not lock \a mutex
> + * \param[in] mutex Mutex to be locked/unlocked by MutexLocker
> + * \param[in] t mark to specify this constructor is called1
> + *
> + */
> +
> +/**
> + * \fn MutexLocker::~MutexLocker()
> + * \brief Destroys MutexLocker and unlock the associated mutex if it is locked
> + */
> +
> +/**
> + * \fn MutexLocker::lock()
> + * \brief Locks the associated mutex
> + */
> +
> +/**
> + * \fn MutexLocker::try_lock()
> + * \brief Tries to lock the associated mutex
> + * \return True if the ownership of the mutex has been acquired successfully,
> + false otherwise.
> + */
> +
> +/**
> + * \fn MutexLocker::unlock()
> + * \brief Unlocks the associated mutex
> + */
> +
> +/**
> + * \class ConditionVariable
> + * \brief std::condition_variable wrapper with clang thread safety annotation
> + */
> +
> +/**
> + * \fn ConditionVariable::ConditionVariable()
> + * \brief Constructs ConditionVariable
> + */
> +
> +/**
> + * \fn ConditionVariable::notify_one()
> + * \brief Delegates std::condition_variable::notify_one()
> + */
> +
> +/**
> + * \fn ConditionVariable::notify_all()
> + * \brief Delegates std::condition_variable::notify_all()
> + */
> +
> +/**
> + * \fn ConditionVariable::wait(MutexLocker& locker, Predicate stopWaiting)
> + * \brief Call std::condition_variable::wait() with \a locker and \a stopWaiting
> + * \param[in] locker MutexLocker that is locked/unlocked during wait()
> + * \param[in] stopWaiting The predicate to be satisfied
> + */
> +
> +/**
> + * \fn ConditionVariable::wait_for(MutexLocker& locker,
> + const std::chrono::duration<Rep, Period> &relTime,
> + Predicate stopWaiting)
> + * \brief Call std::condition_variable::wait_for() with \a locker, \a relTime,
> + \a stopWaiting
> + * \param[in] locker MutexLocker that is locked/unlocked during wait()
> + * \param[in] relTime std::chrono::duration representing the maximum time to
> + spend waiting
> + * \param[in] stopWaiting The predicate to be satisfied
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index b893135f..b2043b7e 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -204,21 +204,6 @@ ThreadData *ThreadData::current()
> return data;
> }
>
> -/**
> - * \typedef ConditionVariable
> - * \brief An alias for std::condition_variable
> - */
> -
> -/**
> - * \typedef Mutex
> - * \brief An alias for std::mutex
> - */
> -
> -/**
> - * \typedef MutexLocker
> - * \brief An alias for std::unique_lock<std::mutex>
> - */
> -
> /**
> * \class Thread
> * \brief A thread of execution
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 040954dd..23be995d 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -14,7 +14,8 @@
> #include <sys/types.h>
> #include <vector>
>
> -#include <libcamera/base/thread.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>
Do you think we could we drop manual inclusion of thread_annotations.h
(here and in other patches) as it's included by mutex.h ? We try not to
rely on indirect inclusion to avoid compilation breakages, but in this
case thread_annotations.h is guaranteed to be included by mutex.h (if it
wasn't, mutex.h wouldn't exist in the first place, we would use the
std::* types).
>
> #include <libcamera/camera.h>
>
> @@ -59,7 +60,7 @@ private:
> int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> - libcamera::Mutex *lock);
> + libcamera::Mutex *lock) LIBCAMERA_TSA_REQUIRES(*lock);
> int vidioc_streamon(V4L2CameraFile *file, int *arg);
> int vidioc_streamoff(V4L2CameraFile *file, int *arg);
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list