Docs: Qutex.md: update

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<bool> and a std::list.
+combination of a SpinLock, a sh_ptr<LockerAndInvoker> 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(
+			int indexOfItemToInsertCurrFrontBehind = min(
-				queue.size(),
+				nQItems - 1,
-				failedAcquirer.serializedContinuation.requiredLocks.size());
+				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
+			 * The reason why we do this rotation is to avoid a particular kind
-			 * deadlock wherein a grid of async requests is perfectly configured
+			 * of deadlock wherein a grid of async requests is perfectly
-			 * so as to guarantee that none of them can make any forward
+			 * configured so as to guarantee that none of them can make any
-			 * progress unless they get reordered.
+			 * 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:
+		/** EXPLANATION:
-		 * I am not sure whether we should only wake up the front item if
+		 * It would be nice to be able to optimize by only awakening if the
-		 * the prev owner was the previous front item; or whether we should
+		 * release()ing lockvoker was at the front of the qutexQ, but if we
-		 * always wake up the new front item.
+		 * don't unconditionally wakeup() the front item, we could get lost
+		 * wakeups. Consider:
 		 *
-		 * Recall that because a sequence can acquire a Qutex without being
+		 *	Lv1 only has 1 requiredLock.
-		 * at the front of the queue (because it could merely be in the top X%
+		 *	Lv2 has 3 requiredLocks. One of its requiredLocks overlaps with
-		 * of items instead), this means that during a call to release(), the
+		 *		Lv1's single requiredLock. So they both share a common lock.
-		 * owning async sequence may not be at the front -- it needs only be in
+		 *	Lv3's currently owns Lv1 & Lv2's common requiredLock.
-		 * the top X% of items.
+		 *	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
+		 * Thus, Lv1 never gets awakened again. The end.
-		 * the new front item after removing the previous owner.
+		 * 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
+		 * Therefore we must always awaken the front item when releas()ing.
-		 * 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.
 		 */
-		LockerAndInvoker &newFront = queue.front();
+		LockerAndInvoker &front = queue.front();
 
 		lock.release();
 
-#ifndef CONFIG_LOCKVOKER_AGGRESSIVE_WAKEUPS
+		wakeUp(front);
-		if (&newFront != &oldFront)
-#endif
-			{ wakeUp(newFront); }
 	}
 
 public: