[libcamera-devel,v2,2/5] libcamera: base: thread: Apply clang thread safety annotation
diff mbox series

Message ID 20220620165027.549085-3-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • Apply clang thread safety annotation libcamera-core and v4l2
Related show

Commit Message

Umang Jain June 20, 2022, 4:50 p.m. UTC 
From: Hirokazu Honda <hiroh@chromium.org>

This annotates member variables of ThreadData by clang thread
safety annotations.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/libcamera/base/thread.cpp | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Laurent Pinchart Aug. 28, 2022, 6:30 p.m. UTC | #1 
Hi Umang and Hiro,

Thank you for the patch.

On Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
> 
> This annotates member variables of ThreadData by clang thread
> safety annotations.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/libcamera/base/thread.cpp | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 6bda9d14..2e26b83c 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -151,7 +151,7 @@ private:
>  	friend class ThreadMain;
>  
>  	Thread *thread_;
> -	bool running_;
> +	bool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>  	pid_t tid_;
>  
>  	Mutex mutex_;
> @@ -160,7 +160,7 @@ private:
>  
>  	ConditionVariable cv_;
>  	std::atomic<bool> exit_;
> -	int exitCode_;
> +	int exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>  
>  	MessageQueue messages_;
>  };
> @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)
>  	{
>  		MutexLocker locker(data_->mutex_);
>  
> -		if (duration == utils::duration::max())
> -			data_->cv_.wait(locker, [&]() { return !data_->running_; });
> -		else
> -			hasFinished = data_->cv_.wait_for(locker, duration,
> -							  [&]() { return !data_->running_; });
> +		if (duration == utils::duration::max()) {
> +			data_->cv_.wait(
> +				locker,
> +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> +					return !data_->running_;
> +				});
> +		} else {
> +			hasFinished = data_->cv_.wait_for(
> +				locker, duration,
> +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> +					return !data_->running_;
> +				});
> +		}

Let's improve readability a bit:

		auto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
			return !data_->running_;
		});

		if (duration == utils::duration::max())
			data_->cv_.wait(locker, isRunning);
		else
			hasFinished = data_->cv_.wait_for(locker, duration,
							  isRunning);

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	}
>  
>  	if (thread_.joinable())
Laurent Pinchart Aug. 28, 2022, 7:18 p.m. UTC | #2 
On Sun, Aug 28, 2022 at 09:30:08PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Umang and Hiro,
> 
> Thank you for the patch.
> 
> On Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:
> > From: Hirokazu Honda <hiroh@chromium.org>
> > 
> > This annotates member variables of ThreadData by clang thread
> > safety annotations.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/libcamera/base/thread.cpp | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> > index 6bda9d14..2e26b83c 100644
> > --- a/src/libcamera/base/thread.cpp
> > +++ b/src/libcamera/base/thread.cpp
> > @@ -151,7 +151,7 @@ private:
> >  	friend class ThreadMain;
> >  
> >  	Thread *thread_;
> > -	bool running_;
> > +	bool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> >  	pid_t tid_;
> >  
> >  	Mutex mutex_;
> > @@ -160,7 +160,7 @@ private:
> >  
> >  	ConditionVariable cv_;
> >  	std::atomic<bool> exit_;
> > -	int exitCode_;
> > +	int exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> >  
> >  	MessageQueue messages_;
> >  };
> > @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)
> >  	{
> >  		MutexLocker locker(data_->mutex_);
> >  
> > -		if (duration == utils::duration::max())
> > -			data_->cv_.wait(locker, [&]() { return !data_->running_; });
> > -		else
> > -			hasFinished = data_->cv_.wait_for(locker, duration,
> > -							  [&]() { return !data_->running_; });
> > +		if (duration == utils::duration::max()) {
> > +			data_->cv_.wait(
> > +				locker,
> > +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> > +					return !data_->running_;
> > +				});
> > +		} else {
> > +			hasFinished = data_->cv_.wait_for(
> > +				locker, duration,
> > +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> > +					return !data_->running_;
> > +				});
> > +		}
> 
> Let's improve readability a bit:
> 
> 		auto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
> 			return !data_->running_;
> 		});
> 
> 		if (duration == utils::duration::max())
> 			data_->cv_.wait(locker, isRunning);
> 		else
> 			hasFinished = data_->cv_.wait_for(locker, duration,
> 							  isRunning);
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Actually... Have you tried compiling this series with clang ? :-)

../../src/libcamera/base/thread.cpp:395:9: error: writing variable 'exitCode_' requires holding mutex 'data_->mutex_' exclusively [-Werror,-Wthread-safety-analysis]
        data_->exitCode_ = code;
               ^
1 error generated.

The most simple fix is to take the lock in Thread::exit(). It however
goes against the design goal of not requiring locks for exit(), as shown
by the exit_ variable being an atomic. I believe the current
implementation is safe, although it would probably be worth it
revisiting the code to double-check that all necessary memory barriers
are in place. You could thus drop the LIBCAMERA_TSA_GUARDED_BY
annotation for exitCode_.

There's also an error in camera_manager.cpp:

../../src/libcamera/camera_manager.cpp:182:2: error: reading variable 'cameras_' requires holding mutex 'mutex_' [-Werror,-Wthread-safety-analysis]
        cameras_.clear();
        ^
1 error generated.

This shuld just be a matter of taking the lock in the CameraManager
destructor in patch 3/5.

> >  	}
> >  
> >  	if (thread_.joinable())
Umang Jain Nov. 3, 2022, 12:38 p.m. UTC | #3 
Hi Laurent,

