[PATCH] thread_pthread: eliminate malloc from signal thread list

[PATCH] thread_pthread: eliminate malloc from signal thread list

[Eric Wong]

ccan/list may be used and the list nodes are embedded directly
into the rb_thread_t structure.

Note: double-checked locking from FGLOCK is still possible, but I am
not convinced it is necessary with modern pthreads implementations.
---
 thread_pthread.c | 90 ++++++++++++++++----------------------------------------
 thread_pthread.h |  9 +++++-
 2 files changed, 34 insertions(+), 65 deletions(-)

diff --git a/thread_pthread.c b/thread_pthread.c
index 26fe9ff..4eca11d 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -1122,15 +1122,7 @@ native_sleep(rb_thread_t *th, struct timeval *timeout_tv)
 }
 
 #ifdef USE_SIGNAL_THREAD_LIST
-struct signal_thread_list {
-    rb_thread_t *th;
-    struct signal_thread_list *prev;
-    struct signal_thread_list *next;
-};
-
-static struct signal_thread_list signal_thread_list_anchor = {
-    0, 0, 0,
-};
+static LIST_HEAD(signal_thread_list_head);
 
 #define FGLOCK(lock, body) do { \
     native_mutex_lock(lock); \
@@ -1144,12 +1136,12 @@ static struct signal_thread_list signal_thread_list_anchor = {
 static void
 print_signal_list(char *str)
 {
-    struct signal_thread_list *list =
-      signal_thread_list_anchor.next;
+    rb_thread_t *th;
+
     thread_debug("list (%s)> ", str);
-    while (list) {
-	thread_debug("%p (%p), ", list->th, list->th->thread_id);
-	list = list->next;
+    list_for_each(&signal_thread_list_head, th,
+		  native_thread_data.signal_thread.node) {
+	thread_debug("%p (%p), ", th, th->thread_id);
     }
     thread_debug("\n");
 }
@@ -1158,47 +1150,23 @@ print_signal_list(char *str)
 static void
 add_signal_thread_list(rb_thread_t *th)
 {
-    if (!th->native_thread_data.signal_thread_list) {
-	FGLOCK(&signal_thread_list_lock, {
-	    struct signal_thread_list *list =
-	      malloc(sizeof(struct signal_thread_list));
-
-	    if (list == 0) {
-		fprintf(stderr, "[FATAL] failed to allocate memory\n");
-		exit(EXIT_FAILURE);
-	    }
-
-	    list->th = th;
-
-	    list->prev = &signal_thread_list_anchor;
-	    list->next = signal_thread_list_anchor.next;
-	    if (list->next) {
-		list->next->prev = list;
-	    }
-	    signal_thread_list_anchor.next = list;
-	    th->native_thread_data.signal_thread_list = list;
-	});
-    }
+    FGLOCK(&signal_thread_list_lock, {
+	if (!th->native_thread_data.signal_thread.listed) {
+	    list_add_tail(&signal_thread_list_head,
+			  &th->native_thread_data.signal_thread.node);
+	}
+    });
 }
 
 static void
 remove_signal_thread_list(rb_thread_t *th)
 {
-    if (th->native_thread_data.signal_thread_list) {
-	FGLOCK(&signal_thread_list_lock, {
-	    struct signal_thread_list *list =
-	      (struct signal_thread_list *)
-		th->native_thread_data.signal_thread_list;
-
-	    list->prev->next = list->next;
-	    if (list->next) {
-		list->next->prev = list->prev;
-	    }
-	    th->native_thread_data.signal_thread_list = 0;
-	    list->th = 0;
-	    free(list); /* ok */
-	});
-    }
+    FGLOCK(&signal_thread_list_lock, {
+	if (th->native_thread_data.signal_thread.listed) {
+	    list_del(&th->native_thread_data.signal_thread.node);
+	    th->native_thread_data.signal_thread.listed = 0;
+	}
+    });
 }
 
 static void
@@ -1231,26 +1199,20 @@ ubf_select(void *ptr)
 static void
 ping_signal_thread_list(void)
 {
-    if (signal_thread_list_anchor.next) {
-	FGLOCK(&signal_thread_list_lock, {
-	    struct signal_thread_list *list;
+    FGLOCK(&signal_thread_list_lock, {
+	rb_thread_t *th;
 
-	    list = signal_thread_list_anchor.next;
-	    while (list) {
-		ubf_select_each(list->th);
-		list = list->next;
-	    }
-	});
-    }
+	list_for_each(&signal_thread_list_head, th,
+	              native_thread_data.signal_thread.node) {
+	    ubf_select_each(th);
+	}
+    });
 }
 
 static int
 check_signal_thread_list(void)
 {
-    if (signal_thread_list_anchor.next)
-	return 1;
-    else
-	return 0;
+    return !list_empty(&signal_thread_list_head);
 }
 #else /* USE_SIGNAL_THREAD_LIST */
 #define add_signal_thread_list(th) (void)(th)
diff --git a/thread_pthread.h b/thread_pthread.h
index 24a4af4..dcffd7a 100644
--- a/thread_pthread.h
+++ b/thread_pthread.h
@@ -11,6 +11,8 @@
 #ifndef RUBY_THREAD_PTHREAD_H
 #define RUBY_THREAD_PTHREAD_H
 
+#include "ccan/list/list.h"
+
 #ifdef HAVE_PTHREAD_NP_H
 #include <pthread_np.h>
 #endif
@@ -26,7 +28,12 @@ typedef struct rb_thread_cond_struct {
 } rb_nativethread_cond_t;
 
 typedef struct native_thread_data_struct {
-    void *signal_thread_list;
+    /* Protected by signal_thread_list_lock: */
+    union {
+	unsigned long listed;
+	struct list_node node;
+    } signal_thread;
+
     rb_nativethread_cond_t sleep_cond;
 } native_thread_data_t;
 
-- 
EW