On 04/20, Andrew Morton wrote:
>
> On Fri, 20 Apr 2007 11:41:46 +0100
> David Howells <dhowells@redhat.com> wrote:
> 
> > There are only two non-net patches that AF_RXRPC depends on:
> > 
> >  (1) The key facility changes.  That's all my code anyway, and shouldn't be a
> >      problem to merge unless someone else has put some changes in there that I
> >      don't know about.
> > 
> >  (2) try_to_cancel_delayed_work().  I suppose I could use
> >      cancel_delayed_work() instead, but that's less efficient as it waits for
> >      the timer completion function to finish.
> 
> There are significant workqueue changes in -mm and I plan to send them
> in for 2.6.22.  I doubt if there's anything in there which directly
> affects cancel_delayed_work(), but making changes of this nature against
> 2.6.21 might lead to grief.

I think it is better to use cancel_delayed_work(), but change it to use
del_timer(). I belive cancel_delayed_work() doesn't need del_timer_sync().

We only care when del_timer() returns true. In that case, if the timer
function still runs (possible for single-threaded wqs), it has already
passed __queue_work().

Oleg.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Dave Hansen <hansendc@us.ibm.com> writes:

> On Thu, 2007-04-19 at 17:19 -0400, Trond Myklebust wrote:
>> > With pid namespaces all kernel threads will disappear so how do
>> > we cope with the problem when the sysadmin can not see the kernel
>> > threads?
>
> Do they actually always disappear, or do we keep them in the
> init_pid_namespace?

In the init pid namespace but not in any of it's children.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Trond Myklebust <trond.myklebust@fys.uio.no> writes:

> On Thu, 2007-04-19 at 14:40 -0700, Andrew Morton wrote:
>> Using signals to communicate with kernel threads is fairly unpleasant, IMO.
>> We have much simpler, faster and more idiomatic ways of communicating
>> between threads in-kernel and there are better ways in which userspace can
>> communicate with the kernel - system calls, for example...
>> 
>> So I think generally any move which gets us away from using signals in
>> kernel threads is moving in a good direction.
>
> I have yet to see a proposal which did. Eric's patch was eliminating
> signals in kernel threads that used them without proposing any
> replacement mechanism or showing that he had plans to do so. That is a
> good reason for a veto.

Possibly I just hadn't looked close enough.  The signals looked like
a redundant mechanism.

>> > > With pid namespaces all kernel threads will disappear so how do
>> > > we cope with the problem when the sysadmin can not see the kernel
>> > > threads?
>> > 
>> > Then you have a usability problem. How does the sysadmin reboot the
>> > system if there is no way to shut down the processes that are hanging on
>> > an unresponsive filesystem?
>> 
>> Where's the hang?  A user process is stuck on h_rwsem?
>> 
>> If so, would it be appropriate to convert the user process to use
>> down_foo_interruptible(), so that the operator can just kill the user
>> process as expected, rather than having to futz around killing kernel
>> threads?
>
> If an NFS server reboots, then the locks held by user processes on the
> client need to be re-established by when it comes up again. Otherwise,
> the processes that thought they were holding locks will suddenly fail.
> This recovery job is currently the done by a kernel thread.
>
> The question is then what to do if the server crashes again while the
> kernel thread is re-establishing the locks. Particularly if it never
> comes back again.
> Currently, the administrator can intervene by killing anything that has
> open files on that volume and kill the recovery kernel thread.
> You'll also note that lockd_down(), nfsd_down() etc all use signals to
> inform lockd(), nfsd() etc that they should be shutting down. Since the
> reclaimer thread is started by the lockd() thread using CLONE_SIGHAND,
> this means that we also automatically kill any lingering recovery
> threads whenever we shutdown lockd().

Maybe I'm missing something but I think you are referring to the semantics
of do_group_exit in the presence of CLONE_THREAD.  All sharing a
sighand should do is cause the sharing of the signal handler.  Causing
allow_signal and disallow_signal to act on a group of threads instead
of a single thread.   I don't recall clone_sighand having any
other effects.

> These mechanisms need to be replaced _before_ we start shooting down
> sigallow() etc in the kernel.

Reasonable if these mechanisms are not redundant.

Thinking it through because everything having to do with nfs mounting and
unmounting is behind the privileged mount operation this is not going to
become an issue until we start allowing unprivileged nfs mounts.  Because
we cannot delegate control of nfs mount and unmount operations until then.

Since signals do not pose a immediate barrier to forward progress like
daemonize and kernel_thread we can leave things as is until we can
sort this out.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Robin Holt <holt@sgi.com> writes:

> I think this was originally coded with daemonize to avoid issues with
> reaping children.  Dean Nelson can correct me if I am wrong.  I assume
> this patch is going in as part of the set which will make these threads
> clear themselves from the children list and if that is the case, I can
> see no issues.

One of my earlier patches guarantees that kthreadd will have pid == 2.

daemonize actually explicitly reparents to init so using daemonize and
kernel_thread provides no help at all with respect to scaling.  It in
fact guarantees you will be on init's list of child processes.

The work to enhance wait is a little tricky and it conflicts with the
utrace patches, which makes it hard to pursue at the moment.

I'm actually sorting out kthread stop so I can complete the pid namespace.
But since all kthreads are children of kthreadd this helps in a small
way with the scaling issue.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 12:55:29AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> 
> kthread_run replaces the kernel_thread and  daemonize calls
> during thread startup.
> 
> Calls to signal_pending were also removed as it is currently
> impossible for the cpci_hotplug thread to receive signals.

This drivers thread are a bit of a miss, although a lot better than
most other pci hotplug drivers :)

Below is more complete conversion to the kthread infrastructure +
wake_up_process to wake the thread.  Note that we had to keep
a thread_finished variable because the existing one had dual use.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/pci/hotplug/cpci_hotplug_core.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/cpci_hotplug_core.c	2007-04-22 12:54:17.000000000 +0200
+++ linux-2.6/drivers/pci/hotplug/cpci_hotplug_core.c	2007-04-22 13:01:42.000000000 +0200
@@ -35,6 +35,7 @@
 #include <linux/smp_lock.h>
 #include <asm/atomic.h>
 #include <linux/delay.h>
+#include <linux/kthread.h>
 #include "cpci_hotplug.h"
 
 #define DRIVER_AUTHOR	"Scott Murray <scottm@somanetworks.com>"
@@ -59,9 +60,8 @@ static int slots;
 static atomic_t extracting;
 int cpci_debug;
 static struct cpci_hp_controller *controller;
-static struct semaphore event_semaphore;	/* mutex for process loop (up if something to process) */
-static struct semaphore thread_exit;		/* guard ensure thread has exited before calling it quits */
-static int thread_finished = 1;
+static struct task_struct *cpci_thread;
+static int thread_finished;
 
 static int enable_slot(struct hotplug_slot *slot);
 static int disable_slot(struct hotplug_slot *slot);
@@ -357,9 +357,7 @@ cpci_hp_intr(int irq, void *data)
 	controller->ops->disable_irq();
 
 	/* Trigger processing by the event thread */
-	dbg("Signal event_semaphore");
-	up(&event_semaphore);
-	dbg("exited cpci_hp_intr");
+	wake_up_process(cpci_thread);
 	return IRQ_HANDLED;
 }
 
@@ -521,17 +519,12 @@ event_thread(void *data)
 {
 	int rc;
 
-	lock_kernel();
-	daemonize("cpci_hp_eventd");
-	unlock_kernel();
-
 	dbg("%s - event thread started", __FUNCTION__);
 	while (1) {
 		dbg("event thread sleeping");
-		down_interruptible(&event_semaphore);
-		dbg("event thread woken, thread_finished = %d",
-		    thread_finished);
-		if (thread_finished || signal_pending(current))
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+		if (kthread_should_stop())
 			break;
 		do {
 			rc = check_slots();
@@ -541,18 +534,17 @@ event_thread(void *data)
 			} else if (rc < 0) {
 				dbg("%s - error checking slots", __FUNCTION__);
 				thread_finished = 1;
-				break;
+				goto out;
 			}
-		} while (atomic_read(&extracting) && !thread_finished);
-		if (thread_finished)
+		} while (atomic_read(&extracting) && !kthread_should_stop());
+		if (kthread_should_stop())
 			break;
 
 		/* Re-enable ENUM# interrupt */
 		dbg("%s - re-enabling irq", __FUNCTION__);
 		controller->ops->enable_irq();
 	}
-	dbg("%s - event thread signals exit", __FUNCTION__);
-	up(&thread_exit);
+ out:
 	return 0;
 }
 
@@ -562,12 +554,8 @@ poll_thread(void *data)
 {
 	int rc;
 
-	lock_kernel();
-	daemonize("cpci_hp_polld");
-	unlock_kernel();
-
 	while (1) {
-		if (thread_finished || signal_pending(current))
+		if (kthread_should_stop() || signal_pending(current))
 			break;
 		if (controller->ops->query_enum()) {
 			do {
@@ -578,48 +566,34 @@ poll_thread(void *data)
 				} else if (rc < 0) {
 					dbg("%s - error checking slots", __FUNCTION__);
 					thread_finished = 1;
-					break;
+					goto out;
 				}
-			} while (atomic_read(&extracting) && !thread_finished);
+			} while (atomic_read(&extracting) && !kthread_should_stop());
 		}
 		msleep(100);
 	}
-	dbg("poll thread signals exit");
-	up(&thread_exit);
+ out:
 	return 0;
 }
 
 static int
 cpci_start_thread(void)
 {
-	int pid;
-
-	/* initialize our semaphores */
-	init_MUTEX_LOCKED(&event_semaphore);
-	init_MUTEX_LOCKED(&thread_exit);
-	thread_finished = 0;
-
 	if (controller->irq)
-		pid = kernel_thread(event_thread, NULL, 0);
+		cpci_thread = kthread_run(event_thread, NULL, "cpci_hp_eventd");
 	else
-		pid = kernel_thread(poll_thread, NULL, 0);
-	if (pid < 0) {
+		cpci_thread = kthread_run(poll_thread, NULL, "cpci_hp_polld");
+	if (IS_ERR(cpci_thread)) {
 		err("Can't start up our thread");
-		return -1;
+		return PTR_ERR(cpci_thread);
 	}
-	dbg("Our thread pid = %d", pid);
 	return 0;
 }
 
 static void
 cpci_stop_thread(void)
 {
-	thread_finished = 1;
-	dbg("thread finish command given");
-	if (controller->irq)
-		up(&event_semaphore);
-	dbg("wait for thread to exit");
-	down(&thread_exit);
+	kthread_stop(cpci_thread);
 }
 
 int
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 12:55:30AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> 
> kthread_run replaces kernel_thread and dameonize.
> 
> allow_signal is unnecessary and has been removed.
> tid_poll was unused and has been removed.

Thread handling in this driver is quite interesting. Greg has his name
in there, so we can blame everything on him ;-)

