[vmkit-commits] FatLock assertion failure?

[Nicolas Geoffray]

Hi Will,

On Thu, Dec 1, 2011 at 11:50 PM, Will Dietz <wdietz2 at illinois.edu> wrote:

> 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.
>

I need to read the paper, but inflating a lock in a multi-threaded
environment does not look pathological to me.


>
> 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.
>

Yes, that's a bigger issue. I'd be fine not reallocating the lock, if I was
assured the associated object could be GC'ed.


>
> 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.
>

I'd rather comment it out. It's been a long time I haven't touched that
code, and these fields were needed for a proper (modulo the bugs we're
currently seeing :)) implementation.

Nicolas


>
> ~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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/vmkit-commits/attachments/20111202/15442cf5/attachment.html>