Fix to the hashtable for misisng mod of hashsize

Addition of multiple reader/single writer locking mechanism

core/depend.mk

@@ -395,7 +395,8 @@ config.o: config.c /usr/include/stdio.h /usr/include/features.h \
  ../include/service.h ../include/spinlock.h ../include/thread.h \
  /usr/include/pthread.h /usr/include/sched.h /usr/include/bits/sched.h \
  /usr/include/bits/setjmp.h ../include/dcb.h ../include/buffer.h \
- ../include/server.h ../include/users.h ../include/hashtable.h
+ ../include/server.h ../include/users.h ../include/hashtable.h \
+ ../include/atomic.h
 users.o: users.c /usr/include/stdlib.h /usr/include/features.h \
  /usr/include/sys/cdefs.h /usr/include/bits/wordsize.h \
  /usr/include/gnu/stubs.h /usr/include/gnu/stubs-64.h \
@@ -408,7 +409,9 @@ users.o: users.c /usr/include/stdlib.h /usr/include/features.h \
  /usr/include/bits/sigset.h /usr/include/bits/time.h \
  /usr/include/sys/sysmacros.h /usr/include/bits/pthreadtypes.h \
  /usr/include/alloca.h /usr/include/string.h /usr/include/xlocale.h \
- ../include/users.h ../include/hashtable.h ../include/atomic.h
+ ../include/users.h ../include/hashtable.h ../include/spinlock.h \
+ ../include/thread.h /usr/include/pthread.h /usr/include/sched.h \
+ /usr/include/bits/sched.h /usr/include/bits/setjmp.h ../include/atomic.h
 hashtable.o: hashtable.c /usr/include/stdlib.h /usr/include/features.h \
  /usr/include/sys/cdefs.h /usr/include/bits/wordsize.h \
  /usr/include/gnu/stubs.h /usr/include/gnu/stubs-64.h \
@@ -421,4 +424,6 @@ hashtable.o: hashtable.c /usr/include/stdlib.h /usr/include/features.h \
  /usr/include/bits/sigset.h /usr/include/bits/time.h \
  /usr/include/sys/sysmacros.h /usr/include/bits/pthreadtypes.h \
  /usr/include/alloca.h /usr/include/string.h /usr/include/xlocale.h \
- ../include/hashtable.h
+ ../include/hashtable.h ../include/spinlock.h ../include/thread.h \
+ /usr/include/pthread.h /usr/include/sched.h /usr/include/bits/sched.h \
+ /usr/include/bits/setjmp.h ../include/atomic.h

core/hashtable.c

@@ -22,6 +22,29 @@
 /**
  * @file hashtable.c General purpose hashtable routines
  *
+ * The hashtable can be create with a custom number of hash buckets,
+ * a hash function and optional functions to call make copies of the key
+ * and value and to free them.
+ *
+ * The hashtable is arrange as a set of linked lists, the number of linked
+ * lists beign the hashsize as requested by the user. Entries are hashed by
+ * calling the hash function that is passed in by the user, this is used as
+ * an index into the array of linked lists, usign modulo hashsize.
+ *
+ * The linked lists are searched using the key comparison function that is
+ * passed into the hash table creation routine.
+ *
+ * By default the hash table keeps the original pointers that are passed in
+ * for the keys and values, however two functions can be supplied to copy these
+ * a copy function and a free function. Please note the same function is used for
+ * the key and the value, if the actions required are different the called functions
+ * must understand how to differenate the key and value.
+ *
+ * The hash table implements a single write, multiple reader locking policy by
+ * using a pair of counters and a spinlock. The spinlock is used to protect the
+ * number of readers and writers counters when taking out locks. Releasing of
+ * locks uses pure atomic actions and thus does not require spinlock protection.
+ *
  * @verbatim
  * Revision History
  *
@@ -31,9 +54,14 @@
  * @endverbatim
  */
 
