Revision

Revise code to avoid warnings.
Add new test routine to test fairness.
Also mordify some comments for clearance.

src/Makefile

@@ -5,10 +5,10 @@ P4:
 	cc -lcounter -llist -lhash -L. -o P4 main.c libcounter.so liblist.so libhash.so -lpthread
 
 libcounter.so:
-	cc -shared -fPIC counter.c lock.h lock.c -o libcounter.so
+	cc -shared -fPIC counter.c lock.h lock.c -o libcounter.so -Wall -Werror
 
 liblist.so:
-	cc -shared -fPIC list.c lock.h lock.c -o liblist.so
+	cc -shared -fPIC list.c lock.h lock.c -o liblist.so -Wall -Werror
 
 libhash.so:
-	cc -shared -fPIC hash.c list.h list.c lock.h lock.c -o libhash.so
+	cc -shared -fPIC hash.c list.h list.c lock.h lock.c -o libhash.so -Wall -Werror

src/counter.c

@@ -2,7 +2,7 @@
 
 /**
  * Initialization part of counter
- * @param c pointer to a counter
+ * @param c Pointer to a counter
  * @param value Initial value of the counter
  */
 void counter_init(counter_t *c, int value) {
@@ -12,8 +12,8 @@ void counter_init(counter_t *c, int value) {
 
 /**
  * Get the value of the counter
- * @param c pointer to a counter
- * @return the current value of the counter
+ * @param c Pointer to a counter
+ * @return The current value of the counter
  */
 int counter_get_value(counter_t *c) {
 #if defined(LOCK_RWLOCK)
@@ -28,7 +28,7 @@ int counter_get_value(counter_t *c) {
 
 /**
  * Increase the counter by 1
- * @param c pointer to a counter
+ * @param c Pointer to a counter
  */
 void counter_increment(counter_t *c) {
 #if defined(LOCK_RWLOCK)
@@ -42,7 +42,7 @@ void counter_increment(counter_t *c) {
 
 /**
  * Decrease the counter by 1
- * @param c pointer to a counter
+ * @param c Pointer to a counter
  */
 void counter_decrement(counter_t *c) {
 #if defined(LOCK_RWLOCK)

src/hash.c

@@ -3,7 +3,7 @@
 /**
  * Initialize the hash table with given bucket size
  * @param hash A pointer to hash table
- * @param size designated bucket size
+ * @param size Designated bucket size
  */
 void hash_init(hash_t *hash, int size) {
     int i;
@@ -51,7 +51,7 @@ void* hash_lookup(hash_t *hash, unsigned int key) {
  * Remove an given hash table and free the pointer of it
  * @param hash The pointer to hash table
  */
-void* hash_destroy(hash_t *hash) {
+void hash_destroy(hash_t *hash) {
     int i;
     for (i = 0; i < hash->bucket_size; i++) {
         list_destroy(&hash->lists[i]);

src/hash.h

@@ -18,6 +18,6 @@ void hash_init(hash_t *hash, int size);
 void hash_insert(hash_t *hash, unsigned int key);
 void hash_delete(hash_t *hash, unsigned int key);
 void *hash_lookup(hash_t *hash, unsigned int key);
-void* hash_destroy(hash_t *hash);
+void hash_destroy(hash_t *hash);
 
 #endif //P4_HASH_H

src/list.c

@@ -12,7 +12,7 @@ void list_init(list_t *list) {
 /**
  * Insert a new node with value key at the head of the list
  * @param list A pointer to a list
- * @param key the value to be inserted
+ * @param key The value to be inserted
  */
 void list_insert(list_t *list, unsigned int key) {
     node_t *new_node = malloc(sizeof(node_t));

src/lock.c

@@ -16,6 +16,8 @@
  * @param op Options that specify the operation to be performed
  * @param val1 A value whose meaning and purpose depends on op
  * @param timeout A pointer to a timespec structure that specifies a timeout for the operation.
+ * For some operations, the least significant four bytes of this argument are instead used as an integer
+ * whose meaning is determined by the operation
  * @param addr2 A pointer to a second futex word that is employed by the operation
  * @param val3 A value whose meaning and purpose depends on op
  * @return
@@ -33,9 +35,9 @@ static inline void cpu_pause(void) {
 }
 
 /**
- * Using embedded assembly instruction "xchg"
- * This instruction implements atomic operation
- * Exchange values from the pointer addr to the newval
+ * Calling embedded assembly instruction "xchg"
+ * This instruction completes atomic operation of
+ * exchanging values from the pointer addr to the newval
  * @param addr A pointer to the mutex value
  * @param newval The new value to be swaped
  * @return The previous value of the pointer addr referenced
@@ -49,21 +51,21 @@ static inline unsigned xchg(void *addr, unsigned newval) {
 }
 
 /**
- * Using embedded assembly instruction "cmpxchg"
- * This instruction implements atomic operation
- * Compare the value of the pointer addr pointed with oldval
+ * Calling embedded assembly instruction "cmpxchg"
+ * This instruction completes atomic operation of
+ * comparing the value of the pointer addr pointed with oldval,
  * if equal, put newval into the position of the addr pointer and return to oldval
  * return to the value pointed by addr instead
  * @param addr A pointer to the mutex value
  * @param oldval A value to be compared
  * @param newval A value to write into addr
  * @return oldval or *addr
  */
-static inline uint cmpxchg(volatile unsigned int *addr, unsigned int oldval, unsigned int newval) {
+static inline uint cmpxchg(void *addr, unsigned int oldval, unsigned int newval) {
     uint ret;
     asm volatile("lock; cmpxchgl %1, %2"
     : "=a" (ret)
-    : "r" (newval), "m" (*addr), "0" (oldval)
+    : "r" (newval), "m" (*(volatile unsigned *)addr), "0" (oldval)
     : "memory");
     return ret;
 }
@@ -118,7 +120,7 @@ void mutex_acquire(mutex_t *lock) {
 }
 
 /**
- * Release the given mutex and wake up threads in waiting condition
+ * Release the given mutex and wake up a thread in waiting condition
  * Notice that release a lock not hold by itself will cause unpredictable result
  * @param lock Pointer to the spin-lock want to release
  */
@@ -136,7 +138,7 @@ void twophase_init(twophase_t *lock) {
 }
 
 /**
- * Try to acquire the given two-phase lock in the loop
+ * Acquire the given two-phase in two phases
  * Wait for some one to release first, the wait time is designated by the LOOP_MAX macro (defined below)
  * If the lock still acquired by someone else after wait phase, it will sleep until someone release the lock
  * lock = 0 means unlock state;
@@ -165,7 +167,7 @@ void twophase_acquire(twophase_t *lock) {
 }
 
 /**
- * Release the given two-phase lock
+ * Release the given two-phase lock in two phases
  * In the first phase it will try to give the lock to someone awake
  * If failed, it will wake up a sleeping thread on this lock
  * @param lock Pointer to the two-phase lock want to release
@@ -211,11 +213,13 @@ void cond_wait(cond_t* cv, twophase_t* mutex) {
     int old_seq = cv->seq;
 
     if (cv->mutex != mutex) {
-        if (cv->mutex != NULL) { // TODO: ERROR
+        if (cv->mutex != NULL) {
+            perror("cond mutex already exists!");
             return;
         }
-        cmpxchg(&cv->mutex, NULL, mutex);
-        if (cv->mutex != mutex) { // TODO: ERROR
+        cmpxchg(&cv->mutex, 0, (unsigned)(unsigned long)mutex);
+        if (cv->mutex != mutex) {
+            perror("cond mutex incompatible!");
             return;
         }
     }
@@ -249,7 +253,7 @@ void cond_broadcast(cond_t* cv) {
     }
     atomic_add(&cv->seq, 1);
 
-    // wake up 1 waiting thread, requeue other threads on mutex to avoid thundering herd
+    // wake up 1 waiting thread, requeue other threads on mutex to avoid thundering herd effect
     sys_futex(&cv->seq, FUTEX_REQUEUE_PRIVATE, 1, (void*) 0x0FFFFFFF, old_mutex, 0);	// *((int*) 0x0FFFFFFF)
 }
 

src/main.c

@@ -110,6 +110,25 @@ void* test5(void* args) {
     return NULL;
 }
 
+long long timeTotal[THREAD_COUNT];
+int opCouunt[THREAD_COUNT];
+
+void* test6(void* args) {
+    int i;
+    int id = (int)args;
+    timeTotal[id] = opCouunt[id] = 0;
+    struct timeval tvb, tve;
+    for (i = 0; i < MAX_N; i++) {
+        gettimeofday(&tvb, NULL);
+        counter_get_value(&counter);
+        gettimeofday(&tve, NULL);
+        timeTotal[id] += tve.tv_usec - tvb.tv_usec;
+        opCouunt[id]++;
+    }
+    return NULL;
+}
+
+// test correctness of counter
 void main1() {
     int i;
     counter_init(&counter, 0);
@@ -125,6 +144,7 @@ void main1() {
     printf("Counter value: %d\n", counter_get_value(&counter));
 }
 
+// test performance of cv
 void main2() {
     int i;
     test_var = 0;
@@ -144,6 +164,7 @@ void main2() {
     printf("Test var: %d\n", test_var);
 }
 
+// test performance of list
 void main3() {
     int i;
     list_init(&list);
@@ -163,6 +184,7 @@ void main3() {
     printf("List sum: %lld vs %lld\n", list_sum(&list), glob_sum);
 }
 
+// test performance of counter
 void main4() {
     int i;
     counter_init(&counter, 0);
@@ -177,7 +199,26 @@ void main4() {
     printf("Time: %f\n", endTimer());
 }
 
+// test fairness
+void main5() {
+    int i;
+    counter_init(&counter, 0);
+    pthread_t* threads = malloc(sizeof(pthread_t)*THREAD_COUNT);
+    for (i = 0; i < THREAD_COUNT; i++) {
+        pthread_create(&threads[i], NULL, test6, (void*)i);
+    }
+    for (i = 0; i < THREAD_COUNT; i++) {
+        pthread_join(threads[i], NULL);
+    }
+
+    printf("Average lock acquire time:\n");
+    for (i = 0; i < THREAD_COUNT; i++) {
+        printf("%f ", 1.0 * timeTotal[i] / opCouunt[i]);
+    }
+    printf("\n");
+}
+
 int main() {
-    main3();
+    main5();
     return 0;
 }