[libcamera-devel] [PATCH v2 05/11] libcamera: base: Add mutex classes with thread safety annotations

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Nov 29, 2021 at 08:44:47PM +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     | 127 +++++++++++++++++++++++++++++
>  include/libcamera/base/thread.h    |   7 +-
>  src/v4l2/v4l2_camera_proxy.h       |   5 +-
>  4 files changed, 132 insertions(+), 8 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..55091eb4
> --- /dev/null
> +++ b/include/libcamera/base/mutex.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * thread.h - Thread support
> + */
> +
> +#pragma once
> +
> +#include <condition_variable>
> +#include <mutex>
> +
> +#include <libcamera/base/thread_annotations.h>
> +
> +namespace libcamera {
> +
> +class ConditionVariable;
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker;
> +
> +/* \todo using Mutex = std::mutex if lib++ is used. */
> +class LIBCAMERA_TSA_CAPABILITY("mutex") Mutex final
> +{
> +public:
> +	constexpr Mutex()
> +	{
> +	}
> +
> +	void lock() LIBCAMERA_TSA_ACQUIRE()
> +	{
> +		mutex_.lock();
> +	}
> +
> +	void unlock() LIBCAMERA_TSA_RELEASE()
> +	{
> +		mutex_.unlock();
> +	}
> +
> +private:
> +	friend MutexLocker;
> +
> +	std::mutex mutex_;
> +};
> +
> +class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
> +{
> +public:
> +	explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
> +		: lock_(mutex.mutex_)
> +	{
> +	}
> +
> +	MutexLocker(Mutex &mutex, std::defer_lock_t t) noexcept LIBCAMERA_TSA_ACQUIRE(mutex)
> +		: lock_(mutex.mutex_, t)
> +	{
> +	}
> +
> +	MutexLocker(Mutex &mutex, std::try_to_lock_t t) noexcept LIBCAMERA_TSA_ACQUIRE(mutex)
> +		: lock_(mutex.mutex_, t)
> +	{
> +	}
> +
> +	MutexLocker(Mutex &mutex, std::adopt_lock_t t) noexcept LIBCAMERA_TSA_ACQUIRE(mutex)
> +		: lock_(mutex.mutex_, t)
> +	{
> +	}
> +
> +	~MutexLocker() LIBCAMERA_TSA_RELEASE()
> +	{
> +	}
> +
> +	void lock() LIBCAMERA_TSA_ACQUIRE()
> +	{
> +		lock_.lock();
> +	}
> +
> +	void unlock() LIBCAMERA_TSA_RELEASE()
> +	{
> +		lock_.unlock();
> +	}
> +
> +	bool try_lock() LIBCAMERA_TSA_TRY_ACQUIRE(true)
> +	{
> +		return lock_.try_lock();
> +	}

I'd move try_lock() before unlock() to match the order of the
std::unique_lock documentation.

> +
> +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_;
> +};

There are more functions available in std::unique_lock and
std::condition_variable, but we can implement them later, if needed.

One issue we need to address now, however, is documentation. This
generates lots of Doxygen warnings.

> +
> +} /* 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/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>
>  
>  #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