Skip to content
Permalink
Browse files

kernel/condition_variable: Granularize locking.

Before this commit, *all* ConditionVariable operations (yes, all;
even Wait, Notify, etc.) went through a single spinlock, that also
protected the sConditionVariableHash. This obviously does not scale
so well with core count, to say the least!

With this commit, we add spinlocks to each Variable and Entry.
This makes locking somewhat more complicated (and nuanced; see
inline comment), but the trade-off seems completely worth it:

(compile HaikuDepot in VMware, 2 cores)
before
real 1m20.219s
user 1m5.619s
sys  0m40.724s

after
real 1m12.667s
user 0m57.684s
sys  0m37.251s

The more cores there are, the more of an optimization this will
likely prove to be. But 10%-across-the-board is not bad to say
the least.

Change-Id: I1e40a997fff58a79e987d7cdcafa8f7358e1115a
  • Loading branch information...
waddlesplash committed Aug 3, 2019
1 parent 489612d commit 37eda488be1c9fee242e8e4bf6ca644dd13441d8
Showing with 67 additions and 38 deletions.
  1. +5 −1 headers/private/kernel/condition_variable.h
  2. +62 −37 src/system/kernel/condition_variable.cpp
@@ -1,5 +1,6 @@
/*
* Copyright 2007-2011, Ingo Weinhold, ingo_weinhold@gmx.de.
* Copyright 2019, Haiku, Inc. All rights reserved.
* Distributed under the terms of the MIT License.
*/
#ifndef _KERNEL_CONDITION_VARIABLE_H
@@ -37,9 +38,10 @@ struct ConditionVariableEntry
inline ConditionVariable* Variable() const { return fVariable; }

private:
inline void AddToVariable(ConditionVariable* variable);
inline void AddToLockedVariable(ConditionVariable* variable);

