[libcamera-devel] libcamera: base: Make MutexLocker(Mutex &mutex) non-blocking
diff mbox series

Message ID 20220427152640.2192-1-ecurtin@redhat.com
State Rejected
Headers show
Series
  • [libcamera-devel] libcamera: base: Make MutexLocker(Mutex &mutex) non-blocking
Related show

Commit Message

Eric Curtin April 27, 2022, 3:26 p.m. UTC 
Camera::acquire claims to be non-blocking, in order to achieve this,
std::unique_lock<std::mutex> constructor must be called with
try_to_lock so it doesn't block if a lock cannot be obtained.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=127
Signed-off-by: Eric Curtin <ecurtin@redhat.com>
---
 include/libcamera/base/mutex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham April 28, 2022, 9:19 a.m. UTC | #1 
Quoting Eric Curtin via libcamera-devel (2022-04-27 16:26:40)
> Camera::acquire claims to be non-blocking, in order to achieve this,
> std::unique_lock<std::mutex> constructor must be called with
> try_to_lock so it doesn't block if a lock cannot be obtained.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=127
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
>  include/libcamera/base/mutex.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> index 2d23e49e..c4a5f791 100644
> --- a/include/libcamera/base/mutex.h
> +++ b/include/libcamera/base/mutex.h
> @@ -45,7 +45,7 @@ class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
>  {
>  public:
>         explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
> -               : lock_(mutex.mutex_)
> +               : lock_(mutex.mutex_, std::try_to_lock)

Does the code in question already call MutexLocker::try_lock(), but it
still blocks?

I.e. - does this just enable / allow calling lock_.try_lock() where
before callers would block regardless?

I can't quite grasp what the implications are here on other locations
that would like to block ... does it make all calls to lock()
non-blocking now? (even if they need it to be?)

https://en.cppreference.com/w/cpp/thread/lock_tag_t doesn't seem to make
it clear what the full effects are :-(

--
Kieran



>         {
>         }
>  
> -- 
> 2.35.1
>
Eric Curtin April 28, 2022, 11:04 a.m. UTC | #2 
On Thu, 28 Apr 2022 at 10:19, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Eric Curtin via libcamera-devel (2022-04-27 16:26:40)
> > Camera::acquire claims to be non-blocking, in order to achieve this,
> > std::unique_lock<std::mutex> constructor must be called with
> > try_to_lock so it doesn't block if a lock cannot be obtained.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=127
> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > ---
> >  include/libcamera/base/mutex.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> > index 2d23e49e..c4a5f791 100644
> > --- a/include/libcamera/base/mutex.h
> > +++ b/include/libcamera/base/mutex.h
> > @@ -45,7 +45,7 @@ class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
> >  {
> >  public:
> >         explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
> > -               : lock_(mutex.mutex_)
> > +               : lock_(mutex.mutex_, std::try_to_lock)
>
> Does the code in question already call MutexLocker::try_lock(), but it
> still blocks?

By default, the std::unique_lock<Mutex>::unique_lock constructor tries
to obtain a blocking lock.

>
> I.e. - does this just enable / allow calling lock_.try_lock() where
> before callers would block regardless?

Yeah it just makes it non-blocking, so if it can't obtain a lock, it just fails.

>
> I can't quite grasp what the implications are here on other locations
> that would like to block ... does it make all calls to lock()
> non-blocking now? (even if they need it to be?)

Yes, this change probably is too broad, we could alternatively just
call the other constructor for this case alone, that's probably more
correct. Will resubmit this way.

>
> https://en.cppreference.com/w/cpp/thread/lock_tag_t doesn't seem to make

I found this link better personally:

https://en.cppreference.com/w/cpp/thread/unique_lock/unique_lock

> it clear what the full effects are :-(
>
> --
> Kieran
>
>
>
> >         {
> >         }
> >
> > --
> > 2.35.1
> >
>
Laurent Pinchart April 28, 2022, 10:09 p.m. UTC | #3 
Hi Eric,

On Wed, Apr 27, 2022 at 04:26:40PM +0100, Eric Curtin via libcamera-devel wrote:
> Camera::acquire claims to be non-blocking, in order to achieve this,
> std::unique_lock<std::mutex> constructor must be called with
> try_to_lock so it doesn't block if a lock cannot be obtained.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=127
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
>  include/libcamera/base/mutex.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> index 2d23e49e..c4a5f791 100644
> --- a/include/libcamera/base/mutex.h
> +++ b/include/libcamera/base/mutex.h
> @@ -45,7 +45,7 @@ class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
>  {
>  public:
>  	explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
> -		: lock_(mutex.mutex_)
> +		: lock_(mutex.mutex_, std::try_to_lock)

With this change, if the mutex is already locked, MutexLocker will
happily continue as if nothing happened. You've certainly made sure
MutexLocker will not block, by effectively disabling all mutexes in
libcamera.

Please look at the backtrace you've posted in bug 127, and check why
std::mutex::lock() blocks indefinitely.

>  	{
>  	}
>

Patch
diff mbox series 

diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
index 2d23e49e..c4a5f791 100644
--- a/include/libcamera/base/mutex.h
+++ b/include/libcamera/base/mutex.h
@@ -45,7 +45,7 @@  class LIBCAMERA_TSA_SCOPED_CAPABILITY MutexLocker final
 {
 public:
 	explicit MutexLocker(Mutex &mutex) LIBCAMERA_TSA_ACQUIRE(mutex)
-		: lock_(mutex.mutex_)
+		: lock_(mutex.mutex_, std::try_to_lock)
 	{
 	}