+static	void hashtable_read_lock(HASHTABLE *table);
+static	void hashtable_read_unlock(HASHTABLE *table);
+static	void hashtable_write_lock(HASHTABLE *table);
+static	void hashtable_write_unlock(HASHTABLE *table);
+
 /**
  * Special null function used as default memory allfunctions in the hashtable
- * implementation. This avoids havign to special case the code that manipulates
+ * implementation. This avoids having to special case the code that manipulates
  * the keys and values
  *
  * @param	data	The data pointer
@@ -65,6 +93,9 @@ HASHTABLE 	*rval;
 	rval->cmpfn = cmpfn;
 	rval->copyfn = nullfn;
 	rval->freefn = nullfn;
+	rval->n_readers = 0;
+	rval->writelock = 0;
+	spinlock_init(&rval->spin);
 	if ((rval->entries = calloc(size, sizeof(HASHENTRIES))) == NULL)
 	{
 		free(rval);
@@ -86,6 +117,7 @@ hashtable_free(HASHTABLE *table)
 int		i;
 HASHENTRIES	*entry, *ptr;
 
+	hashtable_write_lock(table);
 	for (i = 0; i < table->hashsize; i++)
 	{
 		entry = table->entries[i];
@@ -135,9 +167,10 @@ hashtable_memory_fns(HASHTABLE *table, HASHMEMORYFN copyfn, HASHMEMORYFN freefn)
 int
 hashtable_add(HASHTABLE *table, void *key, void *value)
 {
-int		hashkey = table->hashfn(key);
+int		hashkey = table->hashfn(key) % table->hashsize;
 HASHENTRIES	*entry;
 
+	hashtable_write_lock(table);
 	entry = table->entries[hashkey % table->hashsize];
 	while (entry->next && entry->key && table->cmpfn(key, entry->key) != 0)
 	{
@@ -152,18 +185,23 @@ HASHENTRIES	*entry;
 	else if (table->cmpfn(key, entry->key) == 0)
 	{
 		/* Duplicate key value */
+		hashtable_write_unlock(table);
 		return 0;
 	}
 	else
 	{
 		HASHENTRIES	*ptr = (HASHENTRIES *)malloc(sizeof(HASHENTRIES));
 		if (ptr == NULL)
+		{
+			hashtable_write_unlock(table);
 			return 0;
+		}
 		ptr->key = table->copyfn(key);
 		ptr->value = table->copyfn(value);
 		ptr->next = NULL;
 		entry->next = ptr;
 	}
+	hashtable_write_unlock(table);
 	return 1;
 }
 
@@ -177,9 +215,10 @@ HASHENTRIES	*entry;
 int
 hashtable_delete(HASHTABLE *table, void *key)
 {
-int		hashkey = table->hashfn(key);
+int		hashkey = table->hashfn(key) % table->hashsize;
 HASHENTRIES	*entry, *ptr;
 
+	hashtable_write_lock(table);
 	entry = table->entries[hashkey % table->hashsize];
 	while (entry && entry->key && table->cmpfn(key, entry->key) != 0)
 	{
@@ -188,6 +227,7 @@ HASHENTRIES	*entry, *ptr;
 	if (entry == NULL || entry->key == NULL)
 	{
 		/* Not found */
+		hashtable_write_unlock(table);
 		return 0;
 	}
 
@@ -218,12 +258,16 @@ HASHENTRIES	*entry, *ptr;
 		while (ptr && ptr->next != entry)
 			ptr = ptr->next;
 		if (ptr == NULL)
+		{
+			hashtable_write_unlock(table);
 			return 0;	/* This should never happen */
+		}
 		ptr->next = entry->next;
 		table->freefn(entry->key);
 		table->freefn(entry->value);
 		free(entry);
 	}
+	hashtable_write_unlock(table);
 	return 1;
 }
 
@@ -237,9 +281,10 @@ HASHENTRIES	*entry, *ptr;
 void *
 hashtable_fetch(HASHTABLE *table, void *key)
 {
-int		hashkey = table->hashfn(key);
+int		hashkey = table->hashfn(key) % table->hashsize;
 HASHENTRIES	*entry;
 
+	hashtable_read_lock(table);
 	entry = table->entries[hashkey % table->hashsize];
 	while (entry && entry->key && table->cmpfn(key, entry->key) != 0)
 	{
@@ -247,11 +292,102 @@ HASHENTRIES	*entry;
 	}
 	if (entry == NULL || entry->key == NULL)
 	{
+		hashtable_read_unlock(table);
 		return NULL;
 	}
 	else
 	{
+		hashtable_read_unlock(table);
 		return entry->value;
 	}
 }
 