private:
spinlock fLock;
ConditionVariable* fVariable;
Thread* fThread;
status_t fWaitStatus;
@@ -88,6 +90,8 @@ struct ConditionVariable {

const void* fObject;
const char* fObjectType;

spinlock fLock;
EntryList fEntries;
ConditionVariable* fNext;

@@ -1,5 +1,6 @@
/*
* Copyright 2007-2011, Ingo Weinhold, ingo_weinhold@gmx.de.
* Copyright 2019, Haiku, Inc. All rights reserved.
* Distributed under the terms of the MIT License.
*/

@@ -42,7 +43,7 @@ struct ConditionVariableHashDefinition {

typedef BOpenHashTable<ConditionVariableHashDefinition> ConditionVariableHash;
static ConditionVariableHash sConditionVariableHash;
static spinlock sConditionVariablesLock;
static rw_spinlock sConditionVariableHashLock;


static int
@@ -93,58 +94,80 @@ ConditionVariableEntry::Add(const void* object)
{
ASSERT(object != NULL);

fThread = thread_get_current_thread();

InterruptsSpinLocker _(sConditionVariablesLock);
InterruptsLocker _;
ReadSpinLocker hashLocker(sConditionVariableHashLock);

fVariable = sConditionVariableHash.Lookup(object);
ConditionVariable* variable = sConditionVariableHash.Lookup(object);

if (fVariable == NULL) {
if (variable == NULL) {
fWaitStatus = B_ENTRY_NOT_FOUND;
return false;
}

fWaitStatus = STATUS_ADDED;
fVariable->fEntries.Add(this);
SpinLocker variableLocker(variable->fLock);
hashLocker.Unlock();

AddToLockedVariable(variable);

return true;
}


inline void
ConditionVariableEntry::AddToLockedVariable(ConditionVariable* variable)
{
ASSERT(fVariable == NULL);

B_INITIALIZE_SPINLOCK(&fLock);
fThread = thread_get_current_thread();
fVariable = variable;
fWaitStatus = STATUS_ADDED;
fVariable->fEntries.Add(this);
}


status_t
ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout)
{
#if KDEBUG
if (!are_interrupts_enabled()) {
panic("ConditionVariableEntry::Wait() called with interrupts "
"disabled, entry: %p, variable: %p", this, fVariable);
return B_ERROR;
}
#endif

InterruptsLocker _;

SpinLocker conditionLocker(sConditionVariablesLock);
SpinLocker entryLocker(fLock);

if (fVariable == NULL)
return fWaitStatus;

SpinLocker conditionLocker(fVariable->fLock);

thread_prepare_to_block(fThread, flags,
THREAD_BLOCK_TYPE_CONDITION_VARIABLE, fVariable);

fWaitStatus = STATUS_WAITING;

conditionLocker.Unlock();
entryLocker.Unlock();

status_t error;
if ((flags & (B_RELATIVE_TIMEOUT | B_ABSOLUTE_TIMEOUT)) != 0)
error = thread_block_with_timeout(flags, timeout);
else
error = thread_block();

conditionLocker.Lock();
entryLocker.Lock();

// remove entry from variable, if not done yet
if (fVariable != NULL) {
fVariable->fEntries.Remove(this);
conditionLocker.Lock();
// we need to check we are still in the variable's entries; there may
// have been a race for our removal.
if (fVariable->fEntries.Contains(this))
fVariable->fEntries.Remove(this);
fVariable = NULL;
}

@@ -162,19 +185,6 @@ ConditionVariableEntry::Wait(const void* object, uint32 flags,
}


inline void
ConditionVariableEntry::AddToVariable(ConditionVariable* variable)
{
fThread = thread_get_current_thread();

InterruptsSpinLocker _(sConditionVariablesLock);

fVariable = variable;
fWaitStatus = STATUS_ADDED;
fVariable->fEntries.Add(this);
}


// #pragma mark - ConditionVariable


@@ -186,6 +196,7 @@ ConditionVariable::Init(const void* object, const char* objectType)
fObject = object;
fObjectType = objectType;
new(&fEntries) EntryList;
B_INITIALIZE_SPINLOCK(&fLock);

T_SCHEDULING_ANALYSIS(InitConditionVariable(this, object, objectType));
NotifyWaitObjectListeners(&WaitObjectListener::ConditionVariableInitialized,
@@ -201,13 +212,13 @@ ConditionVariable::Publish(const void* object, const char* objectType)
fObject = object;
fObjectType = objectType;
new(&fEntries) EntryList;
B_INITIALIZE_SPINLOCK(&fLock);

T_SCHEDULING_ANALYSIS(InitConditionVariable(this, object, objectType));
NotifyWaitObjectListeners(&WaitObjectListener::ConditionVariableInitialized,
this);

InterruptsLocker _;
SpinLocker locker(sConditionVariablesLock);
InterruptsWriteSpinLocker _(sConditionVariableHashLock);

ASSERT_PRINT(sConditionVariableHash.Lookup(object) == NULL,
"condition variable: %p\n", sConditionVariableHash.Lookup(object));
@@ -221,7 +232,9 @@ ConditionVariable::Unpublish()
{
ASSERT(fObject != NULL);

InterruptsSpinLocker locker(sConditionVariablesLock);
InterruptsLocker _;
WriteSpinLocker hashLocker(sConditionVariableHashLock);
SpinLocker selfLocker(fLock);

#if KDEBUG
ConditionVariable* variable = sConditionVariableHash.Lookup(fObject);
@@ -235,6 +248,8 @@ ConditionVariable::Unpublish()
fObject = NULL;
fObjectType = NULL;

hashLocker.Unlock();

if (!fEntries.IsEmpty())
_NotifyLocked(true, B_ENTRY_NOT_FOUND);
}
@@ -243,7 +258,8 @@ ConditionVariable::Unpublish()
void
ConditionVariable::Add(ConditionVariableEntry* entry)
{
entry->AddToVariable(this);
InterruptsSpinLocker _(fLock);
entry->AddToLockedVariable(this);
}


@@ -259,7 +275,7 @@ ConditionVariable::Wait(uint32 flags, bigtime_t timeout)
/*static*/ void
ConditionVariable::NotifyOne(const void* object, status_t result)
{
InterruptsSpinLocker locker(sConditionVariablesLock);
InterruptsReadSpinLocker locker(sConditionVariableHashLock);
ConditionVariable* variable = sConditionVariableHash.Lookup(object);
locker.Unlock();
if (variable == NULL)
@@ -272,7 +288,7 @@ ConditionVariable::NotifyOne(const void* object, status_t result)
/*static*/ void
ConditionVariable::NotifyAll(const void* object, status_t result)
{
InterruptsSpinLocker locker(sConditionVariablesLock);
InterruptsReadSpinLocker locker(sConditionVariableHashLock);
ConditionVariable* variable = sConditionVariableHash.Lookup(object);
locker.Unlock();
if (variable == NULL)
@@ -285,7 +301,7 @@ ConditionVariable::NotifyAll(const void* object, status_t result)
void
ConditionVariable::_Notify(bool all, status_t result)
{
InterruptsSpinLocker locker(sConditionVariablesLock);
InterruptsSpinLocker _(fLock);

if (!fEntries.IsEmpty()) {
if (result > B_OK) {
@@ -298,18 +314,25 @@ ConditionVariable::_Notify(bool all, status_t result)
}


/*! Called with interrupts disabled and the condition variable spinlock and
scheduler lock held.
*/
/*! Called with interrupts disabled and the condition variable's spinlock held.
*/
void
ConditionVariable::_NotifyLocked(bool all, status_t result)
{
// dequeue and wake up the blocked threads
// Dequeue and wake up the blocked threads.
// We *cannot* hold our own lock while acquiring the Entry's lock,
// as this leads to a (non-theoretical!) race between the Entry
// entering Wait() and acquiring its own lock, and then acquiring ours.
while (ConditionVariableEntry* entry = fEntries.RemoveHead()) {
release_spinlock(&fLock);

SpinLocker _(entry->fLock);
entry->fVariable = NULL;

if (entry->fWaitStatus <= 0)
if (entry->fWaitStatus <= 0) {
acquire_spinlock(&fLock);
continue;
}

if (entry->fWaitStatus == STATUS_WAITING) {
SpinLocker _(entry->fThread->scheduler_lock);
@@ -318,6 +341,8 @@ ConditionVariable::_NotifyLocked(bool all, status_t result)

entry->fWaitStatus = result;

acquire_spinlock(&fLock);

if (!all)
break;
}

0 comments on commit 37eda48

Please sign in to comment.
You can’t perform that action at this time.