[libcamera-devel,v6,3/6] ipa: raspberrypi: Switch to std::scoped_lock in the Metadata class

12236 diff mbox series

Replace std::lock_guard with std::scoped_lock. When locking a single
mutex, both are functionally the same. When locking two mutexes in the
operator= overload, the scoped_lock uses a deadlock avoidance algorithm
to avoid deadlock.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/metadata.hpp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

HI Naush

Thanks for this patch. I must confess I haven't encountered this
"scoped_lock" before, but it seems to be just the ticket.

On Mon, 10 May 2021 at 10:59, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Replace std::lock_guard with std::scoped_lock. When locking a single
> mutex, both are functionally the same. When locking two mutexes in the
> operator= overload, the scoped_lock uses a deadlock avoidance algorithm
> to avoid deadlock.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/raspberrypi/controller/metadata.hpp | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
> index 07dd28ed9e0a..e735cfbe0480 100644
> --- a/src/ipa/raspberrypi/controller/metadata.hpp
> +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> @@ -22,14 +22,14 @@ public:
>         template<typename T>
>         void Set(std::string const &tag, T const &value)
>         {
> -               std::lock_guard<std::mutex> lock(mutex_);
> +               std::scoped_lock lock(mutex_);
>                 data_[tag] = value;
>         }
>
>         template<typename T>
>         int Get(std::string const &tag, T &value) const
>         {
> -               std::lock_guard<std::mutex> lock(mutex_);
> +               std::scoped_lock lock(mutex_);
>                 auto it = data_.find(tag);
>                 if (it == data_.end())
>                         return -1;
> @@ -39,14 +39,13 @@ public:
>
>         void Clear()
>         {
> -               std::lock_guard<std::mutex> lock(mutex_);
> +               std::scoped_lock lock(mutex_);
>                 data_.clear();
>         }
>
>         Metadata &operator=(Metadata const &other)
>         {
> -               std::lock_guard<std::mutex> lock(mutex_);
> -               std::lock_guard<std::mutex> other_lock(other.mutex_);
> +               std::scoped_lock lock(mutex_, other.mutex_);
>                 data_ = other.data_;
>                 return *this;
>         }
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Naush,

Thank you for the patch.

On Mon, May 10, 2021 at 10:58:12AM +0100, Naushir Patuck wrote:
> Replace std::lock_guard with std::scoped_lock. When locking a single
> mutex, both are functionally the same. When locking two mutexes in the
> operator= overload, the scoped_lock uses a deadlock avoidance algorithm
> to avoid deadlock.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

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

> ---
>  src/ipa/raspberrypi/controller/metadata.hpp | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
> index 07dd28ed9e0a..e735cfbe0480 100644
> --- a/src/ipa/raspberrypi/controller/metadata.hpp
> +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> @@ -22,14 +22,14 @@ public:
>  	template<typename T>
>  	void Set(std::string const &tag, T const &value)
>  	{
> -		std::lock_guard<std::mutex> lock(mutex_);
> +		std::scoped_lock lock(mutex_);
>  		data_[tag] = value;
>  	}
>  
>  	template<typename T>
>  	int Get(std::string const &tag, T &value) const
>  	{
> -		std::lock_guard<std::mutex> lock(mutex_);
> +		std::scoped_lock lock(mutex_);
>  		auto it = data_.find(tag);
>  		if (it == data_.end())
>  			return -1;
> @@ -39,14 +39,13 @@ public:
>  
>  	void Clear()
>  	{
> -		std::lock_guard<std::mutex> lock(mutex_);
> +		std::scoped_lock lock(mutex_);
>  		data_.clear();
>  	}
>  
>  	Metadata &operator=(Metadata const &other)
>  	{
> -		std::lock_guard<std::mutex> lock(mutex_);
> -		std::lock_guard<std::mutex> other_lock(other.mutex_);
> +		std::scoped_lock lock(mutex_, other.mutex_);
>  		data_ = other.data_;
>  		return *this;
>  	}