+
+/**
+ * Take a read lock on the hashtable.
+ *
+ * The hashtable support multiple readers and a single writer,
+ * we have a spinlock to protect the two counts, n_readers and
+ * writelock.
+ *
+ * We take the hashtable spinlock and then check that writelock
+ * is set to zero. If not we release the spinlock and do dirty
+ * reads of writelock until it goes to 0. Once it is zero we
+ * acquire the spinlock again and test that writelock is still
+ * 0.
+ *
+ * With writelock set to zero we increment n_readers with the
+ * spinlock still held.
+ *
+ * @param table		The hashtable to lock.
+ */
+static void
+hashtable_read_lock(HASHTABLE *table)
+{
+	spinlock_acquire(&table->spin);
+	while (table->writelock)
+	{
+		spinlock_release(&table->spin);
+		while (table->writelock)
+			;
+		spinlock_acquire(&table->spin);
+	}
+	table->n_readers++;
+	spinlock_release(&table->spin);
+}
+
+/**
+ * Release a previously obtained readlock.
+ *
+ * Simply decrement the n_readers value for the hash table
+ *
+ * @param table		The hash table to unlock
+ */
+static void
+hashtable_read_unlock(HASHTABLE *table)
+{
+	atomic_add(&table->n_readers, -1);
+}
+
+/**
+ * Obtain an exclusive write lock for the hash table.
+ *
+ * We acquire the hashtable spinlock, check for the number of
+ * readers beign zero. If it is not we hold the spinlock and
+ * loop waiting for the n_readers to reach zero. This will prevent
+ * any new readers beign granted access but will not prevent current
+ * readers releasing the read lock.
+ *
+ * Once we have no readers we increment writelock and test if we are
+ * the only writelock holder, if not we repeat the process. We hold
+ * the spinlock throughout the process since both read and write
+ * locks do not require the spinlock to be acquired.
+ *
+ * @param table	The table to lock for updates
+ */
+static void
+hashtable_write_lock(HASHTABLE *table)
+{
+int	available;
+
+	spinlock_acquire(&table->spin);
+	do {
+		while (table->n_readers)
+			;
+		available = atomic_add(&table->writelock, 1);
+		if (available != 0)
+			atomic_add(&table->writelock, -1);
+	} while (available != 0);
+	spinlock_release(&table->spin);
+}
+
+/**
+ * Release the write lock on the hash table.
+ *
+ * @param table The hash table to unlock
+ */
+static void
+hashtable_write_unlock(HASHTABLE *table)
+{
+	atomic_add(&table->writelock, -1);
+}

include/hashtable.h

@@ -30,6 +30,8 @@
  *
  * @endverbatim
  */
+#include <spinlock.h>
+#include <atomic.h>
 
 /**
  * The entries within a hashtable.
@@ -58,6 +60,9 @@ typedef struct hashtable {
 	int		(*cmpfn)(void *, void *);	/**< The key comparison function */
 	HASHMEMORYFN	copyfn;				/**< Optional copy function */
 	HASHMEMORYFN	freefn;				/**< Optional free function */
+	SPINLOCK	spin;				/**< Internal spinlock for the hashtable */
+	int		n_readers;			/**< Number of clients reading the table */
+	int		writelock;			/**< The table is locked by a writer */
 } HASHTABLE;
 
 extern HASHTABLE	*hashtable_alloc(int, int (*hashfn)(), int (*cmpfn)());

include/spinlock.h

@@ -31,7 +31,7 @@
 #include <thread.h>
 
 typedef struct spinlock {
-	int		lock;
+	volatile int	lock;
 #if DEBUG
 	int		spins;
 	int		acquired;

include/users.h

@@ -18,6 +18,7 @@
  * Copyright SkySQL Ab 2013
  */
 #include <hashtable.h>
+
 /**
  * @file users.h The functions to manipulate the table of users maintained
  * for each service
@@ -40,6 +41,7 @@ typedef struct {
 	int	n_deletes;		/**< The number of deletes */
 	int	n_fetches;		/**< The number of fetchs */
 } USERS_STATS;
+
 /**
  * The user table, this contains the username and authentication data required
  * for the authentication implementation within the gateway.