hrev53335 adds 2 changesets to branch 'master'
old head: 6cb38c63195a275a7a65b4fb79499586ead07432
new head: 37eda488be1c9fee242e8e4bf6ca644dd13441d8
overview:
https://git.haiku-os.org/haiku/log/?qt=range&q=37eda488be1c+%5E6cb38c63195a
----------------------------------------------------------------------------
489612d43f55: kernel/condition_variable: Reorder some methods.
This way the static functions (used only in KDL) are below
the actual notification methods, which are now all grouped
together in one place.
No functional change.
37eda488be1c: 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
[ Augustin Cavalier <waddlesplash@xxxxxxxxx> ]
----------------------------------------------------------------------------
2 files changed, 98 insertions(+), 69 deletions(-)
headers/private/kernel/condition_variable.h | 6 +-
src/system/kernel/condition_variable.cpp | 161 ++++++++++++++----------
############################################################################
Commit: 489612d43f55f04e6c36a1ee8d1d224c5768c263
URL: https://git.haiku-os.org/haiku/commit/?id=489612d43f55
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Sat Aug 3 15:12:06 2019 UTC
kernel/condition_variable: Reorder some methods.
This way the static functions (used only in KDL) are below
the actual notification methods, which are now all grouped
together in one place.
No functional change.
----------------------------------------------------------------------------
diff --git a/src/system/kernel/condition_variable.cpp
b/src/system/kernel/condition_variable.cpp
index 85c7a3f87b..b3defeecff 100644
--- a/src/system/kernel/condition_variable.cpp
+++ b/src/system/kernel/condition_variable.cpp
@@ -282,37 +282,6 @@ ConditionVariable::NotifyAll(const void* object, status_t
result)
}
-/*static*/ void
-ConditionVariable::ListAll()
-{
- kprintf(" variable object (type) waiting
threads\n");
-
kprintf("------------------------------------------------------------\n");
- ConditionVariableHash::Iterator it(&sConditionVariableHash);
- while (ConditionVariable* variable = it.Next()) {
- // count waiting threads
- int count = variable->fEntries.Count();
-
- kprintf("%p %p %-20s %15d\n", variable, variable->fObject,
- variable->fObjectType, count);
- }
-}
-
-
-void
-ConditionVariable::Dump() const
-{
- kprintf("condition variable %p\n", this);
- kprintf(" object: %p (%s)\n", fObject, fObjectType);
- kprintf(" threads:");
-
- for (EntryList::ConstIterator it = fEntries.GetIterator();
- ConditionVariableEntry* entry = it.Next();) {
- kprintf(" %" B_PRId32, entry->fThread->id);
- }
- kprintf("\n");
-}
-
-
void
ConditionVariable::_Notify(bool all, status_t result)
{
@@ -355,6 +324,37 @@ ConditionVariable::_NotifyLocked(bool all, status_t result)
}
+/*static*/ void
+ConditionVariable::ListAll()
+{
+ kprintf(" variable object (type) waiting
threads\n");
+
kprintf("------------------------------------------------------------\n");
+ ConditionVariableHash::Iterator it(&sConditionVariableHash);
+ while (ConditionVariable* variable = it.Next()) {
+ // count waiting threads
+ int count = variable->fEntries.Count();
+
+ kprintf("%p %p %-20s %15d\n", variable, variable->fObject,
+ variable->fObjectType, count);
+ }
+}
+
+
+void
+ConditionVariable::Dump() const
+{
+ kprintf("condition variable %p\n", this);
+ kprintf(" object: %p (%s)\n", fObject, fObjectType);
+ kprintf(" threads:");
+
+ for (EntryList::ConstIterator it = fEntries.GetIterator();
+ ConditionVariableEntry* entry = it.Next();) {
+ kprintf(" %" B_PRId32, entry->fThread->id);
+ }
+ kprintf("\n");
+}
+
+
// #pragma mark -
############################################################################
Revision: hrev53335
Commit: 37eda488be1c9fee242e8e4bf6ca644dd13441d8
URL: https://git.haiku-os.org/haiku/commit/?id=37eda488be1c
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Sat Aug 3 15:22:49 2019 UTC
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
----------------------------------------------------------------------------
diff --git a/headers/private/kernel/condition_variable.h
b/headers/private/kernel/condition_variable.h
index 4e91baec22..36b9dc0561 100644
--- a/headers/private/kernel/condition_variable.h
+++ b/headers/private/kernel/condition_variable.h
@@ -1,5 +1,6 @@
/*
* Copyright 2007-2011, Ingo Weinhold, ingo_weinhold@xxxxxx.
+ * Copyright 2019, Haiku, Inc. All rights reserved.
* Distributed under the terms of the MIT License.
*/
#ifndef _KERNEL_CONDITION_VARIABLE_H
@@ -37,9 +38,10 @@ public:
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 @@ protected:
const void* fObject;
const char* fObjectType;
+
+ spinlock fLock;
EntryList fEntries;
ConditionVariable* fNext;
diff --git a/src/system/kernel/condition_variable.cpp
b/src/system/kernel/condition_variable.cpp
index b3defeecff..16ac6b809c 100644
--- a/src/system/kernel/condition_variable.cpp
+++ b/src/system/kernel/condition_variable.cpp
@@ -1,5 +1,6 @@
/*
* Copyright 2007-2011, Ingo Weinhold, ingo_weinhold@xxxxxx.
+ * 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,46 +94,64 @@ 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)
@@ -140,11 +159,15 @@ ConditionVariableEntry::Wait(uint32 flags, bigtime_t
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;
}
