Re: [PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 03:25:42PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:13PM +0100, Alexandru Gheorghe wrote:
> > vsyncworker::Routine assumes that when -EINTR is returned by
> > WaitForSignalOrExitLocked the lock as been released, which is not
> > true, so it hangs if a vsyncworker is never enabled and Exit is
> > called.
> > 
> > There are two code paths in WaitForSignalOrExitLocked that return
> > -EINTR, one releases the lock the other doesn't.
> > Looking at the clients of WaitForSignalOrExitLocked all assume the lock
> > is still held, except vsyncworker::Routine.
> > So, the proper fix needs two changes:
> > - Make WaitForSignalOrExitLocked consistent and always hold the lock
> >   when exiting.
> > - Release lock in vsynworker::Routine on all code paths.
> > 
> > Signed-off-by: Alexandru Gheorghe 
> > ---
> >  vsyncworker.cpp | 1 +
> >  worker.cpp  | 6 +++---
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> > index 3bfe4be..7c0c741 100644
> > --- a/vsyncworker.cpp
> > +++ b/vsyncworker.cpp
> > @@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
> >if (!enabled_) {
> >  ret = WaitForSignalOrExitLocked();
> >  if (ret == -EINTR) {
> > +  Unlock();
> >return;
> >  }
> >}
> > diff --git a/worker.cpp b/worker.cpp
> > index da6c580..5b351e0 100644
> > --- a/worker.cpp
> > +++ b/worker.cpp
> > @@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t 
> > max_nanoseconds) {
> >  ret = -ETIMEDOUT;
> >}
> >  
> > +  // release leaves mutex locked when going out of scope
> > +  lk.release();
> > +
> >// exit takes precedence on timeout
> >if (should_exit())
> >  ret = -EINTR;
> >  
> > -  // release leaves mutex locked when going out of scope
> > -  lk.release();
> > -
> 
> I'm not sure why this chunk makes a difference? If the above was
> "return -EINTR;" it would, but it's just assigning ret.
> 

You are certainly right, just dyslexia on my side. 
I will update the patch.

> Sean
> 
> >return ret;
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Cheers,
Alex G
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:13PM +0100, Alexandru Gheorghe wrote:
> vsyncworker::Routine assumes that when -EINTR is returned by
> WaitForSignalOrExitLocked the lock as been released, which is not
> true, so it hangs if a vsyncworker is never enabled and Exit is
> called.
> 
> There are two code paths in WaitForSignalOrExitLocked that return
> -EINTR, one releases the lock the other doesn't.
> Looking at the clients of WaitForSignalOrExitLocked all assume the lock
> is still held, except vsyncworker::Routine.
> So, the proper fix needs two changes:
> - Make WaitForSignalOrExitLocked consistent and always hold the lock
>   when exiting.
> - Release lock in vsynworker::Routine on all code paths.
> 
> Signed-off-by: Alexandru Gheorghe 
> ---
>  vsyncworker.cpp | 1 +
>  worker.cpp  | 6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> index 3bfe4be..7c0c741 100644
> --- a/vsyncworker.cpp
> +++ b/vsyncworker.cpp
> @@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
>if (!enabled_) {
>  ret = WaitForSignalOrExitLocked();
>  if (ret == -EINTR) {
> +  Unlock();
>return;
>  }
>}
> diff --git a/worker.cpp b/worker.cpp
> index da6c580..5b351e0 100644
> --- a/worker.cpp
> +++ b/worker.cpp
> @@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t 
> max_nanoseconds) {
>  ret = -ETIMEDOUT;
>}
>  
> +  // release leaves mutex locked when going out of scope
> +  lk.release();
> +
>// exit takes precedence on timeout
>if (should_exit())
>  ret = -EINTR;
>  
> -  // release leaves mutex locked when going out of scope
> -  lk.release();
> -

I'm not sure why this chunk makes a difference? If the above was
"return -EINTR;" it would, but it's just assigning ret.

Sean

>return ret;
>  }
>  
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Robert Foss]

Hey Alexandru,

Feel free my SoB on this patch:
Signed-off-by: Robert Foss 


Rob.


On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:

vsyncworker::Routine assumes that when -EINTR is returned by
WaitForSignalOrExitLocked the lock as been released, which is not
true, so it hangs if a vsyncworker is never enabled and Exit is
called.

There are two code paths in WaitForSignalOrExitLocked that return
-EINTR, one releases the lock the other doesn't.
Looking at the clients of WaitForSignalOrExitLocked all assume the lock
is still held, except vsyncworker::Routine.
So, the proper fix needs two changes:
- Make WaitForSignalOrExitLocked consistent and always hold the lock
   when exiting.
- Release lock in vsynworker::Routine on all code paths.

Signed-off-by: Alexandru Gheorghe 
---
  vsyncworker.cpp | 1 +
  worker.cpp  | 6 +++---
  2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/vsyncworker.cpp b/vsyncworker.cpp
index 3bfe4be..7c0c741 100644
--- a/vsyncworker.cpp
+++ b/vsyncworker.cpp
@@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
if (!enabled_) {
  ret = WaitForSignalOrExitLocked();
  if (ret == -EINTR) {
+  Unlock();
return;
  }
}
diff --git a/worker.cpp b/worker.cpp
index da6c580..5b351e0 100644
--- a/worker.cpp
+++ b/worker.cpp
@@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t 
max_nanoseconds) {
  ret = -ETIMEDOUT;
}
  
+  // release leaves mutex locked when going out of scope

+  lk.release();
+
// exit takes precedence on timeout
if (should_exit())
  ret = -EINTR;
  
-  // release leaves mutex locked when going out of scope

-  lk.release();
-
return ret;
  }
  


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Alexandru Gheorghe]

vsyncworker::Routine assumes that when -EINTR is returned by
WaitForSignalOrExitLocked the lock as been released, which is not
true, so it hangs if a vsyncworker is never enabled and Exit is
called.

There are two code paths in WaitForSignalOrExitLocked that return
-EINTR, one releases the lock the other doesn't.
Looking at the clients of WaitForSignalOrExitLocked all assume the lock
is still held, except vsyncworker::Routine.
So, the proper fix needs two changes:
- Make WaitForSignalOrExitLocked consistent and always hold the lock
  when exiting.
- Release lock in vsynworker::Routine on all code paths.

Signed-off-by: Alexandru Gheorghe 
---
 vsyncworker.cpp | 1 +
 worker.cpp  | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/vsyncworker.cpp b/vsyncworker.cpp
index 3bfe4be..7c0c741 100644
--- a/vsyncworker.cpp
+++ b/vsyncworker.cpp
@@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
   if (!enabled_) {
 ret = WaitForSignalOrExitLocked();
 if (ret == -EINTR) {
+  Unlock();
   return;
 }
   }
diff --git a/worker.cpp b/worker.cpp
index da6c580..5b351e0 100644
--- a/worker.cpp
+++ b/worker.cpp
@@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t 
max_nanoseconds) {
 ret = -ETIMEDOUT;
   }
 
+  // release leaves mutex locked when going out of scope
+  lk.release();
+
   // exit takes precedence on timeout
   if (should_exit())
 ret = -EINTR;
 
-  // release leaves mutex locked when going out of scope
-  lk.release();
-
   return ret;
 }
 
-- 
2.7.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel