Windows: Improve locking in threading abstraction

Convert the usbi_mutex_t type to a CRITICAL_SECTION object. There
are numerous advantages including lower resource usage and a better
fast-path (doesn't require entering kernel space).

Simplify the condition variable implementation by not associating a
wait structure with a particular thread ID. This is not needed and
causes an unnecessary search through the linked list of any available
wait structures when the real optimization is just reusing an already
created event object.

Also, while here remove all the checks for NULL pointers because we
don't do such a silly thing inside the library.

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>

3 files changed, 34 insertions, 87 deletions

diff --git a/libusb/os/threads_windows.c b/libusb/os/threads_windows.c
index 7c2e52d..7bcb870 100644
--- a/libusb/os/threads_windows.c
+++ b/libusb/os/threads_windows.c
@@ -20,21 +20,17 @@
 
 #include <config.h>
 
-#include <objbase.h>
 #include <errno.h>
 
 #include "libusbi.h"
 
 struct usbi_cond_perthread {
 	struct list_head list;
-	DWORD tid;
 	HANDLE event;
 };
 
 int usbi_mutex_static_lock(usbi_mutex_static_t *mutex)
 {
-	if (!mutex)
-		return EINVAL;
 	while (InterlockedExchange(mutex, 1) == 1)
 		SleepEx(0, TRUE);
 	return 0;
@@ -42,76 +38,44 @@ int usbi_mutex_static_lock(usbi_mutex_static_t *mutex)
 
 int usbi_mutex_static_unlock(usbi_mutex_static_t *mutex)
 {
-	if (!mutex)
-		return EINVAL;
 	InterlockedExchange(mutex, 0);
 	return 0;
 }
 
 int usbi_mutex_init(usbi_mutex_t *mutex)
 {
-	if (!mutex)
-		return EINVAL;
-	*mutex = CreateMutex(NULL, FALSE, NULL);
-	if (!*mutex)
-		return ENOMEM;
+	InitializeCriticalSection(mutex);
 	return 0;
 }
 
 int usbi_mutex_lock(usbi_mutex_t *mutex)
 {
-	DWORD result;
-
-	if (!mutex)
-		return EINVAL;
-	result = WaitForSingleObject(*mutex, INFINITE);
-	if (result == WAIT_OBJECT_0 || result == WAIT_ABANDONED)
-		return 0; // acquired (ToDo: check that abandoned is ok)
-	else
-		return EINVAL; // don't know how this would happen
-			       //   so don't know proper errno
+	EnterCriticalSection(mutex);
+	return 0;
 }
 
 int usbi_mutex_unlock(usbi_mutex_t *mutex)
 {
-	if (!mutex)
-		return EINVAL;
-	if (ReleaseMutex(*mutex))
-		return 0;
-	else
-		return EPERM;
+	LeaveCriticalSection(mutex);
+	return 0;
 }
 
 int usbi_mutex_trylock(usbi_mutex_t *mutex)
 {
-	DWORD result;
-
-	if (!mutex)
-		return EINVAL;
-	result = WaitForSingleObject(*mutex, 0);
-	if (result == WAIT_OBJECT_0 || result == WAIT_ABANDONED)
-		return 0; // acquired (ToDo: check that abandoned is ok)
-	else if (result == WAIT_TIMEOUT)
-		return EBUSY;
+	if (TryEnterCriticalSection(mutex))
+		return 0;
 	else
-		return EINVAL; // don't know how this would happen
-			       //   so don't know proper error
+		return EBUSY;
 }
 
 int usbi_mutex_destroy(usbi_mutex_t *mutex)
 {
-	// It is not clear if CloseHandle failure is due to failure to unlock.
-	//   If so, this should be errno=EBUSY.
-	if (!mutex || !CloseHandle(*mutex))
-		return EINVAL;
-	*mutex = NULL;
+	DeleteCriticalSection(mutex);
 	return 0;
 }
 
 int usbi_cond_init(usbi_cond_t *cond)
 {
-	if (!cond)
-		return EINVAL;
 	list_init(&cond->waiters);
 	list_init(&cond->not_waiting);
 	return 0;
@@ -120,13 +84,11 @@ int usbi_cond_init(usbi_cond_t *cond)
 int usbi_cond_destroy(usbi_cond_t *cond)
 {
 	// This assumes no one is using this anymore.  The check MAY NOT BE safe.
-	struct usbi_cond_perthread *pos, *next_pos;
+	struct usbi_cond_perthread *pos, *next;
 
-	if(!cond)
-		return EINVAL;
 	if (!list_empty(&cond->waiters))
 		return EBUSY; // (!see above!)
-	list_for_each_entry_safe(pos, next_pos, &cond->not_waiting, list, struct usbi_cond_perthread) {
+	list_for_each_entry_safe(pos, next, &cond->not_waiting, list, struct usbi_cond_perthread) {
 		CloseHandle(pos->event);
 		list_del(&pos->list);
 		free(pos);
@@ -139,70 +101,57 @@ int usbi_cond_broadcast(usbi_cond_t *cond)
 	// Assumes mutex is locked; this is not in keeping with POSIX spec, but
 	//   libusb does this anyway, so we simplify by not adding more sync
 	//   primitives to the CV definition!
-	int fail = 0;
 	struct usbi_cond_perthread *pos;
+	int r = 0;
 
-	if (!cond)
-		return EINVAL;
 	list_for_each_entry(pos, &cond->waiters, list, struct usbi_cond_perthread) {
 		if (!SetEvent(pos->event))
-			fail = 1;
+			r = EINVAL;
 	}
 	// The wait function will remove its respective item from the list.
-	return fail ? EINVAL : 0;
+	return r;
 }
 
-__inline static int usbi_cond_intwait(usbi_cond_t *cond,
+static int usbi_cond_intwait(usbi_cond_t *cond,
 	usbi_mutex_t *mutex, DWORD timeout_ms)
 {
 	struct usbi_cond_perthread *pos;
-	int r, found = 0;
-	DWORD r2, tid = GetCurrentThreadId();
+	DWORD r;
 
-	if (!cond || !mutex)
-		return EINVAL;
-	list_for_each_entry(pos, &cond->not_waiting, list, struct usbi_cond_perthread) {
-		if(tid == pos->tid) {
-			found = 1;
-			break;
-		}
-	}
-
-	if (!found) {
-		pos = calloc(1, sizeof(struct usbi_cond_perthread));
-		if (!pos)
+	// Same assumption as usbi_cond_broadcast() holds
+	if (list_empty(&cond->not_waiting)) {
+		pos = malloc(sizeof(*pos));
+		if (pos == NULL)
 			return ENOMEM; // This errno is not POSIX-allowed.
-		pos->tid = tid;
 		pos->event = CreateEvent(NULL, FALSE, FALSE, NULL); // auto-reset.
-		if (!pos->event) {
+		if (pos->event == NULL) {
 			free(pos);
 			return ENOMEM;
 		}
-		list_add(&pos->list, &cond->not_waiting);
+	} else {
+		pos = list_first_entry(&cond->not_waiting, struct usbi_cond_perthread, list);
+		list_del(&pos->list); // remove from not_waiting list.
+		// Ensure the event is clear before waiting
+		WaitForSingleObject(pos->event, 0);
 	}
 
-	list_del(&pos->list); // remove from not_waiting list.
 	list_add(&pos->list, &cond->waiters);
 
-	r  = usbi_mutex_unlock(mutex);
-	if (r)
-		return r;
-
-	r2 = WaitForSingleObject(pos->event, timeout_ms);
-	r = usbi_mutex_lock(mutex);
-	if (r)
-		return r;
+	LeaveCriticalSection(mutex);
+	r = WaitForSingleObject(pos->event, timeout_ms);
+	EnterCriticalSection(mutex);
 
 	list_del(&pos->list);
 	list_add(&pos->list, &cond->not_waiting);
 
-	if (r2 == WAIT_OBJECT_0)
+	if (r == WAIT_OBJECT_0)
 		return 0;
-	else if (r2 == WAIT_TIMEOUT)
+	else if (r == WAIT_TIMEOUT)
 		return ETIMEDOUT;
 	else
 		return EINVAL;
 }
+
 // N.B.: usbi_cond_*wait() can also return ENOMEM, even though pthread_cond_*wait cannot!
 int usbi_cond_wait(usbi_cond_t *cond, usbi_mutex_t *mutex)
 {
@@ -223,8 +172,6 @@ int usbi_cond_timedwait(usbi_cond_t *cond,
 
 int usbi_tls_key_create(usbi_tls_key_t *key)
 {
-	if (!key)
-		return EINVAL;
 	*key = TlsAlloc();
 	if (*key == TLS_OUT_OF_INDEXES)
 		return ENOMEM;
diff --git a/libusb/os/threads_windows.h b/libusb/os/threads_windows.h
index e97ee78..288b3b4 100644
--- a/libusb/os/threads_windows.h
+++ b/libusb/os/threads_windows.h
@@ -24,7 +24,7 @@
 #define usbi_mutex_static_t	volatile LONG
 #define USBI_MUTEX_INITIALIZER	0
 
-#define usbi_mutex_t		HANDLE
+#define usbi_mutex_t		CRITICAL_SECTION
 
 typedef struct usbi_cond {
 	// Every time a thread touches the CV, it winds up in one of these lists.
diff --git a/libusb/os/windows_nt_common.c b/libusb/os/windows_nt_common.c
index d935394..94f8770 100644
--- a/libusb/os/windows_nt_common.c
+++ b/libusb/os/windows_nt_common.c
@@ -123,7 +123,7 @@ typedef struct htab_entry {
 } htab_entry;
 
 static htab_entry *htab_table = NULL;
-static usbi_mutex_t htab_mutex = NULL;
+static usbi_mutex_t htab_mutex;
 static unsigned long htab_filled;
 
 /* Before using the hash table we must allocate memory for it.