Hi Will,<br><br><div class="gmail_quote">On Thu, Dec 1, 2011 at 11:50 PM, Will Dietz <span dir="ltr"><<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On Thu, Dec 1, 2011 at 3:19 PM, Nicolas Geoffray<br>
<<a href="mailto:nicolas.geoffray@gmail.com">nicolas.geoffray@gmail.com</a>> wrote:<br>
> Thanks Will for debugging the problem! Indeed, I commented the releasing of<br>
> the fat lock and it worked without any crashes. That's great news because it<br>
> looks like we nailed it. The bad news though is that if we don't release a<br>
> fat lock, it will never be able to get used again. Even worst is if we need<br>
> to keep a pointer to the associated object. The lock will just keep that<br>
> object alive forever.<br>
> If it helps, I'm ok with submitting a change that basically comments out the<br>
> release of a FatLock. When I'll have time, I'll investigate more at which<br>
> point do we have a race.<br>
> Cheers,<br>
> Nicolas<br>
><br>
<br>
</div>Welcome, I was very glad at how well that fixes it myself :).<br>
<br>
There are two issues you mention--one is worrying about consuming Lock<br>
resources, and I think that's not really a concern. Looking at this:<br>
<br>
<a href="http://www.research.ibm.com/people/d/dfb/thinlocks-publications.html#Bacon03Retrospective" target="_blank">http://www.research.ibm.com/people/d/dfb/thinlocks-publications.html#Bacon03Retrospective</a><br>
<br>
They claim it never happens in practice, and only theoretically<br>
happens in pathological cases. They also describe the difficulty of<br>
deflation a tad (but not as well as others) including brief mentions<br>
of solutions that might be of interest to VMKit. Anyway I don't think<br>
we should worry about running out of space for locks.<br></blockquote><div><br></div><div>I need to read the paper, but inflating a lock in a multi-threaded environment does not look pathological to me.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
The other issue you mention is miscalculating the liveness of an<br>
object through its lock. I agree this is something we should fix but<br>
I'm okay postponing it for now.<br></blockquote><div><br></div><div>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.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
FWIW inlined below is a sample patch that's been working for me. If<br>
you prefer we can comment out the code instead of removing it, I'm<br>
fine either way. Removing the spinlocks/thread counts is a nice<br>
bonus.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>Nicolas</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
~Will<br>
<br>
>From f4bf1be53feaa8b419fea8654c34fb04f735f61d Mon Sep 17 00:00:00 2001<br>
From: Will Dietz <<a href="mailto:w@wdtz.org">w@wdtz.org</a>><br>
Date: Tue, 29 Nov 2011 14:24:44 -0600<br>
Subject: [PATCH] Remove code that deflates fat locks, due to races it doesn't<br>
work on SMP.<br>
<br>
Fixes bug where multithreaded code would occasionally assert out with:<br>
<div class="im">j3: /home/will/vmkit-svn/lib/vmkit/CommonThread/ObjectLocks.cpp:305:<br>
bool vmkit::FatLock::acquire(gc *): Assertion `obj->header &<br>
ThinLock::FatMask' failed.<br>
</div>---<br>
include/vmkit/ObjectLocks.h | 9 +------<br>
lib/vmkit/CommonThread/ObjectLocks.cpp | 43 +------------------------------<br>
2 files changed, 3 insertions(+), 49 deletions(-)<br>
<br>
diff --git a/include/vmkit/ObjectLocks.h b/include/vmkit/ObjectLocks.h<br>
index 1b0b4d3..7409e80 100644<br>
--- a/include/vmkit/ObjectLocks.h<br>
+++ b/include/vmkit/ObjectLocks.h<br>
@@ -68,9 +68,6 @@ public:<br>
class FatLock : public vmkit::PermanentObject {<br>
private:<br>
vmkit::LockRecursive internalLock;<br>
- vmkit::SpinLock spinLock;<br>
- uint32_t waitingThreads;<br>
- uint32_t lockingThreads;<br>
LockingThread* firstThread;<br>
gc* associatedObject;<br>
uint32_t index;<br>
@@ -171,14 +168,10 @@ public:<br>
static const uint64_t ThinCountShift = NonLockBits;<br>
static const uint64_t ThinCountAdd = 1LL << NonLockBits;<br>
<br>
- /// initialise - Initialise the value of the lock.<br>
- ///<br>
- static void removeFatLock(FatLock* fatLock, LockSystem& table);<br>
-<br>
/// overflowThinlock - Change the lock of this object to a fat lock because<br>
/// we have reached the maximum number of locks.<br>
static void overflowThinLock(gc* object, LockSystem& table);<br>
-<br>
+<br>
/// changeToFatlock - Change the lock of this object to a fat lock. The lock<br>
/// may be in a thin lock or fat lock state.<br>
static FatLock* changeToFatlock(gc* object, LockSystem& table);<br>
diff --git a/lib/vmkit/CommonThread/ObjectLocks.cpp<br>
b/lib/vmkit/CommonThread/ObjectLocks.cpp<br>
index 9456045..cd99492 100644<br>
--- a/lib/vmkit/CommonThread/ObjectLocks.cpp<br>
+++ b/lib/vmkit/CommonThread/ObjectLocks.cpp<br>
@@ -39,27 +39,7 @@ void ThinLock::overflowThinLock(gc* object,<br>
LockSystem& table) {<br>
} while (((object->header) & ~NonLockBitsMask) != ID);<br>
assert(obj->associatedObject == object);<br>
}<br>
-<br>
-/// initialise - Initialise the value of the lock.<br>
-///<br>
-void ThinLock::removeFatLock(FatLock* fatLock, LockSystem& table) {<br>
- gc* object = fatLock->associatedObject;<br>
- llvm_gcroot(object, 0);<br>
- word_t ID;<br>
- word_t oldValue = 0;<br>
- word_t newValue = 0;<br>
- word_t yieldedValue = 0;<br>
<br>
- ID = fatLock->getID();<br>
- do {<br>
- oldValue = object->header;<br>
- newValue = oldValue & NonLockBitsMask;<br>
- yieldedValue = __sync_val_compare_and_swap(&object->header,<br>
oldValue, newValue);<br>
- } while (oldValue != yieldedValue);<br>
- assert((oldValue & NonLockBitsMask) != ID);<br>
- fatLock->associatedObject = NULL;<br>
-}<br>
-<br>
FatLock* ThinLock::changeToFatlock(gc* object, LockSystem& table) {<br>
llvm_gcroot(object, 0);<br>
if (!(object->header & FatMask)) {<br>
@@ -262,8 +242,6 @@ FatLock::FatLock(uint32_t i, gc* a) {<br>
firstThread = NULL;<br>
index = i;<br>
setAssociatedObject(a);<br>
- waitingThreads = 0;<br>
- lockingThreads = 0;<br>
nextFreeLock = NULL;<br>
}<br>
<br>
@@ -275,11 +253,6 @@ void FatLock::release(gc* obj, LockSystem& table) {<br>
llvm_gcroot(obj, 0);<br>
assert(associatedObject && "No associated object when releasing");<br>
assert(associatedObject == obj && "Mismatch object in lock");<br>
- if (!waitingThreads && !lockingThreads &&<br>
- internalLock.recursionCount() == 1) {<br>
- vmkit::ThinLock::removeFatLock(this, table);<br>
- table.deallocate(this);<br>
- }<br>
internalLock.unlock();<br>
}<br>
<br>
@@ -287,16 +260,8 @@ void FatLock::release(gc* obj, LockSystem& table) {<br>
///<br>
bool FatLock::acquire(gc* obj) {<br>
llvm_gcroot(obj, 0);<br>
-<br>
- spinLock.lock();<br>
- lockingThreads++;<br>
- spinLock.unlock();<br>
-<br>
+<br>
internalLock.lock();<br>
-<br>
- spinLock.lock();<br>
- lockingThreads--;<br>
- spinLock.unlock();<br>
<br>
if (associatedObject != obj) {<br>
internalLock.unlock();<br>
@@ -421,8 +386,6 @@ bool LockingThread::wait(<br>
<br>
bool timeout = false;<br>
<br>
- l->waitingThreads++;<br>
-<br>
while (!this->interruptFlag && this->nextWaiting) {<br>
if (timed) {<br>
timeout = varcondThread.timedWait(&l->internalLock, info);<br>
@@ -432,9 +395,7 @@ bool LockingThread::wait(<br>
}<br>
}<br>
assert(vmkit::ThinLock::owner(self, table) && "Not owner after wait");<br>
-<br>
- l->waitingThreads--;<br>
-<br>
+<br>
assert((!l->firstThread || (l->firstThread->prevWaiting &&<br>
l->firstThread->nextWaiting)) && "Inconsistent list");<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
1.7.5.1<br>
</font></span></blockquote></div><br>