- Add more strict in_atomic() checking to the mutex entry

function just to be extra safety and paranoid.

- Rewrite the thread shim to take full advantage of the
new kernel kthread API.  This greatly simplifies things.

- Add a new regression test for thread_exit() to ensure
it properly terminates a thread immediately without
allowing futher execution of the thread.



git-svn-id: https://outreach.scidac.gov/svn/spl/trunk@69 7e1ea52c-4ff2-0310-8f11-9dd32ca42a1c

include/sys/mutex.h

@@ -6,6 +6,7 @@ extern "C" {
 #endif
 
 #include <linux/module.h>
+#include <linux/hardirq.h>
 #include <sys/types.h>
 
 /* See the "Big Theory Statement" in solaris mutex.c.
@@ -65,6 +66,14 @@ static __inline__ void
 mutex_enter(kmutex_t *mp)
 {
 	BUG_ON(mp->km_magic != KM_MAGIC);
+
+	if (unlikely(in_atomic() && !current->exit_state)) {
+		dump_stack();
+		printk("Scheduling while atomic: %s/0x%08x/%d\n",
+		       current->comm, preempt_count(), current->pid);
+		BUG();
+	}
+
 	down(&mp->km_sem);  /* Will check in_atomic() for us */
 	BUG_ON(mp->km_owner != NULL);
 	mp->km_owner = current;
@@ -78,6 +87,14 @@ mutex_tryenter(kmutex_t *mp)
 	int result;
 
 	BUG_ON(mp->km_magic != KM_MAGIC);
+
+	if (unlikely(in_atomic() && !current->exit_state)) {
+		dump_stack();
+		printk("Scheduling while atomic: %s/0x%08x/%d\n",
+		       current->comm, preempt_count(), current->pid);
+		BUG();
+	}
+
 	result = down_trylock(&mp->km_sem); /* returns 0 if acquired */
 	if (result == 0) {
 		BUG_ON(mp->km_owner != NULL);

modules/spl/spl-thread.c

@@ -1,4 +1,5 @@
 #include <sys/thread.h>
+#include <sys/kmem.h>
 
 /*
  * Thread interfaces
@@ -10,9 +11,6 @@ typedef struct thread_priv_s {
 	size_t tp_len;			/* Len to be passed to function */
 	int tp_state;			/* State to start thread at */
 	pri_t tp_pri;			/* Priority to start threat at */
-	volatile kthread_t *tp_task;	/* Task pointer for new thread */
-	spinlock_t tp_lock;		/* Syncronization lock */
-        wait_queue_head_t tp_waitq;	/* Syncronization wait queue */
 } thread_priv_t;
 
 static int
@@ -22,21 +20,13 @@ thread_generic_wrapper(void *arg)
 	void (*func)(void *);
 	void *args;
 
-        spin_lock(&tp->tp_lock);
 	BUG_ON(tp->tp_magic != TP_MAGIC);
 	func = tp->tp_func;
 	args = tp->tp_args;
-	tp->tp_task = get_current();
 	set_current_state(tp->tp_state);
-	set_user_nice((kthread_t *)tp->tp_task, PRIO_TO_NICE(tp->tp_pri));
+	set_user_nice((kthread_t *)get_current(), PRIO_TO_NICE(tp->tp_pri));
+	kmem_free(arg, sizeof(thread_priv_t));
 
-        spin_unlock(&tp->tp_lock);
-	wake_up(&tp->tp_waitq);
-
-	/* DO NOT USE 'ARG' AFTER THIS POINT, EVER, EVER, EVER!
-	 * Local variables are used here because after the calling thread
-	 * has been woken up it will exit and this memory will no longer
-	 * be safe to access since it was declared on the callers stack. */
 	if (func)
 		func(args);
 
@@ -59,7 +49,7 @@ __thread_create(caddr_t stk, size_t  stksize, thread_func_t func,
 		const char *name, void *args, size_t len, int *pp,
 		int state, pri_t pri)
 {
-	thread_priv_t tp;
+	thread_priv_t *tp;
 	DEFINE_WAIT(wait);
 	struct task_struct *tsk;
 
@@ -68,45 +58,24 @@ __thread_create(caddr_t stk, size_t  stksize, thread_func_t func,
 	BUG_ON(stk != NULL);
 	BUG_ON(stk != 0);
 
-	/* Variable tp is located on the stack and not the heap because I want
-	 * to minimize any chance of a failure, since the Solaris code is designed
-	 * such that this function cannot fail.  This is a little dangerous since
-	 * we're passing a stack address to a new thread but correct locking was
-	 * added to ensure the callee can use the data safely until wake_up(). */
-	tp.tp_magic = TP_MAGIC;
-	tp.tp_func  = func;
-	tp.tp_args  = args;
-	tp.tp_len   = len;
-	tp.tp_state = state;
-	tp.tp_pri   = pri;
-	tp.tp_task  = NULL;
-	spin_lock_init(&tp.tp_lock);
-        init_waitqueue_head(&tp.tp_waitq);
+	tp = kmem_alloc(sizeof(thread_priv_t), KM_SLEEP);
+	if (tp == NULL)
+		return NULL;
 
-	spin_lock(&tp.tp_lock);
+	tp->tp_magic = TP_MAGIC;
+	tp->tp_func  = func;
+	tp->tp_args  = args;
+	tp->tp_len   = len;
+	tp->tp_state = state;
+	tp->tp_pri   = pri;
 
-	tsk = kthread_create(thread_generic_wrapper, (void *)&tp, "%s", name);
+	tsk = kthread_create(thread_generic_wrapper, (void *)tp, "%s", name);
 	if (IS_ERR(tsk)) {
 		printk("spl: Failed to create thread: %ld\n", PTR_ERR(tsk));
 		return NULL;
 	}
 
 	wake_up_process(tsk);
-
-	/* All signals are ignored due to sleeping TASK_UNINTERRUPTIBLE */
-	for (;;) {
-		prepare_to_wait(&tp.tp_waitq, &wait, TASK_UNINTERRUPTIBLE);
-		if (tp.tp_task != NULL)
-			break;
-
-		spin_unlock(&tp.tp_lock);
-		schedule();
-		spin_lock(&tp.tp_lock);
-	}
-
-	BUG_ON(tsk != tp.tp_task); /* Extra paranoia */
-	spin_unlock(&tp.tp_lock);
-
-	return (kthread_t *)tp.tp_task;
+	return (kthread_t *)tsk;
 }
 EXPORT_SYMBOL(__thread_create);

modules/splat/splat-thread.c

@@ -6,7 +6,11 @@
 
 #define SPLAT_THREAD_TEST1_ID		0x0601
 #define SPLAT_THREAD_TEST1_NAME		"create"
-#define SPLAT_THREAD_TEST1_DESC		"Validate thread creation and destruction"
+#define SPLAT_THREAD_TEST1_DESC		"Validate thread creation"
+
+#define SPLAT_THREAD_TEST2_ID		0x0602
+#define SPLAT_THREAD_TEST2_NAME		"exit"
+#define SPLAT_THREAD_TEST2_DESC		"Validate thread exit"
 
 #define SPLAT_THREAD_TEST_MAGIC		0x4488CC00UL
 
@@ -18,24 +22,84 @@ typedef struct thread_priv {
 	int tp_rc;
 } thread_priv_t;
 
+static int
+splat_thread_rc(thread_priv_t *tp, int rc)
+{
+	int ret;
+
+	spin_lock(&tp->tp_lock);
+	ret = (tp->tp_rc == rc);
+	spin_unlock(&tp->tp_lock);
+
+	return ret;
+}
 
 static void
-splat_thread_work(void *priv)
+splat_thread_work1(void *priv)
 {
 	thread_priv_t *tp = (thread_priv_t *)priv;
 
 	spin_lock(&tp->tp_lock);
 	ASSERT(tp->tp_magic == SPLAT_THREAD_TEST_MAGIC);
 	tp->tp_rc = 1;
-
 	spin_unlock(&tp->tp_lock);
-	wake_up(&tp->tp_waitq);
 
+	wake_up(&tp->tp_waitq);
 	thread_exit();
 }
 
 static int
 splat_thread_test1(struct file *file, void *arg)
+{
+	thread_priv_t tp;
+        DEFINE_WAIT(wait);
+	kthread_t *thr;
+
+	tp.tp_magic = SPLAT_THREAD_TEST_MAGIC;
+	tp.tp_file = file;
+        spin_lock_init(&tp.tp_lock);
+	init_waitqueue_head(&tp.tp_waitq);
+	tp.tp_rc = 0;
+
+	thr = (kthread_t *)thread_create(NULL, 0, splat_thread_work1, &tp, 0,
+			                 &p0, TS_RUN, minclsyspri);
+	/* Must never fail under Solaris, but we check anyway since this
+	 * can happen in the linux SPL, we may want to change this behavior */
+	if (thr == NULL)
+		return  -ESRCH;
+
+	/* Sleep until the thread sets tp.tp_rc == 1 */
+	wait_event(tp.tp_waitq, splat_thread_rc(&tp, 1));
+
+        splat_vprint(file, SPLAT_THREAD_TEST1_NAME, "%s",
+	           "Thread successfully started properly\n");
+	return 0;
+}
+
+static void
+splat_thread_work2(void *priv)
+{
+	thread_priv_t *tp = (thread_priv_t *)priv;
+
+	spin_lock(&tp->tp_lock);
+	ASSERT(tp->tp_magic == SPLAT_THREAD_TEST_MAGIC);
+	tp->tp_rc = 1;
+	spin_unlock(&tp->tp_lock);
+
+	wake_up(&tp->tp_waitq);
+	thread_exit();
+
+	/* The following code is unreachable when thread_exit() is
+	 * working properly, which is exactly what we're testing */
+	spin_lock(&tp->tp_lock);
+	tp->tp_rc = 2;
+	spin_unlock(&tp->tp_lock);
+
+	wake_up(&tp->tp_waitq);
+}
+
+static int
+splat_thread_test2(struct file *file, void *arg)
 {
 	thread_priv_t tp;
         DEFINE_WAIT(wait);
@@ -48,32 +112,29 @@ splat_thread_test1(struct file *file, void *arg)
 	init_waitqueue_head(&tp.tp_waitq);
 	tp.tp_rc = 0;
 
-	spin_lock(&tp.tp_lock);
-
-	thr = (kthread_t *)thread_create(NULL, 0, splat_thread_work, &tp, 0,
+	thr = (kthread_t *)thread_create(NULL, 0, splat_thread_work2, &tp, 0,
 			                 &p0, TS_RUN, minclsyspri);
-	/* Must never fail under Solaris, but we check anyway so we can
-	 * report an error when this impossible thing happens */
-	if (thr == NULL) {
-		rc = -ESRCH;
-		goto out;
+	/* Must never fail under Solaris, but we check anyway since this
+	 * can happen in the linux SPL, we may want to change this behavior */
+	if (thr == NULL)
+		return  -ESRCH;
+
+	/* Sleep until the thread sets tp.tp_rc == 1 */
+	wait_event(tp.tp_waitq, splat_thread_rc(&tp, 1));
+
+	/* Sleep until the thread sets tp.tp_rc == 2, or until we hit
+	 * the timeout.  If thread exit is working properly we should
+	 * hit the timeout and never see to.tp_rc == 2. */
+	rc = wait_event_timeout(tp.tp_waitq, splat_thread_rc(&tp, 2), HZ / 10);
+	if (rc > 0) {
+		rc = -EINVAL;
+	        splat_vprint(file, SPLAT_THREAD_TEST2_NAME, "%s",
+		           "Thread did not exit properly at thread_exit()\n");
+	} else {
+	        splat_vprint(file, SPLAT_THREAD_TEST2_NAME, "%s",
+		           "Thread successfully exited at thread_exit()\n");
 	}
 
-        for (;;) {
-                prepare_to_wait(&tp.tp_waitq, &wait, TASK_UNINTERRUPTIBLE);
-                if (tp.tp_rc)
-                        break;
-
-                spin_unlock(&tp.tp_lock);
-                schedule();
-                spin_lock(&tp.tp_lock);
-        }
-
-        splat_vprint(file, SPLAT_THREAD_TEST1_NAME, "%s",
-	           "Thread successfully started and exited cleanly\n");
-out:
-	spin_unlock(&tp.tp_lock);
-
 	return rc;
 }
 
@@ -96,6 +157,8 @@ splat_thread_init(void)
 
         SPLAT_TEST_INIT(sub, SPLAT_THREAD_TEST1_NAME, SPLAT_THREAD_TEST1_DESC,
                       SPLAT_THREAD_TEST1_ID, splat_thread_test1);
+        SPLAT_TEST_INIT(sub, SPLAT_THREAD_TEST2_NAME, SPLAT_THREAD_TEST2_DESC,
+                      SPLAT_THREAD_TEST2_ID, splat_thread_test2);
 
         return sub;
 }
@@ -104,6 +167,7 @@ void
 splat_thread_fini(splat_subsystem_t *sub)
 {
         ASSERT(sub);
+        SPLAT_TEST_FINI(sub, SPLAT_THREAD_TEST2_ID);
         SPLAT_TEST_FINI(sub, SPLAT_THREAD_TEST1_ID);
 
         kfree(sub);