[vmkit-commits] FatLock assertion failure?

[Will Dietz]

On Thu, Dec 1, 2011 at 3:19 PM, Nicolas Geoffray
<nicolas.geoffray at gmail.com> wrote:
> Thanks Will for debugging the problem! Indeed, I commented the releasing of
> the fat lock and it worked without any crashes. That's great news because it
> looks like we nailed it. The bad news though is that if we don't release a
> fat lock, it will never be able to get used again. Even worst is if we need
> to keep a pointer to the associated object. The lock will just keep that
> object alive forever.
> If it helps, I'm ok with submitting a change that basically comments out the
> release of a FatLock. When I'll have time, I'll investigate more at which
> point do we have a race.
> Cheers,
> Nicolas
>

Welcome, I was very glad at how well that fixes it myself :).

There are two issues you mention--one is worrying about consuming Lock
resources, and I think that's not really a concern.  Looking at this:

http://www.research.ibm.com/people/d/dfb/thinlocks-publications.html#Bacon03Retrospective

They claim it never happens in practice, and only theoretically
happens in pathological cases.  They also describe the difficulty of
deflation a tad (but not as well as others) including brief mentions
of solutions that might be of interest to VMKit.  Anyway I don't think
we should worry about running out of space for locks.

The other issue you mention is miscalculating the liveness of an
object through its lock.  I agree this is something we should fix but
I'm okay postponing it for now.

FWIW inlined below is a sample patch that's been working for me.  If
you prefer we can comment out the code instead of removing it, I'm
fine either way.  Removing the spinlocks/thread counts is a nice
bonus.

~Will

>From f4bf1be53feaa8b419fea8654c34fb04f735f61d Mon Sep 17 00:00:00 2001
From: Will Dietz <w at wdtz.org>
Date: Tue, 29 Nov 2011 14:24:44 -0600
Subject: [PATCH] Remove code that deflates fat locks, due to races it doesn't
 work on SMP.

Fixes bug where multithreaded code would occasionally assert out with:
j3: /home/will/vmkit-svn/lib/vmkit/CommonThread/ObjectLocks.cpp:305:
bool vmkit::FatLock::acquire(gc *): Assertion `obj->header &
ThinLock::FatMask' failed.
---
 include/vmkit/ObjectLocks.h            |    9 +------
 lib/vmkit/CommonThread/ObjectLocks.cpp |   43 +------------------------------
 2 files changed, 3 insertions(+), 49 deletions(-)

diff --git a/include/vmkit/ObjectLocks.h b/include/vmkit/ObjectLocks.h
index 1b0b4d3..7409e80 100644
--- a/include/vmkit/ObjectLocks.h
+++ b/include/vmkit/ObjectLocks.h
@@ -68,9 +68,6 @@ public:
 class FatLock : public vmkit::PermanentObject {
 private:
   vmkit::LockRecursive internalLock;
-  vmkit::SpinLock spinLock;
-  uint32_t waitingThreads;
-  uint32_t lockingThreads;
   LockingThread* firstThread;
   gc* associatedObject;
   uint32_t index;
@@ -171,14 +168,10 @@ public:
   static const uint64_t ThinCountShift = NonLockBits;
   static const uint64_t ThinCountAdd = 1LL << NonLockBits;

-  /// initialise - Initialise the value of the lock.
-  ///
-  static void removeFatLock(FatLock* fatLock, LockSystem& table);
-
   /// overflowThinlock - Change the lock of this object to a fat lock because
   /// we have reached the maximum number of locks.
   static void overflowThinLock(gc* object, LockSystem& table);
-
+
   /// changeToFatlock - Change the lock of this object to a fat lock. The lock
   /// may be in a thin lock or fat lock state.
   static FatLock* changeToFatlock(gc* object, LockSystem& table);
diff --git a/lib/vmkit/CommonThread/ObjectLocks.cpp
b/lib/vmkit/CommonThread/ObjectLocks.cpp
index 9456045..cd99492 100644
--- a/lib/vmkit/CommonThread/ObjectLocks.cpp
+++ b/lib/vmkit/CommonThread/ObjectLocks.cpp
@@ -39,27 +39,7 @@ void ThinLock::overflowThinLock(gc* object,
LockSystem& table) {
   } while (((object->header) & ~NonLockBitsMask) != ID);
   assert(obj->associatedObject == object);
 }
-
-/// initialise - Initialise the value of the lock.
-///
-void ThinLock::removeFatLock(FatLock* fatLock, LockSystem& table) {
-  gc* object = fatLock->associatedObject;
-  llvm_gcroot(object, 0);
-  word_t ID;
-  word_t oldValue = 0;
-  word_t newValue = 0;
-  word_t yieldedValue = 0;

-  ID = fatLock->getID();
-  do {
-    oldValue = object->header;
-    newValue = oldValue & NonLockBitsMask;
-    yieldedValue = __sync_val_compare_and_swap(&object->header,
oldValue, newValue);
-  } while (oldValue != yieldedValue);
-  assert((oldValue & NonLockBitsMask) != ID);
-  fatLock->associatedObject = NULL;
-}
-
 FatLock* ThinLock::changeToFatlock(gc* object, LockSystem& table) {
   llvm_gcroot(object, 0);
   if (!(object->header & FatMask)) {
@@ -262,8 +242,6 @@ FatLock::FatLock(uint32_t i, gc* a) {
   firstThread = NULL;
   index = i;
   setAssociatedObject(a);
-  waitingThreads = 0;
-  lockingThreads = 0;
   nextFreeLock = NULL;
 }

@@ -275,11 +253,6 @@ void FatLock::release(gc* obj, LockSystem& table) {
   llvm_gcroot(obj, 0);
   assert(associatedObject && "No associated object when releasing");
   assert(associatedObject == obj && "Mismatch object in lock");
-  if (!waitingThreads && !lockingThreads &&
-      internalLock.recursionCount() == 1) {
-    vmkit::ThinLock::removeFatLock(this, table);
-    table.deallocate(this);
-  }
   internalLock.unlock();
 }

@@ -287,16 +260,8 @@ void FatLock::release(gc* obj, LockSystem& table) {
 ///
 bool FatLock::acquire(gc* obj) {
   llvm_gcroot(obj, 0);
-
-  spinLock.lock();
-  lockingThreads++;
-  spinLock.unlock();
-
+
   internalLock.lock();
-
-  spinLock.lock();
-  lockingThreads--;
-  spinLock.unlock();

   if (associatedObject != obj) {
     internalLock.unlock();
@@ -421,8 +386,6 @@ bool LockingThread::wait(

   bool timeout = false;

-  l->waitingThreads++;
-
   while (!this->interruptFlag && this->nextWaiting) {
     if (timed) {
       timeout = varcondThread.timedWait(&l->internalLock, info);
@@ -432,9 +395,7 @@ bool LockingThread::wait(
     }
   }
   assert(vmkit::ThinLock::owner(self, table) && "Not owner after wait");
-
-  l->waitingThreads--;
-
+
   assert((!l->firstThread || (l->firstThread->prevWaiting &&
          l->firstThread->nextWaiting)) && "Inconsistent list");

-- 
1.7.5.1