diff --git a/docs/design/qutexes.md b/docs/design/qutexes.md index 41722d6..0648d11 100644 --- a/docs/design/qutexes.md +++ b/docs/design/qutexes.md @@ -238,7 +238,7 @@ LockSet::release() ``` Now, the Qutex class is what we'll use for synchronization. It's just a -combination of 2 atomic and a std::list. +combination of a SpinLock, a sh_ptr and a std::list. ``` class SpinLock @@ -389,7 +389,7 @@ public: { if (*rIt != tryingLockvoker) { continue; } - foundInRear == true; + foundInRear = true; break; } @@ -410,9 +410,10 @@ public: { lock.acquire(); + const int nQItems = queue.size(); // Rotate queue members if failedAcquirer is at front of queue. LockerAndInvoker &currFront = queue.front(); - if (currFront == failedAcquirer) + if (currFront == failedAcquirer && nQItems > 1) { /** EXPLANATION: * Rotate the top LockSet.size() items in the queue by moving @@ -430,17 +431,17 @@ public: * acquire the Qutex. Being the only item in the ticketQ * means that you must succeed at acquiring the Qutex. */ - int swapPosition = min( - queue.size(), - failedAcquirer.serializedContinuation.requiredLocks.size()); + int indexOfItemToInsertCurrFrontBehind = min( + nQItems - 1, + failedAcquirer.serializedContinuation.requiredLocks.size() - 1); /* EXPLANATION: - * Swap them here. + * Rotate them here. * - * The reason why we do this swap is to avoid a particular kind of - * deadlock wherein a grid of async requests is perfectly configured - * so as to guarantee that none of them can make any forward - * progress unless they get reordered. + * The reason why we do this rotation is to avoid a particular kind + * of deadlock wherein a grid of async requests is perfectly + * configured so as to guarantee that none of them can make any + * forward progress unless they get reordered. * * Consider 2 different locks with 2 different items in them * each, both of which come from 2 particular requests: @@ -480,6 +481,11 @@ public: * in the queue if the current front item is the failed acquirer. * So that's why we do this rotation here. */ + // The first arg (the iterator) is a ref in case it must be updated. + rotate( + currFront.serializedContinuation.requiredLocks.getLockDesc( + *this).second, + indexOfItemToInsertCurrFrontBehind); } currOwner.release(); @@ -488,17 +494,42 @@ public: lock.release(); - wakeUp(newFront); + /** EXPLANATION: + * We should always awaken whoever is at the front of the queue, even if + * we didn't rotate. Why? Consider this scenario: + * + * Lv1 has LockSet.size==1. Lv2 has LockSet.size==3. + * Lv1's required lock overlaps with Lv2's set of 3 required locks. + * Lv1 registers itself in its 1 qutex's queue. + * Lv2 registers itself in all 3 of its qutexes' queues. + * Lv2 acquires the lock that it needs in common with Lv1. + * (Assume that Lv2 was not at the front of the common qutex's + * internal queue -- it only needed to be in the top 66%.) + * Lv1 tries to acquire the common lock and fails. It gets taken off of + * its io_service. It's now asleep until it gets + * re-added into an io_service. + * Lv2 fails to acquire the other 2 locks it needs and backoff()s from + * the common lock it shares with Lv1. + * + * If Lv2 does NOT awaken the item at the front of the common lock's + * queue (aka: Lv1), then Lv1 is doomed to never wake up again. + * + * Hence: backout() callers should always wake up the lockvoker at the + * front of their queue before leaving. + * + * The exception is if the item at the front is the backout() caller + * itself. This can happen if, for example a multi-locking lockvoker + * is backing off of a qutex within which it's the only waiter. + */ + if (newFront != failedAcquirer) { + wakeUp(newFront); + } } void release() { lock.acquire(); -#ifndef CONFIG_LOCKVOKER_AGGRESSIVE_WAKEUPS - LockerAndInvoker &oldFront = queue.front(); -#endif - /* Get the saved iterator and use it to unregister. * Don't acquire lock because we already acquired it in this function. */ @@ -507,53 +538,40 @@ public: currOwner.release(); - /** NOTE: - * I am not sure whether we should only wake up the front item if - * the prev owner was the previous front item; or whether we should - * always wake up the new front item. + /** EXPLANATION: + * It would be nice to be able to optimize by only awakening if the + * release()ing lockvoker was at the front of the qutexQ, but if we + * don't unconditionally wakeup() the front item, we could get lost + * wakeups. Consider: * - * Recall that because a sequence can acquire a Qutex without being - * at the front of the queue (because it could merely be in the top X% - * of items instead), this means that during a call to release(), the - * owning async sequence may not be at the front -- it needs only be in - * the top X% of items. + * Lv1 only has 1 requiredLock. + * Lv2 has 3 requiredLocks. One of its requiredLocks overlaps with + * Lv1's single requiredLock. So they both share a common lock. + * Lv3's currently owns Lv1 & Lv2's common requiredLock. + * Lv3 release()s that common lock. + * Lv1 happens to be next in queue after Lv3 unregisters itself. + * Lv3 wakes up Lv1. + * Just before Lv1 can acquire the common lock, Lv2 acquires it now, + * because it only needs to be in the top 66% to succeed. + * Lv1 checks the currOwner and sees that it's owned. Lv1 is now + * dequeued from its io_service. It won't be awakened until someone + * awakens it. + * Lv2 finishes its critical section and releas()es the common lock. + * Lv2 was not at the front of the qutexQ, so it does NOT awaken the + * current item at the front. * - * When the owning sequence is the front, then we should definitely wake - * the new front item after removing the previous owner. + * Thus, Lv1 never gets awakened again. The end. + * This also means that no LockSet.size()==1 lockvoker will ever be able + * to run again since they can only run if they are at the front of the + * qutexQ. * - * But when the front item is not the prev owner, then should we wake it - * up? It should have been awakened previously, so if it's still at the - * front, that implies that it failed to make forward progress last time - * it awoke. You could argue that during the interim while this owner - * did its thing, it's possible for the current front item to have - * become capable of acquiring all of its requiredLocks, due to changes - * in the other locks in its requiredLocks set. This is a fair argument. - * - * You could also argue that we can just wait until its registered - * items in its other locks' ticketQs reach the front of those other - * ticketQs, and then when that happens, those locks will wake it up - * when release() is called. - * - * The latter eases contention since one could argue that we have a - * surer chance of it successfully acquiring all of its requiredLocks if - * we wait until it bubbles to the front of another ticketQ. Whereas, - * if we aggressively/greedily awaken it to try just because an item in - * another ticketQ has been removed, we're just introducing contention. - * Both cases have good arguments. The aggressive approach enables us to - * potentially retire more requests, and thus increase throughput. - * - * This current pseudocode assumes the latter and waits for the other - * locks' ticketQs to wake it up when it reaches the top in their - * Qs. + * Therefore we must always awaken the front item when releas()ing. */ - LockerAndInvoker &newFront = queue.front(); + LockerAndInvoker &front = queue.front(); lock.release(); -#ifndef CONFIG_LOCKVOKER_AGGRESSIVE_WAKEUPS - if (&newFront != &oldFront) -#endif - { wakeUp(newFront); } + wakeUp(front); } public: