Re: [PATCH] rxrpc: use kthread_ API

[Christoph Hellwig <hch@lst.de>]

On Tue, Feb 14, 2006 at 08:04:04PM +0000, David Howells wrote:
> > Well, kthread_run can returns errors.  If you want to avoid that for
> > some reasons we'd need a local varibale
> 
> So what? There's an implicit local variable anyway. The compiler doesn't pass
> the address of rxrpc_krxiod_thread to kthread_run() so that the latter can
> fill it in; the result is returned in a register. Sticking a local variable in
> the source would just attach a label to that register. Moving the assignment
> to the global variable to after the error checking if-statement would then
> mean the store instruction is emitted after the branch rather than before it.
> 
> > , which would be rather silly.
> 
> No. It'd probably be faster in the error case (no write to the memory).

New patch below that implements it the way you want, although I find it
harder to read.  Note that both versions generate exactly the same code for
me (gcc version 4.0.2 20050806 (prerelease) (Debian 4.0.1-4) on ppc)


Index: linux-2.6/net/rxrpc/krxiod.c
===================================================================
--- linux-2.6.orig/net/rxrpc/krxiod.c	2006-02-14 16:23:31.000000000 +0100
+++ linux-2.6/net/rxrpc/krxiod.c	2006-02-14 21:10:53.000000000 +0100
@@ -13,14 +13,15 @@
 #include <linux/completion.h>
 #include <linux/spinlock.h>
 #include <linux/init.h>
+#include <linux/kthread.h>
 #include <rxrpc/krxiod.h>
 #include <rxrpc/transport.h>
 #include <rxrpc/peer.h>
 #include <rxrpc/call.h>
 #include "internal.h"
 
-static DECLARE_WAIT_QUEUE_HEAD(rxrpc_krxiod_sleepq);
-static DECLARE_COMPLETION(rxrpc_krxiod_dead);
+
+static struct task_struct *rxrpc_krxiod_thread;
 
 static atomic_t rxrpc_krxiod_qcount = ATOMIC_INIT(0);
 
@@ -30,20 +31,14 @@
 static LIST_HEAD(rxrpc_krxiod_callq);
 static DEFINE_SPINLOCK(rxrpc_krxiod_callq_lock);
 
-static volatile int rxrpc_krxiod_die;
-
 /*****************************************************************************/
 /*
  * Rx I/O daemon
  */
 static int rxrpc_krxiod(void *arg)
 {
-	DECLARE_WAITQUEUE(krxiod,current);
-
 	printk("Started krxiod %d\n",current->pid);
 
-	daemonize("krxiod");
-
 	/* loop around waiting for work to do */
 	do {
 		/* wait for work or to be told to exit */
@@ -51,19 +46,16 @@
 		if (!atomic_read(&rxrpc_krxiod_qcount)) {
 			set_current_state(TASK_INTERRUPTIBLE);
 
-			add_wait_queue(&rxrpc_krxiod_sleepq, &krxiod);
-
 			for (;;) {
 				set_current_state(TASK_INTERRUPTIBLE);
 				if (atomic_read(&rxrpc_krxiod_qcount) ||
-				    rxrpc_krxiod_die ||
+				    kthread_should_stop() ||
 				    signal_pending(current))
 					break;
 
 				schedule();
 			}
 
-			remove_wait_queue(&rxrpc_krxiod_sleepq, &krxiod);
 			set_current_state(TASK_RUNNING);
 		}
 		_debug("### End Wait");
@@ -143,11 +135,9 @@
                 /* discard pending signals */
 		rxrpc_discard_my_signals();
 
-	} while (!rxrpc_krxiod_die);
-
-	/* and that's all */
-	complete_and_exit(&rxrpc_krxiod_dead, 0);
+	} while (!kthread_should_stop());
 
+	return 0;
 } /* end rxrpc_krxiod() */
 
 /*****************************************************************************/
@@ -156,7 +146,12 @@
  */
 int __init rxrpc_krxiod_init(void)
 {
-	return kernel_thread(rxrpc_krxiod, NULL, 0);
+	struct task_struct *t = kthread_run(rxrpc_krxiod, NULL, "krxiod");
+
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	rxrpc_krxiod_thread = t;
+	return 0;
 
 } /* end rxrpc_krxiod_init() */
 
@@ -166,10 +161,7 @@
  */
 void rxrpc_krxiod_kill(void)
 {
-	rxrpc_krxiod_die = 1;
-	wake_up_all(&rxrpc_krxiod_sleepq);
-	wait_for_completion(&rxrpc_krxiod_dead);
-
+	kthread_stop(rxrpc_krxiod_thread);
 } /* end rxrpc_krxiod_kill() */
 
 /*****************************************************************************/
@@ -194,7 +186,7 @@
 		}
 
 		spin_unlock_irqrestore(&rxrpc_krxiod_transportq_lock, flags);
-		wake_up_all(&rxrpc_krxiod_sleepq);
+		wake_up_process(rxrpc_krxiod_thread);
 	}
 
 	_leave("");
@@ -239,7 +231,7 @@
 		}
 		spin_unlock_irqrestore(&rxrpc_krxiod_callq_lock, flags);
 	}
-	wake_up_all(&rxrpc_krxiod_sleepq);
+	wake_up_process(rxrpc_krxiod_thread);
 
 } /* end rxrpc_krxiod_queue_call() */
 
Index: linux-2.6/net/rxrpc/krxsecd.c
===================================================================
--- linux-2.6.orig/net/rxrpc/krxsecd.c	2006-02-14 16:23:31.000000000 +0100
+++ linux-2.6/net/rxrpc/krxsecd.c	2006-02-14 21:11:25.000000000 +0100
@@ -19,6 +19,7 @@
 #include <linux/completion.h>
 #include <linux/spinlock.h>
 #include <linux/init.h>
+#include <linux/kthread.h>
 #include <rxrpc/krxsecd.h>
 #include <rxrpc/transport.h>
 #include <rxrpc/connection.h>
@@ -30,8 +31,8 @@
 #include <net/sock.h>
 #include "internal.h"
 
-static DECLARE_WAIT_QUEUE_HEAD(rxrpc_krxsecd_sleepq);
-static DECLARE_COMPLETION(rxrpc_krxsecd_dead);
+
+static struct task_struct *rxrpc_krxsecd_thread;
 static volatile int rxrpc_krxsecd_die;
 
 static atomic_t rxrpc_krxsecd_qcount;
@@ -49,14 +50,10 @@
  */
 static int rxrpc_krxsecd(void *arg)
 {
-	DECLARE_WAITQUEUE(krxsecd, current);
-
 	int die;
 
 	printk("Started krxsecd %d\n", current->pid);
 
-	daemonize("krxsecd");
-
 	/* loop around waiting for work to do */
 	do {
 		/* wait for work or to be told to exit */
@@ -64,22 +61,19 @@
 		if (!atomic_read(&rxrpc_krxsecd_qcount)) {
 			set_current_state(TASK_INTERRUPTIBLE);
 
-			add_wait_queue(&rxrpc_krxsecd_sleepq, &krxsecd);
-
 			for (;;) {
 				set_current_state(TASK_INTERRUPTIBLE);
 				if (atomic_read(&rxrpc_krxsecd_qcount) ||
-				    rxrpc_krxsecd_die ||
+				    kthread_should_stop() ||
 				    signal_pending(current))
 					break;
 
 				schedule();
 			}
 
-			remove_wait_queue(&rxrpc_krxsecd_sleepq, &krxsecd);
 			set_current_state(TASK_RUNNING);
 		}
-		die = rxrpc_krxsecd_die;
+		die = kthread_should_stop();
 		_debug("### End Wait");
 
 		/* see if there're incoming calls in need of authenticating */
@@ -114,9 +108,8 @@
 
 	} while (!die);
 
-	/* and that's all */
-	complete_and_exit(&rxrpc_krxsecd_dead, 0);
 
+	return 0;
 } /* end rxrpc_krxsecd() */
 
 /*****************************************************************************/
@@ -125,7 +118,12 @@
  */
 int __init rxrpc_krxsecd_init(void)
 {
-	return kernel_thread(rxrpc_krxsecd, NULL, 0);
+	struct task_struct *t = kthread_run(rxrpc_krxsecd, NULL, "krxsecd");
+
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	rxrpc_krxsecd_thread = t;
+	return 0;
 
 } /* end rxrpc_krxsecd_init() */
 
@@ -136,9 +134,7 @@
 void rxrpc_krxsecd_kill(void)
 {
 	rxrpc_krxsecd_die = 1;
-	wake_up_all(&rxrpc_krxsecd_sleepq);
-	wait_for_completion(&rxrpc_krxsecd_dead);
-
+	kthread_stop(rxrpc_krxsecd_thread);
 } /* end rxrpc_krxsecd_kill() */
 
 /*****************************************************************************/
@@ -197,7 +193,7 @@
 
 	spin_unlock(&rxrpc_krxsecd_initmsgq_lock);
 
-	wake_up(&rxrpc_krxsecd_sleepq);
+	wake_up_process(rxrpc_krxsecd_thread);
 
 	_leave("");
 } /* end rxrpc_krxsecd_queue_incoming_call() */
Index: linux-2.6/net/rxrpc/krxtimod.c
===================================================================
--- linux-2.6.orig/net/rxrpc/krxtimod.c	2006-02-14 16:23:31.000000000 +0100
+++ linux-2.6/net/rxrpc/krxtimod.c	2006-02-14 21:11:55.000000000 +0100
@@ -13,15 +13,14 @@
 #include <linux/init.h>
 #include <linux/sched.h>
 #include <linux/completion.h>
+#include <linux/kthread.h>
 #include <rxrpc/rxrpc.h>
 #include <rxrpc/krxtimod.h>
 #include <asm/errno.h>
 #include "internal.h"
 
-static DECLARE_COMPLETION(krxtimod_alive);
-static DECLARE_COMPLETION(krxtimod_dead);
-static DECLARE_WAIT_QUEUE_HEAD(krxtimod_sleepq);
-static int krxtimod_die;
+
+static struct task_struct *rxrpc_krxtimod_thread;
 
 static LIST_HEAD(krxtimod_list);
 static DEFINE_SPINLOCK(krxtimod_lock);
@@ -34,15 +33,12 @@
  */
 int rxrpc_krxtimod_start(void)
 {
-	int ret;
-
-	ret = kernel_thread(krxtimod, NULL, 0);
-	if (ret < 0)
-		return ret;
+	struct task_struct *t = kthread_run(krxtimod, NULL, "krxtimod");
 
-	wait_for_completion(&krxtimod_alive);
-
-	return ret;
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	rxrpc_krxtimod_thread = t;
+	return 0;
 } /* end rxrpc_krxtimod_start() */
 
 /*****************************************************************************/
@@ -51,11 +47,7 @@
  */
 void rxrpc_krxtimod_kill(void)
 {
-	/* get rid of my daemon */
-	krxtimod_die = 1;
-	wake_up(&krxtimod_sleepq);
-	wait_for_completion(&krxtimod_dead);
-
+	kthread_stop(rxrpc_krxtimod_thread);
 } /* end rxrpc_krxtimod_kill() */
 
 /*****************************************************************************/
@@ -64,30 +56,22 @@
  */
 static int krxtimod(void *arg)
 {
-	DECLARE_WAITQUEUE(myself, current);
-
 	rxrpc_timer_t *timer;
 
 	printk("Started krxtimod %d\n", current->pid);
 
-	daemonize("krxtimod");
-
-	complete(&krxtimod_alive);
-
 	/* loop around looking for things to attend to */
  loop:
 	set_current_state(TASK_INTERRUPTIBLE);
-	add_wait_queue(&krxtimod_sleepq, &myself);
 
 	for (;;) {
 		unsigned long jif;
 		long timeout;
 
 		/* deal with the server being asked to die */
-		if (krxtimod_die) {
-			remove_wait_queue(&krxtimod_sleepq, &myself);
+		if (kthread_should_stop()) {
 			_leave("");
-			complete_and_exit(&krxtimod_dead, 0);
+			return 0;
 		}
 
 		try_to_freeze();
@@ -125,7 +109,6 @@
 	 *   entry
 	 */
  immediate:
-	remove_wait_queue(&krxtimod_sleepq, &myself);
 	set_current_state(TASK_RUNNING);
 
 	_debug("@@@ Begin Timeout of %p", timer);
@@ -171,7 +154,7 @@
 
 	spin_unlock(&krxtimod_lock);
 
-	wake_up(&krxtimod_sleepq);
+	wake_up_process(rxrpc_krxtimod_thread);
 
 	_leave("");
 } /* end rxrpc_krxtimod_add_timer() */
@@ -196,7 +179,7 @@
 
 	spin_unlock(&krxtimod_lock);
 
-	wake_up(&krxtimod_sleepq);
+	wake_up_process(rxrpc_krxtimod_thread);
 
 	_leave(" = %d", ret);
 	return ret;