On 8/29/22 12:48 AM, Laurent Pinchart wrote:
> On Sun, Aug 28, 2022 at 09:30:08PM +0300, Laurent Pinchart via libcamera-devel wrote:
>> Hi Umang and Hiro,
>>
>> Thank you for the patch.
>>
>> On Mon, Jun 20, 2022 at 10:20:24PM +0530, Umang Jain via libcamera-devel wrote:
>>> From: Hirokazu Honda <hiroh@chromium.org>
>>>
>>> This annotates member variables of ThreadData by clang thread
>>> safety annotations.
>>>
>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   src/libcamera/base/thread.cpp | 22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
>>> index 6bda9d14..2e26b83c 100644
>>> --- a/src/libcamera/base/thread.cpp
>>> +++ b/src/libcamera/base/thread.cpp
>>> @@ -151,7 +151,7 @@ private:
>>>   	friend class ThreadMain;
>>>   
>>>   	Thread *thread_;
>>> -	bool running_;
>>> +	bool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>>>   	pid_t tid_;
>>>   
>>>   	Mutex mutex_;
>>> @@ -160,7 +160,7 @@ private:
>>>   
>>>   	ConditionVariable cv_;
>>>   	std::atomic<bool> exit_;
>>> -	int exitCode_;
>>> +	int exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>>>   
>>>   	MessageQueue messages_;
>>>   };
>>> @@ -422,11 +422,19 @@ bool Thread::wait(utils::duration duration)
>>>   	{
>>>   		MutexLocker locker(data_->mutex_);
>>>   
>>> -		if (duration == utils::duration::max())
>>> -			data_->cv_.wait(locker, [&]() { return !data_->running_; });
>>> -		else
>>> -			hasFinished = data_->cv_.wait_for(locker, duration,
>>> -							  [&]() { return !data_->running_; });
>>> +		if (duration == utils::duration::max()) {
>>> +			data_->cv_.wait(
>>> +				locker,
>>> +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
>>> +					return !data_->running_;
>>> +				});
>>> +		} else {
>>> +			hasFinished = data_->cv_.wait_for(
>>> +				locker, duration,
>>> +				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
>>> +					return !data_->running_;
>>> +				});
>>> +		}
>> Let's improve readability a bit:
>>
>> 		auto isRunning = [&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
>> 			return !data_->running_;
>> 		});
>>
>> 		if (duration == utils::duration::max())
>> 			data_->cv_.wait(locker, isRunning);
>> 		else
>> 			hasFinished = data_->cv_.wait_for(locker, duration,
>> 							  isRunning);
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Actually... Have you tried compiling this series with clang ? :-)

oops, seems I missed :(
>
> ../../src/libcamera/base/thread.cpp:395:9: error: writing variable 'exitCode_' requires holding mutex 'data_->mutex_' exclusively [-Werror,-Wthread-safety-analysis]
>          data_->exitCode_ = code;
>                 ^
> 1 error generated.
>
> The most simple fix is to take the lock in Thread::exit(). It however
> goes against the design goal of not requiring locks for exit(), as shown
> by the exit_ variable being an atomic. I believe the current
> implementation is safe, although it would probably be worth it
> revisiting the code to double-check that all necessary memory barriers
> are in place. You could thus drop the LIBCAMERA_TSA_GUARDED_BY
> annotation for exitCode_.

Ack.
>
> There's also an error in camera_manager.cpp:
>
> ../../src/libcamera/camera_manager.cpp:182:2: error: reading variable 'cameras_' requires holding mutex 'mutex_' [-Werror,-Wthread-safety-analysis]
>          cameras_.clear();
>          ^
> 1 error generated.
>
> This shuld just be a matter of taking the lock in the CameraManager
> destructor in patch 3/5.

Acutally cleanup() is not called in the destructor path but in 
CameraManager::Private::run() , so we need to take a lock here (taking a 
lock in CameraManager::Private::run() didn't seem fine).

-       cameras_.clear();
+       {
+               MutexLocker locker(mutex_);
+               cameras_.clear();
+       }
+

Posting a new v3 to address these changes.
>
>>>   	}
>>>   
>>>   	if (thread_.joinable())

Patch
diff mbox series 

diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
index 6bda9d14..2e26b83c 100644
--- a/src/libcamera/base/thread.cpp
+++ b/src/libcamera/base/thread.cpp
@@ -151,7 +151,7 @@  private:
 	friend class ThreadMain;
 
 	Thread *thread_;
-	bool running_;
+	bool running_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
 	pid_t tid_;
 
 	Mutex mutex_;
@@ -160,7 +160,7 @@  private:
 
 	ConditionVariable cv_;
 	std::atomic<bool> exit_;
-	int exitCode_;
+	int exitCode_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
 
 	MessageQueue messages_;
 };
@@ -422,11 +422,19 @@  bool Thread::wait(utils::duration duration)
 	{
 		MutexLocker locker(data_->mutex_);
 
-		if (duration == utils::duration::max())
-			data_->cv_.wait(locker, [&]() { return !data_->running_; });
-		else
-			hasFinished = data_->cv_.wait_for(locker, duration,
-							  [&]() { return !data_->running_; });
+		if (duration == utils::duration::max()) {
+			data_->cv_.wait(
+				locker,
+				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
+					return !data_->running_;
+				});
+		} else {
+			hasFinished = data_->cv_.wait_for(
+				locker, duration,
+				[&]() LIBCAMERA_TSA_REQUIRES(data_->mutex_) {
+					return !data_->running_;
+				});
+		}
 	}
 
 	if (thread_.joinable())