Below is my take at cleaning at least the thread-related bits up:

 - full switch to kthread infrastructure
 - switch semOperations to a mutex, and give it a proper name
 - remove the useless hpc_poll_thread wrapper
 - remove ibmphp_hpc_initvars - everything left can easily be
   initialized statically


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/pci/hotplug/ibmphp.h
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/ibmphp.h	2007-04-22 12:44:04.000000000 +0200
+++ linux-2.6/drivers/pci/hotplug/ibmphp.h	2007-04-22 12:44:07.000000000 +0200
@@ -392,7 +392,6 @@ extern int ibmphp_add_pfmem_from_mem (st
 extern struct bus_node *ibmphp_find_res_bus (u8);
 extern void ibmphp_print_test (void);	/* for debugging purposes */
 
-extern void ibmphp_hpc_initvars (void);
 extern int ibmphp_hpc_readslot (struct slot *, u8, u8 *);
 extern int ibmphp_hpc_writeslot (struct slot *, u8);
 extern void ibmphp_lock_operations (void);
Index: linux-2.6/drivers/pci/hotplug/ibmphp_core.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/ibmphp_core.c	2007-04-22 12:44:11.000000000 +0200
+++ linux-2.6/drivers/pci/hotplug/ibmphp_core.c	2007-04-22 12:44:16.000000000 +0200
@@ -1368,8 +1368,6 @@ static int __init ibmphp_init(void)
 
 	ibmphp_debug = debug;
 
-	ibmphp_hpc_initvars();
-
 	for (i = 0; i < 16; i++)
 		irqs[i] = 0;
 
Index: linux-2.6/drivers/pci/hotplug/ibmphp_hpc.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/ibmphp_hpc.c	2007-04-22 12:31:23.000000000 +0200
+++ linux-2.6/drivers/pci/hotplug/ibmphp_hpc.c	2007-04-22 12:45:51.000000000 +0200
@@ -35,6 +35,7 @@
 #include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 
 #include "ibmphp.h"
 
@@ -101,12 +102,10 @@ static int to_debug = 0;
 //----------------------------------------------------------------------------
 // global variables
 //----------------------------------------------------------------------------
-static int ibmphp_shutdown;
-static int tid_poll;
-static struct mutex sem_hpcaccess;	// lock access to HPC
-static struct semaphore semOperations;	// lock all operations and
+static struct task_struct *ibmphp_poll_thread;
+static DEFINE_MUTEX(sem_hpcaccess);	// lock access to HPC
+static DEFINE_MUTEX(ibmphp_op_sem);	// lock all operations and
 					// access to data structures
-static struct semaphore sem_exit;	// make sure polling thread goes away
 //----------------------------------------------------------------------------
 // local function prototypes
 //----------------------------------------------------------------------------
@@ -116,33 +115,11 @@ static u8 hpc_writecmdtoindex (u8, u8);
 static u8 hpc_readcmdtoindex (u8, u8);
 static void get_hpc_access (void);
 static void free_hpc_access (void);
-static void poll_hpc (void);
 static int process_changeinstatus (struct slot *, struct slot *);
 static int process_changeinlatch (u8, u8, struct controller *);
-static int hpc_poll_thread (void *);
 static int hpc_wait_ctlr_notworking (int, struct controller *, void __iomem *, u8 *);
 //----------------------------------------------------------------------------
 
-
-/*----------------------------------------------------------------------
-* Name:    ibmphp_hpc_initvars
-*
-* Action:  initialize semaphores and variables
-*---------------------------------------------------------------------*/
-void __init ibmphp_hpc_initvars (void)
-{
-	debug ("%s - Entry\n", __FUNCTION__);
-
-	mutex_init(&sem_hpcaccess);
-	init_MUTEX (&semOperations);
-	init_MUTEX_LOCKED (&sem_exit);
-	to_debug = 0;
-	ibmphp_shutdown = 0;
-	tid_poll = 0;
-
-	debug ("%s - Exit\n", __FUNCTION__);
-}
-
 /*----------------------------------------------------------------------
 * Name:    i2c_ctrl_read
 *
@@ -798,7 +775,7 @@ void free_hpc_access (void)
 *---------------------------------------------------------------------*/
 void ibmphp_lock_operations (void)
 {
-	down (&semOperations);
+	mutex_lock(&ibmphp_op_sem);
 	to_debug = 1;
 }
 
@@ -808,7 +785,7 @@ void ibmphp_lock_operations (void)
 void ibmphp_unlock_operations (void)
 {
 	debug ("%s - Entry\n", __FUNCTION__);
-	up (&semOperations);
+	mutex_unlock(&ibmphp_op_sem);
 	to_debug = 0;
 	debug ("%s - Exit\n", __FUNCTION__);
 }
@@ -819,7 +796,7 @@ void ibmphp_unlock_operations (void)
 #define POLL_LATCH_REGISTER	0
 #define POLL_SLOTS		1
 #define POLL_SLEEP		2
-static void poll_hpc (void)
+static int poll_hpc(void *data)
 {
 	struct slot myslot;
 	struct slot *pslot = NULL;
@@ -833,12 +810,9 @@ static void poll_hpc (void)
 
 	debug ("%s - Entry\n", __FUNCTION__);
 
-	while (!ibmphp_shutdown) {
-		if (ibmphp_shutdown) 
-			break;
-		
+	while (!kthread_should_stop()) {
 		/* try to get the lock to do some kind of hardware access */
-		down (&semOperations);
+		mutex_lock(&ibmphp_op_sem);
 
 		switch (poll_state) {
 		case POLL_LATCH_REGISTER: 
@@ -893,14 +867,13 @@ static void poll_hpc (void)
 			break;
 		case POLL_SLEEP:
 			/* don't sleep with a lock on the hardware */
-			up (&semOperations);
+			mutex_unlock(&ibmphp_op_sem);
 			msleep(POLL_INTERVAL_SEC * 1000);
 
-			if (ibmphp_shutdown) 
+			if (kthread_should_stop())
 				break;
 			
-			down (&semOperations);
-			
+			mutex_lock(&ibmphp_op_sem);
 			if (poll_count >= POLL_LATCH_CNT) {
 				poll_count = 0;
 				poll_state = POLL_SLOTS;
@@ -909,12 +882,13 @@ static void poll_hpc (void)
 			break;
 		}	
 		/* give up the hardware semaphore */
-		up (&semOperations);
+		mutex_unlock(&ibmphp_op_sem);
 		/* sleep for a short time just for good measure */
 		msleep(100);
 	}
-	up (&sem_exit);
 	debug ("%s - Exit\n", __FUNCTION__);
+
+	return 0;
 }
 
 
@@ -1050,47 +1024,19 @@ static int process_changeinlatch (u8 old
 }
 
 /*----------------------------------------------------------------------
-* Name:    hpc_poll_thread
-*
-* Action:  polling
-*
-* Return   0
-* Value:
-*---------------------------------------------------------------------*/
-static int hpc_poll_thread (void *data)
-{
-	debug ("%s - Entry\n", __FUNCTION__);
-
-	daemonize("hpc_poll");
-	allow_signal(SIGKILL);
-
-	poll_hpc ();
-
-	tid_poll = 0;
-	debug ("%s - Exit\n", __FUNCTION__);
-	return 0;
-}
-
-
-/*----------------------------------------------------------------------
 * Name:    ibmphp_hpc_start_poll_thread
 *
 * Action:  start polling thread
 *---------------------------------------------------------------------*/
 int __init ibmphp_hpc_start_poll_thread (void)
 {
-	int rc = 0;
-
-	debug ("%s - Entry\n", __FUNCTION__);
-
-	tid_poll = kernel_thread (hpc_poll_thread, NULL, 0);
-	if (tid_poll < 0) {
+	ibmphp_poll_thread = kthread_run(poll_hpc, NULL, "hpc_poll");
+	if (IS_ERR(ibmphp_poll_thread)) {
 		err ("%s - Error, thread not started\n", __FUNCTION__);
-		rc = -1;
+		return PTR_ERR(ibmphp_poll_thread);
 	}
 
-	debug ("%s - Exit tid_poll[%d] rc[%d]\n", __FUNCTION__, tid_poll, rc);
-	return rc;
+	return 0;
 }
 
 /*----------------------------------------------------------------------
@@ -1100,28 +1046,11 @@ int __init ibmphp_hpc_start_poll_thread 
 *---------------------------------------------------------------------*/
 void __exit ibmphp_hpc_stop_poll_thread (void)
 {
-	debug ("%s - Entry\n", __FUNCTION__);
+	kthread_stop(ibmphp_poll_thread);
 
-	ibmphp_shutdown = 1;
-	debug ("before locking operations \n");
 	ibmphp_lock_operations ();
-	debug ("after locking operations \n");
-	
-	// wait for poll thread to exit
-	debug ("before sem_exit down \n");
-	down (&sem_exit);
-	debug ("after sem_exit down \n");
-
-	// cleanup
-	debug ("before free_hpc_access \n");
 	free_hpc_access ();
-	debug ("after free_hpc_access \n");
 	ibmphp_unlock_operations ();
-	debug ("after unlock operations \n");
-	up (&sem_exit);
-	debug ("after sem exit up\n");
-
-	debug ("%s - Exit\n", __FUNCTION__);
 }
 
 /*----------------------------------------------------------------------
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 12:55:31AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> 
> This patch changes cpqphp to use kthread_run and not
> kernel_thread and daemonize to startup and setup
> the cpqphp thread.

Thread handling in this driver (and actually everything else) seems
to be written by a crackmonkey.

Here's my take at fixing everything slightly related to thread handling
up:

 - full switch to kthread infrastructure
 - remove unused semaphore as mutex and waitqueue in long_delay - 
   in fact that whole function should just go away as the user would
   be a lot more happy with just msleep_interruptible.
 - use wake_up_process for waking the thread


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/pci/hotplug/cpqphp_ctrl.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/cpqphp_ctrl.c	2007-04-22 12:46:33.000000000 +0200
+++ linux-2.6/drivers/pci/hotplug/cpqphp_ctrl.c	2007-04-22 12:53:58.000000000 +0200
@@ -37,6 +37,7 @@
 #include <linux/smp_lock.h>
 #include <linux/pci.h>
 #include <linux/pci_hotplug.h>
+#include <linux/kthread.h>
 #include "cpqphp.h"
 
 static u32 configure_new_device(struct controller* ctrl, struct pci_func *func,
@@ -45,34 +46,20 @@ static int configure_new_function(struct
 			u8 behind_bridge, struct resource_lists *resources);
 static void interrupt_event_handler(struct controller *ctrl);
 
-static struct semaphore event_semaphore;	/* mutex for process loop (up if something to process) */
-static struct semaphore event_exit;		/* guard ensure thread has exited before calling it quits */
-static int event_finished;
-static unsigned long pushbutton_pending;	/* = 0 */
 
-/* things needed for the long_delay function */
-static struct semaphore		delay_sem;
-static wait_queue_head_t	delay_wait;
+static struct task_struct *cpqhp_event_thread;
+static unsigned long pushbutton_pending;	/* = 0 */
 
 /* delay is in jiffies to wait for */
 static void long_delay(int delay)
 {
-	DECLARE_WAITQUEUE(wait, current);
-	
-	/* only allow 1 customer into the delay queue at once
-	 * yes this makes some people wait even longer, but who really cares?
-	 * this is for _huge_ delays to make the hardware happy as the 
-	 * signals bounce around
+	/*
+	 * XXX(hch): if someone is bored please convert all callers
+	 * to call msleep_interruptible directly.  They really want
+	 * to specify timeouts in natural units and spend a lot of
+	 * effort converting them to jiffies..
 	 */
-	down (&delay_sem);
-
-	init_waitqueue_head(&delay_wait);
-
-	add_wait_queue(&delay_wait, &wait);
 	msleep_interruptible(jiffies_to_msecs(delay));
-	remove_wait_queue(&delay_wait, &wait);
-	
-	up(&delay_sem);
 }
 
 
@@ -955,8 +942,8 @@ irqreturn_t cpqhp_ctrl_intr(int IRQ, voi
 	}
 
 	if (schedule_flag) {
-		up(&event_semaphore);
-		dbg("Signal event_semaphore\n");
+		wake_up_process(cpqhp_event_thread);
+		dbg("Waking even thread");
 	}
 	return IRQ_HANDLED;
 }
@@ -1738,7 +1725,7 @@ static u32 remove_board(struct pci_func 
 static void pushbutton_helper_thread(unsigned long data)
 {
 	pushbutton_pending = data;
-	up(&event_semaphore);
+	wake_up_process(cpqhp_event_thread);
 }
 
 
@@ -1746,16 +1733,14 @@ static void pushbutton_helper_thread(uns
 static int event_thread(void* data)
 {
 	struct controller *ctrl;
-	lock_kernel();
-	daemonize("phpd_event");
-	
-	unlock_kernel();
 
 	while (1) {
 		dbg("!!!!event_thread sleeping\n");
-		down_interruptible (&event_semaphore);
-		dbg("event_thread woken finished = %d\n", event_finished);
-		if (event_finished) break;
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+
+		if (kthread_should_stop())
+			break;
 		/* Do stuff here */
 		if (pushbutton_pending)
 			cpqhp_pushbutton_thread(pushbutton_pending);
@@ -1764,38 +1749,24 @@ static int event_thread(void* data)
 				interrupt_event_handler(ctrl);
 	}
 	dbg("event_thread signals exit\n");
-	up(&event_exit);
 	return 0;
 }
 
-
 int cpqhp_event_start_thread(void)
 {
-	int pid;
-
-	/* initialize our semaphores */
-	init_MUTEX(&delay_sem);
-	init_MUTEX_LOCKED(&event_semaphore);
-	init_MUTEX_LOCKED(&event_exit);
-	event_finished=0;
-
-	pid = kernel_thread(event_thread, NULL, 0);
-	if (pid < 0) {
+	cpqhp_event_thread = kthread_run(event_thread, NULL, "phpd_event");
+	if (IS_ERR(cpqhp_event_thread)) {
 		err ("Can't start up our event thread\n");
-		return -1;
+		return PTR_ERR(cpqhp_event_thread);
 	}
-	dbg("Our event thread pid = %d\n", pid);
+
 	return 0;
 }
 
 
 void cpqhp_event_stop_thread(void)
 {
-	event_finished = 1;
-	dbg("event_thread finish command given\n");
-	up(&event_semaphore);
-	dbg("wait for event_thread to exit\n");
-	down(&event_exit);
+	kthread_stop(cpqhp_event_thread);
 }
 
 
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Looks like you were missing at least the pcie hotplug driver.  Another
one of the horrible thread abuses in drivers/pci/hotpug.

 - full conversion to kthread infrastructure
 - use wake_up_process to wake the thread up

Like most pci hotplug drivers it still uses very race non-atomic variable
assignment to communicated with the thread, but that's something the
maintainers should look into.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_ctrl.c	2007-04-22 11:36:58.000000000 +0200
+++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c	2007-04-22 11:42:56.000000000 +0200
@@ -32,14 +32,13 @@
 #include <linux/types.h>
 #include <linux/smp_lock.h>
 #include <linux/pci.h>
+#include <linux/kthread.h>
 #include "../pci.h"
 #include "pciehp.h"
 
 static void interrupt_event_handler(struct controller *ctrl);
 
-static struct semaphore event_semaphore;	/* mutex for process loop (up if something to process) */
-static struct semaphore event_exit;		/* guard ensure thread has exited before calling it quits */
-static int event_finished;
+static struct task_struct *pciehpd_event_thread;
 static unsigned long pushbutton_pending;	/* = 0 */
 static unsigned long surprise_rm_pending;	/* = 0 */
 
@@ -93,8 +92,9 @@ u8 pciehp_handle_attention_button(u8 hp_
 		info("Button ignore on Slot(%s)\n", slot_name(p_slot));
 	}
 
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return 0;
 
@@ -135,8 +135,9 @@ u8 pciehp_handle_switch_change(u8 hp_slo
 		taskInfo->event_type = INT_SWITCH_CLOSE;
 	}
 
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return rc;
 }
@@ -178,8 +179,9 @@ u8 pciehp_handle_presence_change(u8 hp_s
 		taskInfo->event_type = INT_PRESENCE_OFF;
 	}
 
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return rc;
 }
@@ -217,8 +219,10 @@ u8 pciehp_handle_power_fault(u8 hp_slot,
 		taskInfo->event_type = INT_POWER_FAULT;
 		info("power fault bit %x set\n", hp_slot);
 	}
+
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return rc;
 }
@@ -362,7 +366,7 @@ static void pushbutton_helper_thread(uns
 {
 	pushbutton_pending = data;
 
-	up(&event_semaphore);
+	wake_up_process(pciehpd_event_thread);
 }
 
 /**
@@ -452,19 +456,14 @@ static void pciehp_surprise_rm_thread(un
 
 
 /* this is the main worker thread */
-static int event_thread(void* data)
+static int event_thread(void *data)
 {
 	struct controller *ctrl;
-	lock_kernel();
-	daemonize("pciehpd_event");
-
-	unlock_kernel();
 
 	while (1) {
-		dbg("!!!!event_thread sleeping\n");
-		down_interruptible (&event_semaphore);
-		dbg("event_thread woken finished = %d\n", event_finished);
-		if (event_finished || signal_pending(current))
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+		if (kthread_should_stop())
 			break;
 		/* Do stuff here */
 		if (pushbutton_pending)
@@ -476,24 +475,15 @@ static int event_thread(void* data)
 				interrupt_event_handler(ctrl);
 	}
 	dbg("event_thread signals exit\n");
-	up(&event_exit);
 	return 0;
 }
 
 int pciehp_event_start_thread(void)
 {
-	int pid;
-
-	/* initialize our semaphores */
-	init_MUTEX_LOCKED(&event_exit);
-	event_finished=0;
-
-	init_MUTEX_LOCKED(&event_semaphore);
-	pid = kernel_thread(event_thread, NULL, 0);
-
-	if (pid < 0) {
+	pciehpd_event_thread = kthread_run(event_thread, NULL, "pciehpd_event");
+	if (IS_ERR(pciehpd_event_thread)) {
 		err ("Can't start up our event thread\n");
-		return -1;
+		return PTR_ERR(pciehpd_event_thread);
 	}
 	return 0;
 }
@@ -501,9 +491,7 @@ int pciehp_event_start_thread(void)
 
 void pciehp_event_stop_thread(void)
 {
-	event_finished = 1;
-	up(&event_semaphore);
-	down(&event_exit);
+	kthread_stop(pciehpd_event_thread);
 }
 
 
@@ -624,7 +612,7 @@ static void interrupt_event_handler(stru
 						dbg("Surprise Removal\n");
 						if (p_slot) {
 							surprise_rm_pending = (unsigned long) p_slot;
-							up(&event_semaphore);
+							wake_up_process(pciehpd_event_thread);
 							update_slot_info(p_slot);
 						}
 					}
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 12:55:28AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> 
> thread_run is used intead of kernel_thread, daemonize, and mucking
> around blocking signals directly.

This is the full conversion I sent to Dave in April 2006, but never got
any feedback to:


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/mtd/mtd_blkdevs.c
===================================================================
--- linux-2.6.orig/drivers/mtd/mtd_blkdevs.c	2007-01-29 10:03:52.000000000 +0100
+++ linux-2.6/drivers/mtd/mtd_blkdevs.c	2007-04-22 13:22:03.000000000 +0200
@@ -20,6 +20,7 @@
 #include <linux/hdreg.h>
 #include <linux/init.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 #include <asm/uaccess.h>
 
 static LIST_HEAD(blktrans_majors);
@@ -28,9 +29,7 @@ extern struct mutex mtd_table_mutex;
 extern struct mtd_info *mtd_table[];
 
 struct mtd_blkcore_priv {
-	struct completion thread_dead;
-	int exiting;
-	wait_queue_head_t thread_wq;
+	struct task_struct *thread;
 	struct request_queue *rq;
 	spinlock_t queue_lock;
 };
@@ -83,38 +82,19 @@ static int mtd_blktrans_thread(void *arg
 	/* we might get involved when memory gets low, so use PF_MEMALLOC */
 	current->flags |= PF_MEMALLOC | PF_NOFREEZE;
 
-	daemonize("%sd", tr->name);
-
-	/* daemonize() doesn't do this for us since some kernel threads
-	   actually want to deal with signals. We can't just call
-	   exit_sighand() since that'll cause an oops when we finally
-	   do exit. */
-	spin_lock_irq(&current->sighand->siglock);
-	sigfillset(&current->blocked);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
-
 	spin_lock_irq(rq->queue_lock);
-
-	while (!tr->blkcore_priv->exiting) {
+	while (!kthread_should_stop()) {
 		struct request *req;
 		struct mtd_blktrans_dev *dev;
 		int res = 0;
-		DECLARE_WAITQUEUE(wait, current);
 
 		req = elv_next_request(rq);
 
 		if (!req) {
-			add_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
-			set_current_state(TASK_INTERRUPTIBLE);
-
 			spin_unlock_irq(rq->queue_lock);
-
+			set_current_state(TASK_INTERRUPTIBLE);
 			schedule();
-			remove_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
-
 			spin_lock_irq(rq->queue_lock);
-
 			continue;
 		}
 
@@ -133,13 +113,13 @@ static int mtd_blktrans_thread(void *arg
 	}
 	spin_unlock_irq(rq->queue_lock);
 
-	complete_and_exit(&tr->blkcore_priv->thread_dead, 0);
+	return 0;
 }
 
 static void mtd_blktrans_request(struct request_queue *rq)
 {
 	struct mtd_blktrans_ops *tr = rq->queuedata;
-	wake_up(&tr->blkcore_priv->thread_wq);
+	wake_up_process(tr->blkcore_priv->thread);
 }
 
 
@@ -388,8 +368,6 @@ int register_mtd_blktrans(struct mtd_blk
 		return ret;
 	}
 	spin_lock_init(&tr->blkcore_priv->queue_lock);
-	init_completion(&tr->blkcore_priv->thread_dead);
-	init_waitqueue_head(&tr->blkcore_priv->thread_wq);
 
 	tr->blkcore_priv->rq = blk_init_queue(mtd_blktrans_request, &tr->blkcore_priv->queue_lock);
 	if (!tr->blkcore_priv->rq) {
@@ -403,13 +381,14 @@ int register_mtd_blktrans(struct mtd_blk
 	blk_queue_hardsect_size(tr->blkcore_priv->rq, tr->blksize);
 	tr->blkshift = ffs(tr->blksize) - 1;
 
-	ret = kernel_thread(mtd_blktrans_thread, tr, CLONE_KERNEL);
-	if (ret < 0) {
+	tr->blkcore_priv->thread = kthread_run(mtd_blktrans_thread, tr,
+			"%sd", tr->name);
+	if (IS_ERR(tr->blkcore_priv->thread)) {
 		blk_cleanup_queue(tr->blkcore_priv->rq);
 		unregister_blkdev(tr->major, tr->name);
 		kfree(tr->blkcore_priv);
 		mutex_unlock(&mtd_table_mutex);
-		return ret;
+		return PTR_ERR(tr->blkcore_priv->thread);
 	}
 
 	INIT_LIST_HEAD(&tr->devs);
@@ -432,9 +411,7 @@ int deregister_mtd_blktrans(struct mtd_b
 	mutex_lock(&mtd_table_mutex);
 
 	/* Clean up the kernel thread */
-	tr->blkcore_priv->exiting = 1;
-	wake_up(&tr->blkcore_priv->thread_wq);
-	wait_for_completion(&tr->blkcore_priv->thread_dead);
+	kthread_stop(tr->blkcore_priv->thread);
 
 	/* Remove it from the list of active majors */
 	list_del(&tr->list);
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> This patch modifies the startup of eehd to use kthread_run
> not a combination of kernel_thread and daemonize.  Making
> the code slightly simpler and more maintainable.

This one has the same scheme as the various s390 drivers where a thread
is spawned using a workqueue on demand.  I think we should not blindly
convert it but think a litte more about it.

The first question is obviously, is this really something we want?
spawning kernel thread on demand without reaping them properly seems
quite dangerous.

The second question is whether this is the right implementation.
kthread_create already works by using a workqueue to create the thread
and then waits for it.  If we really want to support creating threads
asynchronously on demand we should have a proper API in kthread.c for
this instead of spreading workqueues.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 01:58:46AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> This patch modifies the startup of rtasd to use kthread_run instaed of
> a combination of kernel_thread and daemonize.  Making the code a little
> simpler and more maintainble.

Looks okay, but I have some questions about the original code.

Why does the driver only check if it really needs to run in the
thread and calls vmalloc from it?  If we did all these in the
initialization function we could actually properly unwind.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, 2007-04-22 at 13:24 +0100, Christoph Hellwig wrote:
> This is the full conversion I sent to Dave in April 2006, but never
> got any feedback to:

Sorry about that; I need prodding sometimes. I'll provide some now... 

Can you show me why the thread won't now miss a wakeup if it goes to
sleep just as a new request is added to its queue?

Having already applied Eric's patch, this is the delta to yours...

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 1aa018a..d065dba 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -29,9 +29,7 @@ extern struct mutex mtd_table_mutex;
 extern struct mtd_info *mtd_table[];
 
 struct mtd_blkcore_priv {
-	struct completion thread_dead;
-	int exiting;
-	wait_queue_head_t thread_wq;
+	struct task_struct *thread;
 	struct request_queue *rq;
 	spinlock_t queue_lock;
 };
@@ -85,26 +83,18 @@ static int mtd_blktrans_thread(void *arg)
 	current->flags |= PF_MEMALLOC | PF_NOFREEZE;
 
 	spin_lock_irq(rq->queue_lock);
-
-	while (!tr->blkcore_priv->exiting) {
+	while (!kthread_should_stop()) {
 		struct request *req;
 		struct mtd_blktrans_dev *dev;
 		int res = 0;
-		DECLARE_WAITQUEUE(wait, current);
 
 		req = elv_next_request(rq);
 
 		if (!req) {
-			add_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
-			set_current_state(TASK_INTERRUPTIBLE);
-
 			spin_unlock_irq(rq->queue_lock);
-
+			set_current_state(TASK_INTERRUPTIBLE);
 			schedule();
-			remove_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
-
 			spin_lock_irq(rq->queue_lock);
-
 			continue;
 		}
 
@@ -123,13 +113,13 @@ static int mtd_blktrans_thread(void *arg)
 	}
 	spin_unlock_irq(rq->queue_lock);
 
-	complete_and_exit(&tr->blkcore_priv->thread_dead, 0);
+	return 0;
 }
 
 static void mtd_blktrans_request(struct request_queue *rq)
 {
 	struct mtd_blktrans_ops *tr = rq->queuedata;
-	wake_up(&tr->blkcore_priv->thread_wq);
+	wake_up_process(tr->blkcore_priv->thread);
 }
 
 
@@ -355,7 +345,6 @@ static struct mtd_notifier blktrans_notifier = {
 
 int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
 {
-	struct task_struct *task;
 	int ret, i;
 
 	/* Register the notifier if/when the first device type is
@@ -379,8 +368,6 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
 		return ret;
 	}
 	spin_lock_init(&tr->blkcore_priv->queue_lock);
-	init_completion(&tr->blkcore_priv->thread_dead);
-	init_waitqueue_head(&tr->blkcore_priv->thread_wq);
 
 	tr->blkcore_priv->rq = blk_init_queue(mtd_blktrans_request, &tr->blkcore_priv->queue_lock);
 	if (!tr->blkcore_priv->rq) {
@@ -394,13 +381,14 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
 	blk_queue_hardsect_size(tr->blkcore_priv->rq, tr->blksize);
 	tr->blkshift = ffs(tr->blksize) - 1;
 
-	task = kthread_run(mtd_blktrans_thread, tr, "%sd", tr->name);
-	if (IS_ERR(task)) {
+	tr->blkcore_priv->thread = kthread_run(mtd_blktrans_thread, tr,
+			"%sd", tr->name);
+	if (IS_ERR(tr->blkcore_priv->thread)) {
 		blk_cleanup_queue(tr->blkcore_priv->rq);
 		unregister_blkdev(tr->major, tr->name);
 		kfree(tr->blkcore_priv);
 		mutex_unlock(&mtd_table_mutex);
-		return PTR_ERR(task);
+		return PTR_ERR(tr->blkcore_priv->thread);
 	}
 
 	INIT_LIST_HEAD(&tr->devs);
@@ -423,9 +411,7 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
 	mutex_lock(&mtd_table_mutex);
 
 	/* Clean up the kernel thread */
-	tr->blkcore_priv->exiting = 1;
-	wake_up(&tr->blkcore_priv->thread_wq);
-	wait_for_completion(&tr->blkcore_priv->thread_dead);
+	kthread_stop(tr->blkcore_priv->thread);
 
 	/* Remove it from the list of active majors */
 	list_del(&tr->list);


-- 
dwmw2

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

James Bottomley <James.Bottomley@SteelEye.com> writes:

> Changelog and cc to linux-scsi, and I think it can go in ... not that it
> matters; nothing ever activates this code inside libsas anyway ...

Should we just remove the relevant code then?

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, 2007-04-22 at 20:38 +0100, Christoph Hellwig wrote:
> On Thu, Apr 19, 2007 at 05:37:53PM -0700, Andrew Morton wrote:
> > On Thu, 19 Apr 2007 01:58:38 -0600
> > "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> > 
> > > From: Eric W. Biederman <ebiederm@xmission.com>
> > > 
> > > This patch modifies the sas scsi host thread startup
> > > to use kthread_run not kernel_thread and deamonize.
> > > kthread_run is slightly simpler and more maintainable.
> > > 
> > 
> > Again, I'll rename this to "partially convert...".  This driver should be
> > using kthread_should_stop() and kthread_stop() rather than the
> > apparently-unnecessary ->queue_thread_kill thing.
> > 
> > This driver was merged two and a half years after the kthread API was
> > available.   Our coding-vs-reviewing effort is out of balance.
> 
> Here's a full conversion.

Changelog and cc to linux-scsi, and I think it can go in ... not that it
matters; nothing ever activates this code inside libsas anyway ...

James


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 01:58:47AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> This patch modifies startup of the kfand to use kthread_run
> not a combination of kernel_thread and daemonize, making
> the code a little simpler and more maintaintable.

Why is this driver using a thread at all?  It's only doing a bunch
of rather short-lived things in the thread.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, Apr 22, 2007 at 02:23:00PM +0100, David Woodhouse wrote:
> On Sun, 2007-04-22 at 13:24 +0100, Christoph Hellwig wrote:
> > This is the full conversion I sent to Dave in April 2006, but never
> > got any feedback to:
> 
> Sorry about that; I need prodding sometimes. I'll provide some now... 
> 
> Can you show me why the thread won't now miss a wakeup if it goes to
> sleep just as a new request is added to its queue?

Exactly the same thing that happened before.  If you look at
wake_up_process it's just a tiny wrapper around try_to_wake_up.

And wake_up expands to __wake_up expaneds to __wake_up_common
which just walks the list of threads attached to the waitqueue
and then calls curr->func, which expands to try_to_wake_up.

So when your thread still is in running state nothing changes.
If your thread is not in running state it'll get woken by both
variants.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 12:55:27AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> 
> This patch just trivially replaces kernel_thread and daemonize
> with a single call to kthread_run.

Here's a better patch that does the full kthread conversion +
switch to wake_up_process.  Only compile tested of course due to
lack of voyager hardware.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/arch/i386/mach-voyager/voyager_cat.c
===================================================================
--- linux-2.6.orig/arch/i386/mach-voyager/voyager_cat.c	2007-04-22 15:19:28.000000000 +0200
+++ linux-2.6/arch/i386/mach-voyager/voyager_cat.c	2007-04-22 15:27:03.000000000 +0200
@@ -1111,7 +1111,7 @@ voyager_cat_do_common_interrupt(void)
 				printk(KERN_ERR "Voyager front panel switch turned off\n");
 				voyager_status.switch_off = 1;
 				voyager_status.request_from_kernel = 1;
-				up(&kvoyagerd_sem);
+				wake_up_process(voyager_thread);
 			}
 			/* Tell the hardware we're taking care of the
 			 * shutdown, otherwise it will power the box off
@@ -1157,7 +1157,7 @@ voyager_cat_do_common_interrupt(void)
 			outb(VOYAGER_CAT_END, CAT_CMD);
 			voyager_status.power_fail = 1;
 			voyager_status.request_from_kernel = 1;
-			up(&kvoyagerd_sem);
+			wake_up_process(voyager_thread);
 		}
 		
 		
Index: linux-2.6/arch/i386/mach-voyager/voyager_thread.c
===================================================================
--- linux-2.6.orig/arch/i386/mach-voyager/voyager_thread.c	2007-04-22 15:15:24.000000000 +0200
+++ linux-2.6/arch/i386/mach-voyager/voyager_thread.c	2007-04-22 15:25:51.000000000 +0200
@@ -24,33 +24,16 @@
 #include <linux/kmod.h>
 #include <linux/completion.h>
 #include <linux/sched.h>
+#include <linux/kthread.h>
 #include <asm/desc.h>
 #include <asm/voyager.h>
 #include <asm/vic.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>
 
-#define THREAD_NAME "kvoyagerd"
 
-/* external variables */
-int kvoyagerd_running = 0;
-DECLARE_MUTEX_LOCKED(kvoyagerd_sem);
-
-static int thread(void *);
-
-static __u8 set_timeout = 0;
-
-/* Start the machine monitor thread.  Return 1 if OK, 0 if fail */
-static int __init
-voyager_thread_start(void)
-{
-	if(kernel_thread(thread, NULL, CLONE_KERNEL) < 0) {
-		/* This is serious, but not fatal */
-		printk(KERN_ERR "Voyager: Failed to create system monitor thread!!!\n");
-		return 1;
-	}
-	return 0;
-}
+struct task_struct *voyager_thread;
+static __u8 set_timeout;
 
 static int
 execute(const char *string)
@@ -110,31 +93,15 @@ check_continuing_condition(void)
 	}
 }
 
-static void
-wakeup(unsigned long unused)
-{
-	up(&kvoyagerd_sem);
-}
-
 static int
 thread(void *unused)
 {
-	struct timer_list wakeup_timer;
-
-	kvoyagerd_running = 1;
-
-	daemonize(THREAD_NAME);
-
-	set_timeout = 0;
-
-	init_timer(&wakeup_timer);
-
-	sigfillset(&current->blocked);
-
 	printk(KERN_NOTICE "Voyager starting monitor thread\n");
 
-	for(;;) {
-		down_interruptible(&kvoyagerd_sem);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(set_timeout ? HZ : MAX_SCHEDULE_TIMEOUT);
+
 		VDEBUG(("Voyager Daemon awoken\n"));
 		if(voyager_status.request_from_kernel == 0) {
 			/* probably awoken from timeout */
@@ -143,20 +110,26 @@ thread(void *unused)
 			check_from_kernel();
 			voyager_status.request_from_kernel = 0;
 		}
-		if(set_timeout) {
-			del_timer(&wakeup_timer);
-			wakeup_timer.expires = HZ + jiffies;
-			wakeup_timer.function = wakeup;
-			add_timer(&wakeup_timer);
-		}
 	}
 }
 
+static int __init
+voyager_thread_start(void)
+{
+	voyager_thread = kthread_run(thread, NULL, "kvoyagerd");
+	if (IS_ERR(voyager_thread)) {
+		printk(KERN_ERR "Voyager: Failed to create system monitor thread.\n");
+		return PTR_ERR(voyager_thread);
+	}
+	return 0;
+}
+
+
 static void __exit
 voyager_thread_stop(void)
 {
-	/* FIXME: do nothing at the moment */
+	kthread_stop(voyager_thread);
 }
 
 module_init(voyager_thread_start);
-//module_exit(voyager_thread_stop);
+module_exit(voyager_thread_stop);
Index: linux-2.6/include/asm-i386/voyager.h
===================================================================
--- linux-2.6.orig/include/asm-i386/voyager.h	2007-04-22 15:18:39.000000000 +0200
+++ linux-2.6/include/asm-i386/voyager.h	2007-04-22 15:24:13.000000000 +0200
@@ -487,15 +487,11 @@ extern struct voyager_qic_cpi *voyager_q
 extern struct voyager_SUS *voyager_SUS;
 
 /* variables exported always */
+extern struct task_struct *voyager_thread;
 extern int voyager_level;
-extern int kvoyagerd_running;
-extern struct semaphore kvoyagerd_sem;
 extern struct voyager_status voyager_status;
 
-
-
 /* functions exported by the voyager and voyager_smp modules */
-
 extern int voyager_cat_readb(__u8 module, __u8 asic, int reg);
 extern void voyager_cat_init(void);
 extern void voyager_detect(struct voyager_bios_info *);
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 05:37:53PM -0700, Andrew Morton wrote:
> On Thu, 19 Apr 2007 01:58:38 -0600
> "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> 
> > From: Eric W. Biederman <ebiederm@xmission.com>
> > 
> > This patch modifies the sas scsi host thread startup
> > to use kthread_run not kernel_thread and deamonize.
> > kthread_run is slightly simpler and more maintainable.
> > 
> 
> Again, I'll rename this to "partially convert...".  This driver should be
> using kthread_should_stop() and kthread_stop() rather than the
> apparently-unnecessary ->queue_thread_kill thing.
> 
> This driver was merged two and a half years after the kthread API was
> available.   Our coding-vs-reviewing effort is out of balance.

Here's a full conversion.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.orig/drivers/scsi/libsas/sas_scsi_host.c	2007-04-22 20:30:39.000000000 +0200
+++ linux-2.6/drivers/scsi/libsas/sas_scsi_host.c	2007-04-22 20:36:51.000000000 +0200
@@ -23,6 +23,8 @@
  *
  */
 
+#include <linux/kthread.h>
+
 #include "sas_internal.h"
 
 #include <scsi/scsi_host.h>
@@ -184,7 +186,7 @@ static int sas_queue_up(struct sas_task 
 	list_add_tail(&task->list, &core->task_queue);
 	core->task_queue_size += 1;
 	spin_unlock_irqrestore(&core->task_queue_lock, flags);
-	up(&core->queue_thread_sema);
+	wake_up_process(core->queue_thread);
 
 	return 0;
 }
@@ -819,7 +821,7 @@ static void sas_queue(struct sas_ha_stru
 	struct sas_internal *i = to_sas_internal(core->shost->transportt);
 
 	spin_lock_irqsave(&core->task_queue_lock, flags);
-	while (!core->queue_thread_kill &&
+	while (!kthread_should_stop() &&
 	       !list_empty(&core->task_queue)) {
 
 		can_queue = sas_ha->lldd_queue_size - core->task_queue_size;
@@ -858,8 +860,6 @@ static void sas_queue(struct sas_ha_stru
 	spin_unlock_irqrestore(&core->task_queue_lock, flags);
 }
 
-static DECLARE_COMPLETION(queue_th_comp);
-
 /**
  * sas_queue_thread -- The Task Collector thread
  * @_sas_ha: pointer to struct sas_ha
@@ -867,40 +867,33 @@ static DECLARE_COMPLETION(queue_th_comp)
 static int sas_queue_thread(void *_sas_ha)
 {
 	struct sas_ha_struct *sas_ha = _sas_ha;
-	struct scsi_core *core = &sas_ha->core;
 
-	daemonize("sas_queue_%d", core->shost->host_no);
 	current->flags |= PF_NOFREEZE;
 
-	complete(&queue_th_comp);
-
 	while (1) {
-		down_interruptible(&core->queue_thread_sema);
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
 		sas_queue(sas_ha);
-		if (core->queue_thread_kill)
+		if (kthread_should_stop())
 			break;
 	}
 
-	complete(&queue_th_comp);
-
 	return 0;
 }
 
 int sas_init_queue(struct sas_ha_struct *sas_ha)
 {
-	int res;
 	struct scsi_core *core = &sas_ha->core;
 
 	spin_lock_init(&core->task_queue_lock);
 	core->task_queue_size = 0;
 	INIT_LIST_HEAD(&core->task_queue);
-	init_MUTEX_LOCKED(&core->queue_thread_sema);
 
-	res = kernel_thread(sas_queue_thread, sas_ha, 0);
-	if (res >= 0)
-		wait_for_completion(&queue_th_comp);
-
-	return res < 0 ? res : 0;
+	core->queue_thread = kthread_run(sas_queue_thread, sas_ha,
+					 "sas_queue_%d", core->shost->host_no);
+	if (IS_ERR(core->queue_thread))
+		return PTR_ERR(core->queue_thread);
+	return 0;
 }
 
 void sas_shutdown_queue(struct sas_ha_struct *sas_ha)
@@ -909,10 +902,7 @@ void sas_shutdown_queue(struct sas_ha_st
 	struct scsi_core *core = &sas_ha->core;
 	struct sas_task *task, *n;
 
-	init_completion(&queue_th_comp);
-	core->queue_thread_kill = 1;
-	up(&core->queue_thread_sema);
-	wait_for_completion(&queue_th_comp);
+	kthread_stop(core->queue_thread);
 
 	if (!list_empty(&core->task_queue))
 		SAS_DPRINTK("HA: %llx: scsi core task queue is NOT empty!?\n",
Index: linux-2.6/include/scsi/libsas.h
===================================================================
--- linux-2.6.orig/include/scsi/libsas.h	2007-04-22 20:32:41.000000000 +0200
+++ linux-2.6/include/scsi/libsas.h	2007-04-22 20:32:59.000000000 +0200
@@ -314,8 +314,7 @@ struct scsi_core {
 	struct list_head  task_queue;
 	int               task_queue_size;
 
-	struct semaphore  queue_thread_sema;
-	int               queue_thread_kill;
+	struct task_struct *queue_thread;
 };
 
 struct sas_ha_event {
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, Apr 22, 2007 at 01:24:53PM +0100, Christoph Hellwig wrote:
> On Thu, Apr 19, 2007 at 12:55:28AM -0600, Eric W. Biederman wrote:
> > From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> > 
> > thread_run is used intead of kernel_thread, daemonize, and mucking
> > around blocking signals directly.
> 
> This is the full conversion I sent to Dave in April 2006, but never got
> any feedback to:

Here's a slightly updated version that corrects the set_current_state
placement as discussed with Dave on irc:


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/mtd/mtd_blkdevs.c
===================================================================
--- linux-2.6.orig/drivers/mtd/mtd_blkdevs.c	2007-01-29 10:03:52.000000000 +0100
+++ linux-2.6/drivers/mtd/mtd_blkdevs.c	2007-04-22 20:39:20.000000000 +0200
@@ -20,6 +20,7 @@
 #include <linux/hdreg.h>
 #include <linux/init.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 #include <asm/uaccess.h>
 
 static LIST_HEAD(blktrans_majors);
@@ -28,9 +29,7 @@ extern struct mutex mtd_table_mutex;
 extern struct mtd_info *mtd_table[];
 
 struct mtd_blkcore_priv {
-	struct completion thread_dead;
-	int exiting;
-	wait_queue_head_t thread_wq;
+	struct task_struct *thread;
 	struct request_queue *rq;
 	spinlock_t queue_lock;
 };
@@ -83,38 +82,19 @@ static int mtd_blktrans_thread(void *arg
 	/* we might get involved when memory gets low, so use PF_MEMALLOC */
 	current->flags |= PF_MEMALLOC | PF_NOFREEZE;
 
-	daemonize("%sd", tr->name);
-
-	/* daemonize() doesn't do this for us since some kernel threads
-	   actually want to deal with signals. We can't just call
-	   exit_sighand() since that'll cause an oops when we finally
-	   do exit. */
-	spin_lock_irq(&current->sighand->siglock);
-	sigfillset(&current->blocked);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
-
 	spin_lock_irq(rq->queue_lock);
-
-	while (!tr->blkcore_priv->exiting) {
+	while (!kthread_should_stop()) {
 		struct request *req;
 		struct mtd_blktrans_dev *dev;
 		int res = 0;
-		DECLARE_WAITQUEUE(wait, current);
 
 		req = elv_next_request(rq);
 
 		if (!req) {
-			add_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
 			set_current_state(TASK_INTERRUPTIBLE);
-
 			spin_unlock_irq(rq->queue_lock);
-
 			schedule();
-			remove_wait_queue(&tr->blkcore_priv->thread_wq, &wait);
-
 			spin_lock_irq(rq->queue_lock);
-
 			continue;
 		}
 
@@ -133,13 +113,13 @@ static int mtd_blktrans_thread(void *arg
 	}
 	spin_unlock_irq(rq->queue_lock);
 
-	complete_and_exit(&tr->blkcore_priv->thread_dead, 0);
+	return 0;
 }
 
 static void mtd_blktrans_request(struct request_queue *rq)
 {
 	struct mtd_blktrans_ops *tr = rq->queuedata;
-	wake_up(&tr->blkcore_priv->thread_wq);
+	wake_up_process(tr->blkcore_priv->thread);
 }
 
 
@@ -388,8 +368,6 @@ int register_mtd_blktrans(struct mtd_blk
 		return ret;
 	}
 	spin_lock_init(&tr->blkcore_priv->queue_lock);
-	init_completion(&tr->blkcore_priv->thread_dead);
-	init_waitqueue_head(&tr->blkcore_priv->thread_wq);
 
 	tr->blkcore_priv->rq = blk_init_queue(mtd_blktrans_request, &tr->blkcore_priv->queue_lock);
 	if (!tr->blkcore_priv->rq) {
@@ -403,13 +381,14 @@ int register_mtd_blktrans(struct mtd_blk
 	blk_queue_hardsect_size(tr->blkcore_priv->rq, tr->blksize);
 	tr->blkshift = ffs(tr->blksize) - 1;
 
-	ret = kernel_thread(mtd_blktrans_thread, tr, CLONE_KERNEL);
-	if (ret < 0) {
+	tr->blkcore_priv->thread = kthread_run(mtd_blktrans_thread, tr,
+			"%sd", tr->name);
+	if (IS_ERR(tr->blkcore_priv->thread)) {
 		blk_cleanup_queue(tr->blkcore_priv->rq);
 		unregister_blkdev(tr->major, tr->name);
 		kfree(tr->blkcore_priv);
 		mutex_unlock(&mtd_table_mutex);
-		return ret;
+		return PTR_ERR(tr->blkcore_priv->thread);
 	}
 
 	INIT_LIST_HEAD(&tr->devs);
@@ -432,9 +411,7 @@ int deregister_mtd_blktrans(struct mtd_b
 	mutex_lock(&mtd_table_mutex);
 
 	/* Clean up the kernel thread */
-	tr->blkcore_priv->exiting = 1;
-	wake_up(&tr->blkcore_priv->thread_wq);
-	wait_for_completion(&tr->blkcore_priv->thread_dead);
+	kthread_stop(tr->blkcore_priv->thread);
 
 	/* Remove it from the list of active majors */
 	list_del(&tr->list);
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 04:24:59PM -0700, Andrew Morton wrote:
> On Thu, 19 Apr 2007 01:58:51 -0600
> "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> 
> > From: Eric W. Biederman <ebiederm@xmission.com>
> > 
> > This patch starts kbenpd using kthread_run replacing
> > a combination of kernel_thread and daemonize.  Making
> > the code a little simpler and more maintainable.
> > 
> >
> 
> 	while (!atomic_read(&s->killed)) {
> 
> ho hum.

Note that this also stands against a full kthread conversion.
Marcel put my old patches for a full kthread conversion in, but
they didn't deal properly with some of the premaure exit cases,
and causes OOPSes.

I don't remember what the problems where, but the case of a thread
terminating earlier and possibly asynchronously is one of the
cases we'll probably have to add to the kthread infrastructure
before all uses of kernel_thread in drivers can be converted.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 03:59:44PM -0700, Andrew Morton wrote:
> There still seems to be quite a lot of complexity in this driver's
> thread handling which could be removed if we did a full conversion 
> to the kthread API.
> 
> It all looks.... surprisingly complex in there.

It is.  There quite a few interesting oddities in this code:

 - creation of a forker thread.  This is superflous when using the
   kthread infrastructure as a thread created by kthread_create
   always comes from our dedicated forker thread.
 - the infinite retry on failure looks very bogus, the system
   doesn't recover very well if you try to fork forever in a loop :)
 - a lot of very overlapping state variables.  My reading of the
   code suggests that both a 'master' and 'backup' thread can
   run at the same time.  I think the code would benefit a lot
   from totally separating these codepathes.
 - start_sync_thread and stop_sync_thread are called with
   unchecked user supplied arguments and bug if they don't
   match the expected values.  While all this is under
   capable(CAP_NET_ADMIN) it still sounds like something to
   fix.
 - and the usual removal of semaphores and completions for
   startup/shutdown would benefit the code a lot, as for most
   thread users.
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

This patch implements the kthread helper functions kthread_start
and kthread_end which make it simple to support a kernel thread
that may decided to exit on it's own before we request it to.
It is still assumed that eventually we will get around to requesting
that the kernel thread stop.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/kthread.h |   23 +++++++++++++++++++++++
 kernel/kthread.c        |   18 ++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index a8ea31d..4f1eff1 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -28,6 +28,29 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 
 void kthread_bind(struct task_struct *k, unsigned int cpu);
 int kthread_stop(struct task_struct *k);
+/**
+ * kthread_start - create and wake a thread.
+ * @threadfn: the function to run until kthread_should_stop().
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * get_task_struct() and wake_up_process. kthread_start should be paired
+ * with kthread_end() so we don't leak task structs.
+ *
+ * Returns the kthread or ERR_PTR(-ENOMEM).
+ */
+#define kthread_start(threadfn, data, namefmt, ...)			   \
+({									   \
+	struct task_struct *__k						   \
+		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						   \
+		get_task_struct(__k);					   \
+		wake_up_process(__k);					   \
+	}								   \
+	__k;								   \
+})
+int kthread_end(struct task_struct *k);
 
 static inline int __kthread_should_stop(struct task_struct *tsk)
 {
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9b3c19f..d6d63c6 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -179,6 +179,24 @@ int kthread_stop(struct task_struct *tsk)
 }
 EXPORT_SYMBOL(kthread_stop);
 
+/**
+ * kthread_end - signal a kthread and wait for it to exit.
+ * @task: The kthread to end.
+ *
+ * Description: Convenient wrapper for kthread_stop() followed by
+ * put_task_struct().  Returns the kthread exit code.
+ *
+ * kthread_start()/kthread_end() can handle kthread that spontaneously exit
+ * before the kthread is requested to terminate.
+ */
+int kthread_end(struct task_struct *task)
+{
+	int ret;
+	ret = kthread_stop(task);
+	put_task_struct(task);
+	return ret;
+}
+EXPORT_SYMBOL(kthread_end);
 
 static __init void kthreadd_setup(void)
 {
-- 
1.5.0.g53756

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 04:12:53PM -0700, Andrew Morton wrote:
> On Thu, 19 Apr 2007 01:58:54 -0600
> "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> 
> > From: Eric W. Biederman <ebiederm@xmission.com>
> > 
> > This patch starts krfcommd using kthread_run instead of a combination
> > of kernel_thread and daemonize making the code slightly simpler
> > and more maintainable.
> 
> gargh, the more I look at these things, the more I agree with Christoph.

Hehe.  Here's a patch to do the full kthread conversion for rfcomm, it
doesn't have the asynchrnous termination issues the other bluetooth drivers
have.  Also handle init failures in rfcomm while we're at it.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/net/bluetooth/rfcomm/core.c
===================================================================
--- linux-2.6.orig/net/bluetooth/rfcomm/core.c	2007-04-22 21:01:31.000000000 +0200
+++ linux-2.6/net/bluetooth/rfcomm/core.c	2007-04-22 21:12:30.000000000 +0200
@@ -37,6 +37,7 @@
 #include <linux/device.h>
 #include <linux/net.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 
 #include <net/sock.h>
 #include <asm/uaccess.h>
@@ -67,7 +68,6 @@ static DEFINE_MUTEX(rfcomm_mutex);
 static unsigned long rfcomm_event;
 
 static LIST_HEAD(session_list);
-static atomic_t terminate, running;
 
 static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
@@ -1846,26 +1846,6 @@ static inline void rfcomm_process_sessio
 	rfcomm_unlock();
 }
 
-static void rfcomm_worker(void)
-{
-	BT_DBG("");
-
-	while (!atomic_read(&terminate)) {
-		if (!test_bit(RFCOMM_SCHED_WAKEUP, &rfcomm_event)) {
-			/* No pending events. Let's sleep.
-			 * Incoming connections and data will wake us up. */
-			set_current_state(TASK_INTERRUPTIBLE);
-			schedule();
-		}
-
-		/* Process stuff */
-		clear_bit(RFCOMM_SCHED_WAKEUP, &rfcomm_event);
-		rfcomm_process_sessions();
-	}
-	set_current_state(TASK_RUNNING);
-	return;
-}
-
 static int rfcomm_add_listener(bdaddr_t *ba)
 {
 	struct sockaddr_l2 addr;
@@ -1931,23 +1911,27 @@ static void rfcomm_kill_listener(void)
 
 static int rfcomm_run(void *unused)
 {
-	rfcomm_thread = current;
-
-	atomic_inc(&running);
-
-	daemonize("krfcommd");
 	set_user_nice(current, -10);
 	current->flags |= PF_NOFREEZE;
 
 	BT_DBG("");
 
 	rfcomm_add_listener(BDADDR_ANY);
+	while (!kthread_should_stop()) {
+		if (!test_bit(RFCOMM_SCHED_WAKEUP, &rfcomm_event)) {
+			/* No pending events. Let's sleep.
+			 * Incoming connections and data will wake us up. */
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+		}
 
-	rfcomm_worker();
-
+		/* Process stuff */
+		clear_bit(RFCOMM_SCHED_WAKEUP, &rfcomm_event);
+		rfcomm_process_sessions();
+	}
+	set_current_state(TASK_RUNNING);
 	rfcomm_kill_listener();
 
-	atomic_dec(&running);
 	return 0;
 }
 
@@ -2052,24 +2036,52 @@ static CLASS_ATTR(rfcomm_dlc, S_IRUGO, r
 /* ---- Initialization ---- */
 static int __init rfcomm_init(void)
 {
+	int err;
+
 	l2cap_load();
 
-	hci_register_cb(&rfcomm_cb);
+	err = hci_register_cb(&rfcomm_cb);
+	if (err)
+		goto out;
 
-	kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+	rfcomm_thread = kthread_run(rfcomm_run, NULL, "krfcommd");
+	if (IS_ERR(rfcomm_thread)) {
+		err = PTR_ERR(rfcomm_thread);
+		goto out_unregister_hci;
+	}
 
-	if (class_create_file(bt_class, &class_attr_rfcomm_dlc) < 0)
+	err = class_create_file(bt_class, &class_attr_rfcomm_dlc);
+	if (err < 0) {
 		BT_ERR("Failed to create RFCOMM info file");
+		goto out_kthread_stop;
+	}
 
-	rfcomm_init_sockets();
+	err = rfcomm_init_sockets();
+	if (err)
+		goto out_remove_sysfs_files;
 
 #ifdef CONFIG_BT_RFCOMM_TTY
-	rfcomm_init_ttys();
+	err = rfcomm_init_ttys();
+	if (err)
+		goto out_cleanup_sockets;
 #endif
 
 	BT_INFO("RFCOMM ver %s", VERSION);
 
 	return 0;
+
+#ifdef CONFIG_BT_RFCOMM_TTY
+ out_cleanup_sockets:
+	rfcomm_cleanup_sockets();
+#endif
+ out_remove_sysfs_files:
+	class_remove_file(bt_class, &class_attr_rfcomm_dlc);
+ out_unregister_hci:
+	hci_unregister_cb(&rfcomm_cb);
+ out_kthread_stop:
+	kthread_stop(rfcomm_thread);
+ out:
+	return err;
 }
 
 static void __exit rfcomm_exit(void)
@@ -2077,15 +2089,7 @@ static void __exit rfcomm_exit(void)
 	class_remove_file(bt_class, &class_attr_rfcomm_dlc);
 
 	hci_unregister_cb(&rfcomm_cb);
-
-	/* Terminate working thread.
-	 * ie. Set terminate flag and wake it up */
-	atomic_inc(&terminate);
-	rfcomm_schedule(RFCOMM_SCHED_STATE);
-
-	/* Wait until thread is running */
-	while (atomic_read(&running))
-		schedule();
+	kthread_stop(rfcomm_thread);
 
 #ifdef CONFIG_BT_RFCOMM_TTY
 	rfcomm_cleanup_ttys();
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 01:58:42AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> Modify zfcperp%s to be started with kthread_run not
> a combination of kernel_thread, daemonize and siginitsetinv
> making the code slightly simpler and more maintainable.

This driver would also benefit from a full kthread conversion.
Unfortunately it has a strange dual-use semaphore (->erp_ready_sem)
that hinders a straight conversion.  Maybe the maintainer can take
a look whether there's a nice way to get rid of that one?

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 01:58:43AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> This patch modifies the startup of kecardd to use
> kthread_run not a kernel_thread combination of kernel_thread
> and daemonize.  Making the code slightly simpler and more
> maintainable.

Looks good.  Given that this is non-modular and there's no
exit function there is no need for further action.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, Apr 19, 2007 at 01:58:44AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> This patch starts the xpc kernel threads using kthread_run
> not a combination of kernel_thread and daemonize.  Resuling
> in slightly simpler and more maintainable code.

This driver is a really twisted maze.  It has a lot of threads,
some of them running through the whole lifetime of the driver,
some short-lived and some in a sort of a pool.

The patch below fixes up the long-lived thread as well as fixing
gazillions of leaks in the init routine by switching to proper
goto-based unwinding.

Note that thread pools are something we have in a few places,
and might be worth handling in the core kthread infrastructure,
as tearing down pools will get a bit complicated using the
kthread APIs.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/arch/ia64/sn/kernel/xpc_main.c
===================================================================
--- linux-2.6.orig/arch/ia64/sn/kernel/xpc_main.c	2007-04-22 21:19:22.000000000 +0200
+++ linux-2.6/arch/ia64/sn/kernel/xpc_main.c	2007-04-22 21:33:54.000000000 +0200
@@ -55,6 +55,7 @@
 #include <linux/delay.h>
 #include <linux/reboot.h>
 #include <linux/completion.h>
+#include <linux/kthread.h>
 #include <asm/sn/intr.h>
 #include <asm/sn/sn_sal.h>
 #include <asm/kdebug.h>
@@ -159,16 +160,14 @@ static struct ctl_table_header *xpc_sysc
 int xpc_disengage_request_timedout;
 
 /* #of IRQs received */
-static atomic_t xpc_act_IRQ_rcvd;
+static atomic_t xpc_act_IRQ_rcvd = ATOMIC_INIT(0);
 
 /* IRQ handler notifies this wait queue on receipt of an IRQ */
 static DECLARE_WAIT_QUEUE_HEAD(xpc_act_IRQ_wq);
 
+static struct task_struct *xpc_hb_checker_thread;
 static unsigned long xpc_hb_check_timeout;
 
-/* notification that the xpc_hb_checker thread has exited */
-static DECLARE_COMPLETION(xpc_hb_checker_exited);
-
 /* notification that the xpc_discovery thread has exited */
 static DECLARE_COMPLETION(xpc_discovery_exited);
 
@@ -250,17 +249,10 @@ xpc_hb_checker(void *ignore)
 	int new_IRQ_count;
 	int force_IRQ=0;
 
-
 	/* this thread was marked active by xpc_hb_init() */
-
-	daemonize(XPC_HB_CHECK_THREAD_NAME);
-
-	set_cpus_allowed(current, cpumask_of_cpu(XPC_HB_CHECK_CPU));
-
 	xpc_hb_check_timeout = jiffies + (xpc_hb_check_interval * HZ);
 
-	while (!(volatile int) xpc_exiting) {
-
+	while (!kthread_should_stop()) {
 		dev_dbg(xpc_part, "woke up with %d ticks rem; %d IRQs have "
 			"been received\n",
 			(int) (xpc_hb_check_timeout - jiffies),
@@ -304,14 +296,10 @@ xpc_hb_checker(void *ignore)
 		(void) wait_event_interruptible(xpc_act_IRQ_wq,
 			    (last_IRQ_count < atomic_read(&xpc_act_IRQ_rcvd) ||
 					jiffies >= xpc_hb_check_timeout ||
-						(volatile int) xpc_exiting));
+					kthread_should_stop()));
 	}
 
 	dev_dbg(xpc_part, "heartbeat checker is exiting\n");
-
-
-	/* mark this thread as having exited */
-	complete(&xpc_hb_checker_exited);
 	return 0;
 }
 
@@ -966,9 +954,7 @@ xpc_do_exit(enum xpc_retval reason)
 	/* wait for the discovery thread to exit */
 	wait_for_completion(&xpc_discovery_exited);
 
-	/* wait for the heartbeat checker thread to exit */
-	wait_for_completion(&xpc_hb_checker_exited);
-
+	kthread_stop(xpc_hb_checker_thread);
 
 	/* sleep for a 1/3 of a second or so */
 	(void) msleep_interruptible(300);
@@ -1219,29 +1205,29 @@ xpc_system_die(struct notifier_block *nb
 int __init
 xpc_init(void)
 {
-	int ret;
+	int ret = -ENODEV;
 	partid_t partid;
 	struct xpc_partition *part;
 	pid_t pid;
 	size_t buf_size;
 
+	if (!ia64_platform_is("sn2"))
+		goto out;
 
-	if (!ia64_platform_is("sn2")) {
-		return -ENODEV;
-	}
-
-
+	ret = -ENOMEM;
 	buf_size = max(XPC_RP_VARS_SIZE,
 				XPC_RP_HEADER_SIZE + XP_NASID_MASK_BYTES);
 	xpc_remote_copy_buffer = xpc_kmalloc_cacheline_aligned(buf_size,
 				     GFP_KERNEL, &xpc_remote_copy_buffer_base);
-	if (xpc_remote_copy_buffer == NULL)
-		return -ENOMEM;
+	if (!xpc_remote_copy_buffer)
+		goto out;
 
 	snprintf(xpc_part->bus_id, BUS_ID_SIZE, "part");
 	snprintf(xpc_chan->bus_id, BUS_ID_SIZE, "chan");
 
 	xpc_sysctl = register_sysctl_table(xpc_sys_dir);
+	if (!xpc_sysctl)
+		goto out_free_remote_buffer;
 
 	/*
 	 * The first few fields of each entry of xpc_partitions[] need to
@@ -1278,12 +1264,6 @@ xpc_init(void)
 	xpc_allow_IPI_ops();
 
 	/*
-	 * Interrupts being processed will increment this atomic variable and
-	 * awaken the heartbeat thread which will process the interrupts.
-	 */
-	atomic_set(&xpc_act_IRQ_rcvd, 0);
-
-	/*
 	 * This is safe to do before the xpc_hb_checker thread has started
 	 * because the handler releases a wait queue.  If an interrupt is
 	 * received before the thread is waiting, it will not go to sleep,
@@ -1294,15 +1274,7 @@ xpc_init(void)
 	if (ret != 0) {
 		dev_err(xpc_part, "can't register ACTIVATE IRQ handler, "
 			"errno=%d\n", -ret);
-
-		xpc_restrict_IPI_ops();
-
-		if (xpc_sysctl) {
-			unregister_sysctl_table(xpc_sysctl);
-		}
-
-		kfree(xpc_remote_copy_buffer_base);
-		return -EBUSY;
+		goto out_restrict_IPI_ops;
 	}
 
 	/*
@@ -1313,29 +1285,23 @@ xpc_init(void)
 	xpc_rsvd_page = xpc_rsvd_page_init();
 	if (xpc_rsvd_page == NULL) {
 		dev_err(xpc_part, "could not setup our reserved page\n");
-
-		free_irq(SGI_XPC_ACTIVATE, NULL);
-		xpc_restrict_IPI_ops();
-
-		if (xpc_sysctl) {
-			unregister_sysctl_table(xpc_sysctl);
-		}
-
-		kfree(xpc_remote_copy_buffer_base);
-		return -EBUSY;
+		ret = -ENOMEM;
+		goto out_free_irq;
 	}
 
 
 	/* add ourselves to the reboot_notifier_list */
 	ret = register_reboot_notifier(&xpc_reboot_notifier);
 	if (ret != 0) {
-		dev_warn(xpc_part, "can't register reboot notifier\n");
+		dev_err(xpc_part, "can't register reboot notifier\n");
+		goto out_free_rsvd_page;
 	}
 
 	/* add ourselves to the die_notifier list (i.e., ia64die_chain) */
 	ret = register_die_notifier(&xpc_die_notifier);
 	if (ret != 0) {
-		dev_warn(xpc_part, "can't register die notifier\n");
+		dev_err(xpc_part, "can't register die notifier\n");
+		goto out_unregister_reboot_notifier;
 	}
 
 
@@ -1353,31 +1319,16 @@ xpc_init(void)
 	 * The real work-horse behind xpc.  This processes incoming
 	 * interrupts and monitors remote heartbeats.
 	 */
-	pid = kernel_thread(xpc_hb_checker, NULL, 0);
-	if (pid < 0) {
+	xpc_hb_checker_thread = kthread_create(xpc_hb_checker, NULL,
+						XPC_HB_CHECK_THREAD_NAME);
+	if (IS_ERR(xpc_hb_checker_thread)) {
 		dev_err(xpc_part, "failed while forking hb check thread\n");
-
-		/* indicate to others that our reserved page is uninitialized */
-		xpc_rsvd_page->vars_pa = 0;
-
-		/* take ourselves off of the reboot_notifier_list */
-		(void) unregister_reboot_notifier(&xpc_reboot_notifier);
-
-		/* take ourselves off of the die_notifier list */
-		(void) unregister_die_notifier(&xpc_die_notifier);
-
-		del_timer_sync(&xpc_hb_timer);
-		free_irq(SGI_XPC_ACTIVATE, NULL);
-		xpc_restrict_IPI_ops();
-
-		if (xpc_sysctl) {
-			unregister_sysctl_table(xpc_sysctl);
-		}
-
-		kfree(xpc_remote_copy_buffer_base);
-		return -EBUSY;
+		ret = PTR_ERR(xpc_hb_checker_thread);
+		goto out_del_hb_timer;
 	}
 
+	kthread_bind(xpc_hb_checker_thread, XPC_HB_CHECK_CPU);
+	wake_up_process(xpc_hb_checker_thread);
 
 	/*
 	 * Startup a thread that will attempt to discover other partitions to
@@ -1403,6 +1354,29 @@ xpc_init(void)
 			  xpc_initiate_partid_to_nasids);
 
 	return 0;
+
+	if (ret != 0) {
+		dev_err(xpc_part, "can't register reboot notifier\n");
+		goto out_free_rsvd_page;
+	}
+
+ out_del_hb_timer:
+	unregister_die_notifier(&xpc_die_notifier);
+ out_unregister_reboot_notifier:
+	unregister_reboot_notifier(&xpc_reboot_notifier);
+ out_free_rsvd_page:
+	/* indicate to others that our reserved page is uninitialized */
+	xpc_rsvd_page->vars_pa = 0;
+	/* XXX(hch): xpc_rsvd_page gets leaked */
+ out_free_irq:
+	free_irq(SGI_XPC_ACTIVATE, NULL);
+ out_restrict_IPI_ops:
+	xpc_restrict_IPI_ops();
+	unregister_sysctl_table(xpc_sysctl);
+ out_free_remote_buffer:
+	kfree(xpc_remote_copy_buffer_base);
+ out:
+	return -EBUSY;
 }
 module_init(xpc_init);
 
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Christoph Hellwig writes:

> Why is this driver using a thread at all?  It's only doing a bunch
> of rather short-lived things in the thread.

It's doing i2c reads and writes, which block, and are actually quite
slow.

Paul.
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> We only care when del_timer() returns true. In that case, if the timer
> function still runs (possible for single-threaded wqs), it has already
> passed __queue_work().

Why do you assume that?

David
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Jes Sorensen <jes@sgi.com> writes:

>
> Like with the previous patch from Eric, I'm CC'ing the correct people
> for this patch (forwarded it in a seperate email). CC'ing irrelevant
> lists such as containers@ and not linux-ia64@ makes it somewhat
> difficult to get proper reviews of these things.

containers is actually relevant because everything not being converted
to a kthread API is actually show stopper issue for the development of
the pid namespace because of the usage of pids by kernel_thread, and
the apparent impossibility to fix daemonize to sort this out.

Now I do agree linux-ia64 is also relevant.

> Russ/Dean/Robin - could one of you provide some feedback to this one
> please.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/23, Christoph Hellwig wrote:
>>
>> On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
>> > 
>> > This patch implements the kthread helper functions kthread_start
>> > and kthread_end which make it simple to support a kernel thread
>> > that may decided to exit on it's own before we request it to.
>> > It is still assumed that eventually we will get around to requesting
>> > that the kernel thread stop.
>> 
>> I don't think having to parallel APIs is a good idea, people will
>> get utterly confused which one to use.  Better always grab a reference
>> in kthread_create and drop it in kthread_stop.  For normal thread
>> no change in behaviour and only slightly more code in the slowpath.
>> 
>> Of course it will need an audit for half-assed kthread conversion
>> first to avoid task_struct reference count leaks.
>
> In that case it is better to grab a reference in kthread(). This also
> close the race when a new thread is woken (freezer) and exits before
> kthread_create() does get_task_struct().
>
>> In addition to that kthrad_end implementation look wrong. When
>> the kthread has exited prematurely no one will call complete
>> on kthread_stop_info.done before it's been setup.
>
> This is not true anymore, see another patch from Eric
>
> 	kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch

Ok.  Thinking about it I agree with Christoph that parallel API's can
be a problem.

However we do still need to support kernel threads where kthread_stop will
never be called.  There appear to be a few legitimate cases where
someone wants to fire off a thread and have it do some work but don't
care at all for stopping it before it is done.

So I propose we add a kthread_orphan as a basic primitive to decrement the
count on the task_struct if we want a kthread to simply exit after it
has done some work.

And as a helper function we can have a kthread_run_orphan.

I think having a kthread_orphan will document what we are doing better
and make it easy to find kernel threads that don't use kthread_stop.

The pain is that this requires an audit of all kernel kthread creators
so that we call kthread_orphan on the right ones, or else we will have
a task_struct leak.  At least that isn't a fatal condition.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, 22 Apr 2007, Christoph Hellwig wrote:

> On Thu, Apr 19, 2007 at 12:55:29AM -0600, Eric W. Biederman wrote:
> > From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
> > 
> > kthread_run replaces the kernel_thread and  daemonize calls
> > during thread startup.
> > 
> > Calls to signal_pending were also removed as it is currently
> > impossible for the cpci_hotplug thread to receive signals.
> 
> This drivers thread are a bit of a miss, although a lot better than
> most other pci hotplug drivers :)

Heh, I guess I'll take that as a compliment. :)
 
> Below is more complete conversion to the kthread infrastructure +
> wake_up_process to wake the thread.  Note that we had to keep
> a thread_finished variable because the existing one had dual use.
[snip]

I'm out of the office today, but I'll give it a spin on a test setup 
tomorrow.

Thanks,

Scott


-- 
Scott Murray
SOMA Networks, Inc.
Toronto, Ontario
e-mail: scottm@somanetworks.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
> 
> This patch implements the kthread helper functions kthread_start
> and kthread_end which make it simple to support a kernel thread
> that may decided to exit on it's own before we request it to.
> It is still assumed that eventually we will get around to requesting
> that the kernel thread stop.

I don't think having to parallel APIs is a good idea, people will
get utterly confused which one to use.  Better always grab a reference
in kthread_create and drop it in kthread_stop.  For normal thread
no change in behaviour and only slightly more code in the slowpath.

Of course it will need an audit for half-assed kthread conversion
first to avoid task_struct reference count leaks.

In addition to that kthrad_end implementation look wrong. When
the kthread has exited prematurely no one will call complete
on kthread_stop_info.done before it's been setup.  Interestingly
the comment there indicates someone thought about threads exiting
early, but it became defunkt during all the rewrites of the
kthread code.


> +/**
> + * kthread_start - create and wake a thread.
> + * @threadfn: the function to run until kthread_should_stop().
> + * @data: data ptr for @threadfn.
> + * @namefmt: printf-style name for the thread.
> + *
> + * Description: Convenient wrapper for kthread_create() followed by
> + * get_task_struct() and wake_up_process. kthread_start should be paired
> + * with kthread_end() so we don't leak task structs.
> + *
> + * Returns the kthread or ERR_PTR(-ENOMEM).
> + */
> +#define kthread_start(threadfn, data, namefmt, ...)			   \
> +({									   \
> +	struct task_struct *__k						   \
> +		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
> +	if (!IS_ERR(__k)) {						   \
> +		get_task_struct(__k);					   \
> +		wake_up_process(__k);					   \
> +	}								   \
> +	__k;								   \
> +})
> +int kthread_end(struct task_struct *k);
>  
>  static inline int __kthread_should_stop(struct task_struct *tsk)
>  {
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 9b3c19f..d6d63c6 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -179,6 +179,24 @@ int kthread_stop(struct task_struct *tsk)
>  }
>  EXPORT_SYMBOL(kthread_stop);
>  
> +/**
> + * kthread_end - signal a kthread and wait for it to exit.
> + * @task: The kthread to end.
> + *
> + * Description: Convenient wrapper for kthread_stop() followed by
> + * put_task_struct().  Returns the kthread exit code.
> + *
> + * kthread_start()/kthread_end() can handle kthread that spontaneously exit
> + * before the kthread is requested to terminate.
> + */
> +int kthread_end(struct task_struct *task)
> +{
> +	int ret;
> +	ret = kthread_stop(task);
> +	put_task_struct(task);
> +	return ret;
> +}
> +EXPORT_SYMBOL(kthread_end);
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Jes Sorensen wrote:
> 
> Russ/Dean/Robin - could one of you provide some feedback to this one
> please.

Dean's on vacation for a couple days and will test it when he gets back.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sun, Apr 22, 2007 at 01:31:55PM +0100, Christoph Hellwig wrote:
> On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote:
> > From: Eric W. Biederman <ebiederm@xmission.com>
> > 
> > This patch modifies the startup of eehd to use kthread_run
> > not a combination of kernel_thread and daemonize.  Making
> > the code slightly simpler and more maintainable.
> 
> This one has the same scheme as the various s390 drivers where a thread
> is spawned using a workqueue on demand.  I think we should not blindly
> convert it but think a litte more about it.
> 
> The first question is obviously, is this really something we want?
> spawning kernel thread on demand without reaping them properly seems
> quite dangerous.

I'm not quite sure what the intent of this patch really is, being
at most a somewhat passing and casual user of kernel threads.

Some background may be useful: (this in reply to some comments from
Andrew Morton)

EEH events are supposed to be very rare, as they correspond to 
hardware failures, typically PCI bus parity errors, but also 
things like wild DMA's.  The code that generates these will limit
them to no more than 6 per hour per pci device. Any more than that,
and the PCI device is permanently disabled (the sysadmin would 
need to do something to recover).

The only reason for using threads here is to get the error recovery
out of an interrupt context (where errors may be detected), and then,
an hour later, decrement a counter (which is how we limit these to 
6 per hour). Thread reaping is "trivial", the thread just exits
after an hour.

Since these are events rare, I've no particular concern about
performance or resource consumption. The current code seems 
to work just fine. :-)

--linas
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Mon, Apr 23, 2007 at 11:45:51AM -0600, Eric W. Biederman wrote:
> Ok.  Thinking about it I agree with Christoph that parallel API's can
> be a problem.
> 
> However we do still need to support kernel threads where kthread_stop will
> never be called.  There appear to be a few legitimate cases where
> someone wants to fire off a thread and have it do some work but don't
> care at all for stopping it before it is done.

There's two cases where it's valid that kthread_stop is not called:

 a) the user is always builtin and the thread runs until the kernel halts.
    examples: voyager, arm ecard
 b) the thread is normally started/stopped, e.g. at module_init/module_exit
    but there is some reason why it could terminate earlier.
    examples:  the various bluetooth threads, nfs-related threads that
    can be killed using signals
 c) we have some kind of asynchronous helper thread.
    examples: various s390 drivers, usbatm, therm_pm72
 d) a driver has threadpools were we need to start/stop threads on demand.
    examples: nfsd, xpc


case a)
	is trivial, we can just ignore the refcounting issue.

case b)
	is what refcounting the task struct and proper handling in
	kthread_stop will deal with.

case c)
	should get a new kthread_create_async api which starts a thread
	without blocking, so we can get rid of the workqueues in the
	s390 drivers.  it should probably also be safe to be called from
	irq context.  What makes this a bit complicated is the need to
	make sure no more thread is running in case the caller terminates
	(shutdown of the structure it's associated with or module removal)

case d)
	should be deal with with a kthread_pool api

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Christoph Hellwig wrote:
> On Thu, Apr 19, 2007 at 01:58:44AM -0600, Eric W. Biederman wrote:
>> From: Eric W. Biederman <ebiederm@xmission.com>
>>
>> This patch starts the xpc kernel threads using kthread_run
>> not a combination of kernel_thread and daemonize.  Resuling
>> in slightly simpler and more maintainable code.
> 
> This driver is a really twisted maze.  It has a lot of threads,
> some of them running through the whole lifetime of the driver,
> some short-lived and some in a sort of a pool.
> 
> The patch below fixes up the long-lived thread as well as fixing
> gazillions of leaks in the init routine by switching to proper
> goto-based unwinding.
> 
> Note that thread pools are something we have in a few places,
> and might be worth handling in the core kthread infrastructure,
> as tearing down pools will get a bit complicated using the
> kthread APIs.

Like with the previous patch from Eric, I'm CC'ing the correct people
for this patch (forwarded it in a seperate email). CC'ing irrelevant
lists such as containers@ and not linux-ia64@ makes it somewhat
difficult to get proper reviews of these things.

Russ/Dean/Robin - could one of you provide some feedback to this one
please.

Thanks,
Jes
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

>> The only reason for using threads here is to get the error recovery
>> out of an interrupt context (where errors may be detected), and then,
>> an hour later, decrement a counter (which is how we limit these to 
>> 6 per hour). Thread reaping is "trivial", the thread just exits
>> after an hour.
>
> In addition, it should be a thread and not done from within keventd
> because :
>
>  - It can take a long time (well, relatively but still too long for a
> work queue)
>
>  - The driver callbacks might need to use keventd or do flush_workqueue
> to synchronize with their own workqueues when doing an internal
> recovery.
>
>> Since these are events rare, I've no particular concern about
>> performance or resource consumption. The current code seems 
>> to work just fine. :-)
>
> I think moving to kthread's is cleaner (just a wrapper around kernel
> threads that simplify dealing with reaping them out mostly) and I agree
> with Christoph that it would be nice to be able to "fire off" kthreads
> from interrupt context.. in many cases, we abuse work queues for things
> that should really done from kthreads instead (basically anything that
> takes more than a couple hundred microsecs or so).

On that note does anyone have a problem is we manage the irq spawning
safe kthreads the same way that we manage the work queue entries.

i.e. by a structure allocated by the caller?

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On 04/23, Christoph Hellwig wrote:
>
> On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
> > 
> > This patch implements the kthread helper functions kthread_start
> > and kthread_end which make it simple to support a kernel thread
> > that may decided to exit on it's own before we request it to.
> > It is still assumed that eventually we will get around to requesting
> > that the kernel thread stop.
> 
> I don't think having to parallel APIs is a good idea, people will
> get utterly confused which one to use.  Better always grab a reference
> in kthread_create and drop it in kthread_stop.  For normal thread
> no change in behaviour and only slightly more code in the slowpath.
> 
> Of course it will need an audit for half-assed kthread conversion
> first to avoid task_struct reference count leaks.

In that case it is better to grab a reference in kthread(). This also
close the race when a new thread is woken (freezer) and exits before
kthread_create() does get_task_struct().

> In addition to that kthrad_end implementation look wrong. When
> the kthread has exited prematurely no one will call complete
> on kthread_stop_info.done before it's been setup.

This is not true anymore, see another patch from Eric

	kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch

Oleg.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On 04/23, David Howells wrote:
> 
> > We only care when del_timer() returns true. In that case, if the timer
> > function still runs (possible for single-threaded wqs), it has already
> > passed __queue_work().
> 
> Why do you assume that?

If del_timer() returns true, the timer was pending. This means it was started
by work->func() (note that __run_timers() clears timer_pending() before calling
timer->function). This in turn means that delayed_work_timer_fn() has already
called __queue_work(dwork), otherwise work->func() has no chance to run.

When del_timer() returns true and delayed_work_timer_fn() doesn't run we are
safe, this doesn't differ from del_timer_sync().

Oleg.

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Not sure... I can see places where I might want to spawn an arbitrary
> number of these without having to preallocate structures... and if I
> allocate on the fly, then I need a way to free that structure when the
> kthread is reaped which I don't think we have currently, do we ? (In
> fact, I could use that for other things too now that I'm thinking of
> it ... I might have a go at providing optional kthread destructors).

Well the basic problem is that for any piece of code that can be modular
we need a way to ensure all threads it has running are shutdown when we
remove the module.

Which means a fire and forget model however simple is unfortunately
the wrong thing.

Now we might be able to wrap this in some kind of manager construct,
so you don't have to manage each thread individually, but we still
have the problem of ensuring all of the threads exit when we terminate
the module.

Further in general it doesn't make sense to grab a module reference
and call that sufficient because we would like to request that the
module exits.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers