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

14841 diff mbox series

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@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

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@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);
>