Commit 438cbecc authored by gejun's avatar gejun

polish comments in butex

parent f2f51143
...@@ -179,7 +179,7 @@ inline int unsleep_if_necessary(ButexBthreadWaiter* w, ...@@ -179,7 +179,7 @@ inline int unsleep_if_necessary(ButexBthreadWaiter* w,
return 0; return 0;
} }
// Using ObjectPool(which never frees memory) to solve the race between // Use ObjectPool(which never frees memory) to solve the race between
// butex_wake() and butex_destroy(). The race is as follows: // butex_wake() and butex_destroy(). The race is as follows:
// //
// class Event { // class Event {
...@@ -212,30 +212,29 @@ inline int unsleep_if_necessary(ButexBthreadWaiter* w, ...@@ -212,30 +212,29 @@ inline int unsleep_if_necessary(ButexBthreadWaiter* w,
// event.wait(); // event.wait();
// } <-- event destroyed // } <-- event destroyed
// //
// Summary: Thread1 passes a stateful condition to thread2 and wait until // Summary: Thread1 passes a stateful condition to Thread2 and waits until
// the the condition is signalled, which basically means the associated // the condition being signalled, which basically means the associated
// job is done and Thread1 can release related resources including the mutex // job is done and Thread1 can release related resources including the mutex
// and condition. The scenario is fine and code is correct. // and condition. The scenario is fine and the code is correct.
// The race needs a closer look. If we look into the unlock at /*1*/, it // The race needs a closer look. The unlock at /*1*/ may have different
// may have different implementations, but the last step is probably an // implementations, but in which the last step is probably an atomic store
// atomic store and butex_wake(), like this: // and butex_wake(), like this:
// //
// locked->store(0); // locked->store(0);
// butex_wake(locked); // butex_wake(locked);
// //
// `locked' represents the locking status of the mutex. The issue is that // The `locked' represents the locking status of the mutex. The issue is that
// just after the store(), the mutex is already unlocked, the code in // just after the store(), the mutex is already unlocked and the code in
// Event.wait() may successfully grab the lock and go through everything // Event.wait() may successfully grab the lock and go through everything
// left and leave foo() function, destroying the mutex and butex, thus the // left and leave foo() function, destroying the mutex and butex, making
// butex_wake(locked) may crash. // the butex_wake(locked) crash.
// To solve this issue, one method is to add reference before store and // To solve this issue, one method is to add reference before store and
// release the reference after butex_wake. However this kind of // release the reference after butex_wake. However reference countings need
// reference-counting needs to be added in nearly every user scenario of // to be added in nearly every user scenario of butex_wake(), which is very
// butex_wake(), which is very error-prone. Another method is never freeing // error-prone. Another method is never freeing butex, with the side effect
// butex, the side effect is that butex_wake() may wake up an unrelated // that butex_wake() may wake up an unrelated butex(the one reuses the memory)
// butex(the one reuses the memory) and cause spurious wakeups. According // and cause spurious wakeups. According to our observations, the race is
// to our observations, the race is infrequent, even rare. The extra spurious // infrequent, even rare. The extra spurious wakeups should be acceptable.
// wakeup should be acceptable.
void* butex_create() { void* butex_create() {
Butex* b = butil::get_object<Butex>(); Butex* b = butil::get_object<Butex>();
...@@ -493,7 +492,7 @@ static void wait_for_butex(void* arg) { ...@@ -493,7 +492,7 @@ static void wait_for_butex(void* arg) {
ButexBthreadWaiter* const bw = static_cast<ButexBthreadWaiter*>(arg); ButexBthreadWaiter* const bw = static_cast<ButexBthreadWaiter*>(arg);
Butex* const b = bw->initial_butex; Butex* const b = bw->initial_butex;
// 1: waiter with timeout should have waiter_state == WAITER_STATE_READY // 1: waiter with timeout should have waiter_state == WAITER_STATE_READY
// before they're are queued, otherwise the waiter is already timedout // before they're queued, otherwise the waiter is already timedout
// and removed by TimerThread, in which case we should stop queueing. // and removed by TimerThread, in which case we should stop queueing.
// //
// Visibility of waiter_state: // Visibility of waiter_state:
...@@ -503,9 +502,9 @@ static void wait_for_butex(void* arg) { ...@@ -503,9 +502,9 @@ static void wait_for_butex(void* arg) {
// tt_lock { get task } // tt_lock { get task }
// waiter_lock { waiter_state=TIMEDOUT } // waiter_lock { waiter_state=TIMEDOUT }
// waiter_lock { use waiter_state } // waiter_lock { use waiter_state }
// tt_lock represents TimerThread::_mutex. Obviously visibility of // tt_lock represents TimerThread::_mutex. Visibility of waiter_state is
// waiter_state are sequenced by two locks, both threads are guaranteed to // sequenced by two locks, both threads are guaranteed to see the correct
// see the correct value. // value.
{ {
BAIDU_SCOPED_LOCK(b->waiter_lock); BAIDU_SCOPED_LOCK(b->waiter_lock);
if (b->value.load(butil::memory_order_relaxed) != bw->expected_value) { if (b->value.load(butil::memory_order_relaxed) != bw->expected_value) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment