From b71e631972d3563641f621a2b0f58def53e334c6 Mon Sep 17 00:00:00 2001
From: Ingo Weinhold <ingo_weinhold@gmx.de>
Date: Wed, 23 Jul 2008 22:56:51 +0000
Subject: [PATCH] * Fixed race condition between OperationCompleted() and the
 scheduler   thread. Interrupting a thread only works when it is already
 waiting.   We do now use a flag to indicate whether the scheduler thread is  
 waiting (avoids thread_interrupt() calls when the thread is in driver  
 code). Furthermore before starting to wait, we check whether any   finisher
 work has to be done -- we do that (and the addition of the   entry to the
 condition variable) with the finisher lock being held to   avoid the race
 condition. * Moved waiting for and getting the next unscheduled request into
 new   method _GetNextUnscheduledRequest().

git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@26594 a95241bf-73f2-0310-859d-f6bbb57e9c96
---
 .../kernel/device_manager/IOScheduler.cpp     | 88 ++++++++++++++-----
 .../kernel/device_manager/IOScheduler.h       |  4 +
 2 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/src/system/kernel/device_manager/IOScheduler.cpp b/src/system/kernel/device_manager/IOScheduler.cpp
index 46a4a562fb..8daeb1cdd7 100644
--- a/src/system/kernel/device_manager/IOScheduler.cpp
+++ b/src/system/kernel/device_manager/IOScheduler.cpp
@@ -22,7 +22,8 @@
 
 IOScheduler::IOScheduler(DMAResource* resource)
 	:
-	fDMAResource(resource)
+	fDMAResource(resource),
+	fWaiting(false)
 {
 	mutex_init(&fLock, "I/O scheduler");
 	B_INITIALIZE_SPINLOCK(&fFinisherLock);
@@ -98,8 +99,7 @@ IOScheduler::AbortRequest(IORequest* request, status_t status)
 void
 IOScheduler::OperationCompleted(IOOperation* operation, status_t status)
 {
-	InterruptsLocker _;
-	SpinLocker locker(fFinisherLock);
+	InterruptsSpinLocker _(fFinisherLock);
 
 	// finish operation only once
 	if (operation->Status() <= 0)
@@ -108,10 +108,11 @@ IOScheduler::OperationCompleted(IOOperation* operation, status_t status)
 	operation->SetStatus(status);
 
 	fCompletedOperations.Add(operation);
-	locker.Unlock();
 
-	locker.SetTo(thread_spinlock, false);
-	thread_interrupt(thread_get_thread_struct_locked(fThread), false);
+	if (fWaiting) {
+		SpinLocker _2(thread_spinlock);
+		thread_interrupt(thread_get_thread_struct_locked(fThread), false);
+	}
 }
 
 
@@ -143,6 +144,15 @@ IOScheduler::_Finisher()
 }
 
 
+/*!	Called with \c fFinisherLock held.
+*/
+bool
+IOScheduler::_FinisherWorkPending()
+{
+	return !fCompletedOperations.IsEmpty();
+}
+
+
 IOOperation*
 IOScheduler::_GetOperation()
 {
@@ -153,12 +163,60 @@ IOScheduler::_GetOperation()
 		if (operation != NULL)
 			return operation;
 
+		// Wait for new operations. First check whether any finisher work has
+		// to be done.
+		InterruptsSpinLocker finisherLocker(fFinisherLock);
+		if (_FinisherWorkPending()) {
+			finisherLocker.Unlock();
+			locker.Unlock();
+			_Finisher();
+			continue;
+		}
+
 		ConditionVariableEntry entry;
 		fFinishedOperationCondition.Add(&entry);
+		fWaiting = true;
 
+		finisherLocker.Unlock();
 		locker.Unlock();
 
-		entry.Wait();
+		entry.Wait(B_CAN_INTERRUPT);
+		fWaiting = false;
+		_Finisher();
+	}
+}
+
+
+IORequest*
+IOScheduler::_GetNextUnscheduledRequest()
+{
+	while (true) {
+		MutexLocker locker(fLock);
+		IORequest* request = fUnscheduledRequests.RemoveHead();
+
+		if (request != NULL)
+			return request;
+
+		// Wait for new requests. First check whether any finisher work has
+		// to be done.
+		InterruptsSpinLocker finisherLocker(fFinisherLock);
+		if (_FinisherWorkPending()) {
+			finisherLocker.Unlock();
+			locker.Unlock();
+			_Finisher();
+			continue;
+		}
+
+		// Wait for new requests.
+		ConditionVariableEntry entry;
+		fNewRequestCondition.Add(&entry);
+		fWaiting = true;
+
+		finisherLocker.Unlock();
+		locker.Unlock();
+
+		entry.Wait(B_CAN_INTERRUPT);
+		fWaiting = false;
 		_Finisher();
 	}
 }
@@ -169,21 +227,7 @@ IOScheduler::_Scheduler()
 {
 // TODO: This is a no-op scheduler. Implement something useful!
 	while (true) {
-		MutexLocker locker(fLock);
-		IORequest* request = fUnscheduledRequests.RemoveHead();
-
-		if (request == NULL) {
-			ConditionVariableEntry entry;
-			fNewRequestCondition.Add(&entry);
-			locker.Unlock();
-
-			if (entry.Wait(B_CAN_INTERRUPT) != B_OK)
-				_Finisher();
-
-			continue;
-		}
-
-		locker.Unlock();
+		IORequest* request = _GetNextUnscheduledRequest();
 
 		if (fDMAResource != NULL) {
 			while (request->RemainingBytes() > 0) {
diff --git a/src/system/kernel/device_manager/IOScheduler.h b/src/system/kernel/device_manager/IOScheduler.h
index 657dd874c1..9287d2abd4 100644
--- a/src/system/kernel/device_manager/IOScheduler.h
+++ b/src/system/kernel/device_manager/IOScheduler.h
@@ -46,10 +46,13 @@ public:
 
 private:
 			void				_Finisher();
+			bool				_FinisherWorkPending();
 			IOOperation*		_GetOperation();
+			IORequest*			_GetNextUnscheduledRequest();
 			status_t			_Scheduler();
 	static	status_t			_SchedulerThread(void* self);
 
+
 private:
 			DMAResource*		fDMAResource;
 			spinlock			fFinisherLock;
@@ -62,6 +65,7 @@ private:
 			ConditionVariable	fFinishedOperationCondition;
 			IOOperationList		fUnusedOperations;
 			IOOperationList		fCompletedOperations;
+			bool				fWaiting;
 };
 
 #endif	// IO_SCHEDULER_H