| Message ID | 1523460149-1740-3-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey Alexandru, Feel free my SoB on this patch: Signed-off-by: Robert Foss <robert.foss@collabora.com> 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 <alexandru-cosmin.gheorghe@arm.com> > --- > 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; > } > >
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 <alexandru-cosmin.gheorghe@arm.com> > --- > 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 >
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 <alexandru-cosmin.gheorghe@arm.com> > > --- > > 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
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; }
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 <alexandru-cosmin.gheorghe@arm.com> --- vsyncworker.cpp | 1 + worker.cpp | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-)