login
Search
The following patches are against 2.6.21.rc6-mm1. Hopefully that is enough to catch most of the recent development activity. I am aiming to remove all kernel threads that handle signals from user space, to remove all calls to daemonize and kernel_thread from non-core kernel code. kernel thrreads handling signals from user space is a problem because it makes the kernel thread part of the user/kernel API which make changing things difficult and it breaks as soon as you are inside of a pid namespace because you won't be able to see your kernel thread. Calling kernel_thread has problems because it returns a pid_t value which once we get to the pid namespace is context depending so it cannot be used to globally identify a process. kernel_thread is also a problem because it traps user space state and requires us to call daemonize to free that state. daemonize is a maintenance problem because every time you play with user space state and limiting things you need to remember to update daemonize. Occasionally it has taken years like in the case of the mount namespace before someone realizes they need to update it. With the kthread api we no longer need daemonize. In addition we don't want kernel threads visible in anything but the initial pid namespace or they will hold a reference to a child pid namespace. However calling kernel_thread from a non-kernel parent in a child pid namespace will give the thread a pid in the child pid namespace, and there is nothing daemonize can do about it. So daemonize appears impossible to support going forward, and I choose to remove all of it's callers rather than attempt to support it. Eric -
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.
Cc: Jyoti Shah <jshah@us.ibm.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pci/hotplug/ibmphp_hpc.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/hotplug/ibmphp_hpc.c b/drivers/pci/hotplug/ibmphp_hpc.c
index 46abaa8..27e12f1 100644
--- a/drivers/pci/hotplug/ibmphp_hpc.c
+++ b/drivers/pci/hotplug/ibmphp_hpc.c
@@ -34,6 +34,7 @@
#include <linux/pci.h>
#include <linux/init.h>
#include <linux/mutex.h>
+#include <linux/kthread.h>
#include "ibmphp.h"
@@ -101,7 +102,6 @@ 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
// access to data structures
@@ -137,7 +137,6 @@ void __init ibmphp_hpc_initvars (void)
init_MUTEX_LOCKED (&sem_exit);
to_debug = 0;
ibmphp_shutdown = 0;
- tid_poll = 0;
debug ("%s - Exit\n", __FUNCTION__);
}
@@ -1060,12 +1059,8 @@ 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;
}
@@ -1078,17 +1073,18 @@ static int hpc_poll_thread (void *data)
*---------------------------------------------------------------------*/
int __init ibmphp_hpc_start_poll_thread (void)
{
+ struct task_struct *task;
int rc = 0;
debug ("%s - Entry\n", __FUNCTION__);
- tid_poll = kernel_thread (hpc_poll_thread, NULL, 0);
- if (tid_poll < 0) {
+ task = kthread_run(hpc_poll_thread, NULL, ...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 ...
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch just trivially replaces kernel_thread and daemonize
with a single call to kthread_run.
CC: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/i386/mach-voyager/voyager_thread.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/i386/mach-voyager/voyager_thread.c b/arch/i386/mach-voyager/voyager_thread.c
index ebfd913..ee23d9b 100644
--- a/arch/i386/mach-voyager/voyager_thread.c
+++ b/arch/i386/mach-voyager/voyager_thread.c
@@ -23,6 +23,7 @@
#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>
@@ -43,7 +44,7 @@ static __u8 set_timeout = 0;
static int __init
voyager_thread_start(void)
{
- if(kernel_thread(thread, NULL, CLONE_KERNEL) < 0) {
+ if (IS_ERR(kthread_run(thread, NULL, "%s", THREAD_NAME))) {
/* This is serious, but not fatal */
printk(KERN_ERR "Voyager: Failed to create system monitor thread!!!\n");
return 1;
@@ -122,8 +123,6 @@ thread(void *unused)
kvoyagerd_running = 1;
- daemonize(THREAD_NAME);
-
set_timeout = 0;
init_timer(&wakeup_timer);
--
1.5.0.g53756
-
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 ...
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted It is my goal to replace all kernel code that handles signals from user space, calls kernel_thread or calls daemonize. All of which the kthread_api makes unncessary. Handling signals from user space is a maintenance problem becuase using a kernel thread is an implementation detail and if user space cares it does not allow us to change the implementation. Calling daemonize is a problem because it has to undo a continually changing set of state generated by user space, requiring the implemetation to change continually. kernel_thread is a problem because it returns a pid_t value. Numeric pids are inherently racy and in the presence of a pid namespace they are no longer global making them useless for general use in the kernel. So this patch renames the pid member of struct saa7134_thread started and changes it's type from pid_t to int. All it has ever been used for is to detect if the kernel thread is has been started so this works. allow_signal(SIGTERM) and the calls to signal_pending have been removed they are needed for the driver to operation. The startup of tvaudio_thread and tvaudio_thread_dep have been modified to use kthread_run instead of a combination of kernel_thread and daemonize. The result is code that is slightly simpler and more maintainable. Cc: Hartmut Hackmann <hartmut.hackmann@t-online.de> Cc: Mauro Carvalho Chehab <mchehab@infradead.org> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- drivers/media/video/saa7134/saa7134-tvaudio.c | 27 ++++++++++++------------- drivers/media/video/saa7134/saa7134.h | 2 +- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/drivers/media/video/saa7134/saa7134-tvaudio.c b/drivers/media/video/saa7134/saa7134-tvaudio.c index 7b56041..b636cb1 100644 --- a/drivers/media/video/saa7134/saa7134-tvaudio.c +++ b/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -27,6 +27,7 @@ #include <linux/kernel.h> #include ...
Here's a refreshed attempt using kthread_should_stop(). Unfortunately, not tested bc we don't have the hardware. cheers, C. From: Sukadev Bhattiprolu <sukadev@us.ibm.com> Replace kernel_thread() with kthread_run() since kernel_thread() is deprecated in drivers/modules. Also remove signalling code as it is not needed in the driver. Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> Cc: Mauro Carvalho Chehab <mchehab@infradead.org> Cc: Containers@lists.osdl.org Cc: video4linux-list@redhat.com Cc: v4l-dvb-maintainer@linuxtv.org --- drivers/media/video/saa7134/saa7134-tvaudio.c | 45 +++++++++++++------------- drivers/media/video/saa7134/saa7134.h | 4 -- 2 files changed, 24 insertions(+), 25 deletions(-) Index: 2.6.21-rc6-mm1/drivers/media/video/saa7134/saa7134.h =================================================================== --- 2.6.21-rc6-mm1.orig/drivers/media/video/saa7134/saa7134.h +++ 2.6.21-rc6-mm1/drivers/media/video/saa7134/saa7134.h @@ -324,10 +324,8 @@ struct saa7134_pgtable { /* tvaudio thread status */ struct saa7134_thread { - pid_t pid; - struct completion exit; + struct task_struct * task; wait_queue_head_t wq; - unsigned int shutdown; unsigned int scan1; unsigned int scan2; unsigned int mode; Index: 2.6.21-rc6-mm1/drivers/media/video/saa7134/saa7134-tvaudio.c =================================================================== --- 2.6.21-rc6-mm1.orig/drivers/media/video/saa7134/saa7134-tvaudio.c +++ 2.6.21-rc6-mm1/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -27,6 +27,7 @@ #include <linux/kernel.h> #include <linux/slab.h> #include <linux/delay.h> +#include <linux/kthread.h> #include <asm/div64.h> #include "saa7134-reg.h" @@ -344,16 +345,22 @@ static int tvaudio_sleep(struct saa7134_ DECLARE_WAITQUEUE(wait, current); ...
I have a patch for this one flying around somewhere aswell. It's trivial to kill the waitqueue and just use wake_up_process, otherwise it looks pretty similar. -
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted To start the nfsv4-delegreturn thread this patch uses kthread_run instead of a combination of kernel_thread and daemonize. In addition allow_signal(SIGKILL) is removed from the expire delegations thread. Cc: Neil Brown <neilb@suse.de> Cc: Trond Myklebust <trond.myklebust@fys.uio.no> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/nfs/delegation.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-) diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index 841c99a..7b9b88c 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -232,7 +232,6 @@ int nfs_do_expire_all_delegations(void *ptr) struct nfs_delegation *delegation; struct inode *inode; - allow_signal(SIGKILL); restart: spin_lock(&clp->cl_lock); if (test_bit(NFS4CLNT_STATE_RECOVER, &clp->cl_state) != 0) @@ -310,8 +309,6 @@ static int recall_thread(void *data) struct nfs_inode *nfsi = NFS_I(inode); struct nfs_delegation *delegation; - daemonize("nfsv4-delegreturn"); - nfs_msync_inode(inode); down_read(&clp->cl_sem); down_write(&nfsi->rwsem); @@ -350,18 +347,18 @@ int nfs_async_inode_return_delegation(struct inode *inode, const nfs4_stateid *s .inode = inode, .stateid = stateid, }; - int status; + struct task_struct *task; init_completion(&data.started); __module_get(THIS_MODULE); - status = kernel_thread(recall_thread, &data, CLONE_KERNEL); - if (status < 0) + task = kthread_run(recall_thread, &data, "nfsv4-delegreturn"); + if (IS_ERR(task)) goto out_module_put; wait_for_completion(&data.started); return data.result; out_module_put: module_put(THIS_MODULE); - return status; + return PTR_ERR(task); } /* -- 1.5.0.g53756 -
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Start the reclaimer thread using kthread_run instead
of a combination of kernel_thread and daemonize.
The small amount of signal handling code is also removed
as it makes no sense and is a maintenance problem to handle
signals in kernel threads.
Cc: Neil Brown <neilb@suse.de>
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/lockd/clntlock.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index f4d45d4..83591f6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -9,6 +9,7 @@
#include <linux/module.h>
#include <linux/types.h>
#include <linux/time.h>
+#include <linux/kthread.h>
#include <linux/nfs_fs.h>
#include <linux/sunrpc/clnt.h>
#include <linux/sunrpc/svc.h>
@@ -153,7 +154,7 @@ nlmclnt_recovery(struct nlm_host *host)
if (!host->h_reclaiming++) {
nlm_get_host(host);
__module_get(THIS_MODULE);
- if (kernel_thread(reclaimer, host, CLONE_KERNEL) < 0)
+ if (IS_ERR(kthread_run(reclaimer, host, "%s-reclaim", host->h_name)))
module_put(THIS_MODULE);
}
}
@@ -166,9 +167,6 @@ reclaimer(void *ptr)
struct file_lock *fl, *next;
u32 nsmstate;
- daemonize("%s-reclaim", host->h_name);
- allow_signal(SIGKILL);
-
down_write(&host->h_rwsem);
/* This one ensures that our parent doesn't terminate while the
@@ -193,8 +191,6 @@ restart:
list_del_init(&fl->fl_u.nfs_fl.list);
/* Why are we leaking memory here? --okir */
- if (signalled())
- continue;
if (nlmclnt_reclaim(host, fl) != 0)
continue;
list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
--
1.5.0.g53756
-
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.
CC: Scott Murray <scottm@somanetworks.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pci/hotplug/cpci_hotplug_core.c | 22 +++++++---------------
1 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index 6845515..c620c7e 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -33,6 +33,7 @@
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/smp_lock.h>
+#include <linux/kthread.h>
#include <asm/atomic.h>
#include <linux/delay.h>
#include "cpci_hotplug.h"
@@ -521,17 +522,13 @@ 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))
+ if (thread_finished)
break;
do {
rc = check_slots();
@@ -562,12 +559,8 @@ poll_thread(void *data)
{
int rc;
- lock_kernel();
- daemonize("cpci_hp_polld");
- unlock_kernel();
-
while (1) {
- if (thread_finished || signal_pending(current))
+ if (thread_finished)
break;
if (controller->ops->query_enum()) {
do {
@@ -592,7 +585,7 @@ poll_thread(void *data)
static int
cpci_start_thread(void)
{
- int pid;
+ struct task_struct *task;
/* initialize our semaphores */
init_MUTEX_LOCKED(&event_semaphore);
@@ -600,14 +593,13 @@ cpci_start_thread(void)
thread_finished = 0;
if (controller->irq)
- pid = ...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 ...[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 -
Sorry, it took me a few days to get to testing this out. It looks good,
but I had to make a couple of tweaks to avoid a hang when rmmod'ing a
board driver. The board drivers do:
cpci_hp_stop()
cpci_hp_unregister_controller(controller)
to shutdown, and the check in cpci_hp_unregister_controller if the thread
is running wasn't working due to a bit too much code being excised. The
result was kthread_stop being called twice, which hangs. I've indicated
my changes to avoid this inline below.
There still needs to be a:
thread_finished = 0;
here, so that things work if a board driver is insmod'ed again after a
As well, there still needs to be a:
thread_finished = 1;
--
Scott Murray
SOMA Networks, Inc.
Toronto, Ontario
e-mail: scottm@somanetworks.com
-
Can you forward the patches with your fix to Andrew to make sure he picks it up? -
Andrew, here is my updated version of Christoph's kthread conversion
patch for cpci_hotplug. I've CC'ed Kristen so she won't be surprised
when this eventually goes to mainline.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Scott Murray <scottm@somanetworks.com>
---
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index 6845515..ed4d44e 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -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 || ...On Mon, 7 May 2007 14:18:29 -0400 (EDT)
A patch in this area would normally go
you->kristen->mainline
|
v
-mm
or
you->kristen->greg->mainline
|
v
-mm
or
you->me->greg->mainline (gets an Acked-by somewhere)
|
v
-mm
or
you->kristen->Len->mainline
|
v
-mm
or something else.
Kristen, how do you want to play this?
Do you run a tree? If so, lemmeatit ;)
-
On Wed, 9 May 2007 16:24:30 -0700 So, we (Greg and I) have talked about this before - I do have a tree, but I normally send things to Greg rather than directly to you. I should probably change that (why add levels of indirection...), but for the immediate purpose of getting this patch tested etc just go ahead and take it in and when I get my act together I will put it in my tree. Thanks, Kristen -
My apologies, it wasn't entirely clear to me that that was happening with the other kthread conversion patches in the big batch, so I didn't follow the normal procedure. I'll try to stick to the regular flow up the chain going forward. Scott -- Scott Murray SOMA Networks, Inc. Toronto, Ontario e-mail: scottm@somanetworks.com -
oh that's OK. I'm kind of a catchall routing service. It's just that in this case I wasn't sure which direction the next hop lay in. -
This patch just trivial converts from calling kernel_thread and daemonize
to just calling kthread_run.
Cc: Andi Kleen <ak@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/i386/kernel/io_apic.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/i386/kernel/io_apic.c b/arch/i386/kernel/io_apic.c
index 24ac67c..84b412a 100644
--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -34,6 +34,7 @@
#include <linux/msi.h>
#include <linux/htirq.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <asm/io.h>
#include <asm/smp.h>
@@ -660,8 +661,6 @@ static int balanced_irq(void *unused)
unsigned long prev_balance_time = jiffies;
long time_remaining = balanced_irq_interval;
- daemonize("kirqd");
-
/* push everything to CPU 0 to give us a starting point. */
for (i = 0 ; i < NR_IRQS ; i++) {
irq_desc[i].pending_mask = cpumask_of_cpu(0);
@@ -721,7 +720,7 @@ static int __init balanced_irq_init(void)
}
printk(KERN_INFO "Starting balanced_irq\n");
- if (kernel_thread(balanced_irq, NULL, CLONE_KERNEL) >= 0)
+ if (!IS_ERR(kthread_run(balanced_irq, NULL, "kirqd")))
return 0;
else
printk(KERN_ERR "balanced_irq_init: failed to spawn balanced_irq");
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patches modifies the pnpbios kernel thread to start
with ktrhead_run not kernel_thread and deamonize. Doing
this makes the code a little simpler and easier to maintain.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pnp/pnpbios/core.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
index c2ed53f..3a201b7 100644
--- a/drivers/pnp/pnpbios/core.c
+++ b/drivers/pnp/pnpbios/core.c
@@ -62,6 +62,7 @@
#include <linux/delay.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <asm/page.h>
#include <asm/desc.h>
@@ -159,9 +160,7 @@ static int pnp_dock_thread(void * unused)
{
static struct pnp_docking_station_info now;
int docked = -1, d = 0;
- daemonize("kpnpbiosd");
- allow_signal(SIGKILL);
- while(!unloading && !signal_pending(current))
+ while (!unloading)
{
int status;
@@ -170,11 +169,8 @@ static int pnp_dock_thread(void * unused)
*/
msleep_interruptible(2000);
- if(signal_pending(current)) {
- if (try_to_freeze())
- continue;
- break;
- }
+ if (try_to_freeze())
+ continue;
status = pnp_bios_dock_station_info(&now);
@@ -582,6 +578,7 @@ subsys_initcall(pnpbios_init);
static int __init pnpbios_thread_init(void)
{
+ struct task_struct *task;
#if defined(CONFIG_PPC_MERGE)
if (check_legacy_ioport(PNPBIOS_BASE))
return 0;
@@ -590,7 +587,8 @@ static int __init pnpbios_thread_init(void)
return 0;
#ifdef CONFIG_HOTPLUG
init_completion(&unload_sem);
- if (kernel_thread(pnp_dock_thread, NULL, CLONE_KERNEL) > 0)
+ task = kthread_run(pnp_dock_thread, NULL, "kpnpbiosd");
+ if (!IS_ERR(task))
unloading = 0;
#endif
return 0;
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Use kthread_run to start the lcs kernel threads not a
combination of kernel_thread and daemonize. This makes
the code slightly simpler and more maintainable.
Cc: Frank Pavlic <fpavlic@de.ibm.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/s390/net/lcs.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
index 08a994f..0300d87 100644
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -36,6 +36,7 @@
#include <linux/in.h>
#include <linux/igmp.h>
#include <linux/delay.h>
+#include <linux/kthread.h>
#include <net/arp.h>
#include <net/ip.h>
@@ -1248,7 +1249,6 @@ lcs_register_mc_addresses(void *data)
struct in_device *in4_dev;
card = (struct lcs_card *) data;
- daemonize("regipm");
if (!lcs_do_run_thread(card, LCS_SET_MC_THREAD))
return 0;
@@ -1728,11 +1728,10 @@ lcs_start_kernel_thread(struct work_struct *work)
struct lcs_card *card = container_of(work, struct lcs_card, kernel_thread_starter);
LCS_DBF_TEXT(5, trace, "krnthrd");
if (lcs_do_start_thread(card, LCS_RECOVERY_THREAD))
- kernel_thread(lcs_recovery, (void *) card, SIGCHLD);
+ kthread_run(lcs_recovery, card, "lcs_recover");
#ifdef CONFIG_IP_MULTICAST
if (lcs_do_start_thread(card, LCS_SET_MC_THREAD))
- kernel_thread(lcs_register_mc_addresses,
- (void *) card, SIGCHLD);
+ kernel_run(lcs_register_mc_addresses, card, "regipm");
#endif
}
@@ -2232,7 +2231,6 @@ lcs_recovery(void *ptr)
int rc;
card = (struct lcs_card *) ptr;
- daemonize("lcs_recover");
LCS_DBF_TEXT(4, trace, "recover1");
if (!lcs_do_run_thread(card, LCS_RECOVERY_THREAD))
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Modify startup of ipvs sync threads to use kthread_run
instead of a weird combination of calling kernel_thread
to start a fork_sync_thread whose hole purpose in life was
to call kernel_thread again starting the actually sync thread
which called daemonize.
To use kthread_run I had to move the name calcuation from
sync_thread into start_sync_thread resulting in a small
amount of code motion.
The result is simpler and more maintainable piece of code.
Cc: Wensong Zhang <wensong@linux-vs.org>
Cc: Julian Anastasov <ja@ssi.bg>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/ipv4/ipvs/ip_vs_sync.c | 49 ++++++++++---------------------------------
1 files changed, 12 insertions(+), 37 deletions(-)
diff --git a/net/ipv4/ipvs/ip_vs_sync.c b/net/ipv4/ipvs/ip_vs_sync.c
index 7ea2d98..c4be9dc 100644
--- a/net/ipv4/ipvs/ip_vs_sync.c
+++ b/net/ipv4/ipvs/ip_vs_sync.c
@@ -29,6 +29,7 @@
#include <linux/in.h>
#include <linux/igmp.h> /* for ip_mc_join_group */
#include <linux/udp.h>
+#include <linux/kthread.h>
#include <net/ip.h>
#include <net/sock.h>
@@ -750,34 +751,23 @@ static int sync_thread(void *startup)
DECLARE_WAITQUEUE(wait, current);
mm_segment_t oldmm;
int state;
- const char *name;
/* increase the module use count */
ip_vs_use_count_inc();
- if (ip_vs_sync_state & IP_VS_STATE_MASTER && !sync_master_pid) {
+ if (ip_vs_sync_state & IP_VS_STATE_MASTER && !sync_master_pid)
state = IP_VS_STATE_MASTER;
- name = "ipvs_syncmaster";
- } else if (ip_vs_sync_state & IP_VS_STATE_BACKUP && !sync_backup_pid) {
+ else if (ip_vs_sync_state & IP_VS_STATE_BACKUP && !sync_backup_pid)
state = IP_VS_STATE_BACKUP;
- name = "ipvs_syncbackup";
- } else {
+ else {
IP_VS_BUG();
ip_vs_use_count_dec();
return -EINVAL;
}
- daemonize(name);
-
oldmm = get_fs();
set_fs(KERNEL_DS);
- /* Block all signals ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
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.
Cc: Jes Sorensen <jes@sgi.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/ia64/sn/kernel/xpc_main.c | 31 +++++++++++++------------------
1 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/arch/ia64/sn/kernel/xpc_main.c b/arch/ia64/sn/kernel/xpc_main.c
index e336e16..5b53642 100644
--- a/arch/ia64/sn/kernel/xpc_main.c
+++ b/arch/ia64/sn/kernel/xpc_main.c
@@ -56,6 +56,7 @@
#include <linux/reboot.h>
#include <linux/completion.h>
#include <linux/kdebug.h>
+#include <linux/kthread.h>
#include <asm/sn/intr.h>
#include <asm/sn/sn_sal.h>
#include <asm/uaccess.h>
@@ -253,8 +254,6 @@ xpc_hb_checker(void *ignore)
/* 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);
@@ -324,8 +323,6 @@ xpc_hb_checker(void *ignore)
static int
xpc_initiate_discovery(void *ignore)
{
- daemonize(XPC_DISCOVERY_THREAD_NAME);
-
xpc_discovery();
dev_dbg(xpc_part, "discovery thread is exiting\n");
@@ -494,8 +491,6 @@ xpc_activating(void *__partid)
dev_dbg(xpc_part, "bringing partition %d up\n", partid);
- daemonize("xpc%02d", partid);
-
/*
* This thread needs to run at a realtime priority to prevent a
* significant performance degradation.
@@ -559,7 +554,7 @@ xpc_activate_partition(struct xpc_partition *part)
{
partid_t partid = XPC_PARTID(part);
unsigned long irq_flags;
- pid_t pid;
+ struct task_struct *task;
spin_lock_irqsave(&part->act_lock, irq_flags);
@@ -571,9 +566,10 @@ xpc_activate_partition(struct xpc_partition *part)
spin_unlock_irqrestore(&part->act_lock, ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
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.
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/powerpc/platforms/pseries/eeh_event.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/eeh_event.c b/arch/powerpc/platforms/pseries/eeh_event.c
index 221dec8..fe7c2e0 100644
--- a/arch/powerpc/platforms/pseries/eeh_event.c
+++ b/arch/powerpc/platforms/pseries/eeh_event.c
@@ -23,6 +23,7 @@
#include <linux/mutex.h>
#include <linux/pci.h>
#include <linux/workqueue.h>
+#include <linux/kthread.h>
#include <asm/eeh_event.h>
#include <asm/ppc-pci.h>
@@ -59,7 +60,6 @@ static int eeh_event_handler(void * dummy)
struct eeh_event *event;
struct pci_dn *pdn;
- daemonize ("eehd");
set_current_state(TASK_INTERRUPTIBLE);
spin_lock_irqsave(&eeh_eventlist_lock, flags);
@@ -105,7 +105,7 @@ static int eeh_event_handler(void * dummy)
*/
static void eeh_thread_launcher(struct work_struct *dummy)
{
- if (kernel_thread(eeh_event_handler, NULL, CLONE_KERNEL) < 0)
+ if (IS_ERR(kthread_run(eeh_event_handler, NULL, "eehd")))
printk(KERN_ERR "Failed to start EEH daemon\n");
}
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
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.
Cc: Swen Schillig <swen@vnet.ibm.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/s390/scsi/zfcp_erp.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index 66c0b09..f26536d 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -21,6 +21,7 @@
#define ZFCP_LOG_AREA ZFCP_LOG_AREA_ERP
+#include <linux/kthread.h>
#include "zfcp_ext.h"
static int zfcp_erp_adisc(struct zfcp_port *);
@@ -985,12 +986,13 @@ static void zfcp_erp_action_dismiss(struct zfcp_erp_action *erp_action)
int
zfcp_erp_thread_setup(struct zfcp_adapter *adapter)
{
- int retval = 0;
+ struct task_struct *task;
atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
- retval = kernel_thread(zfcp_erp_thread, adapter, SIGCHLD);
- if (retval < 0) {
+ task = kthread_run(zfcp_erp_thread, adapter,
+ "zfcperp%s", zfcp_get_busid_by_adapter(adapter));
+ if (IS_ERR(task)) {
ZFCP_LOG_NORMAL("error: creation of erp thread failed for "
"adapter %s\n",
zfcp_get_busid_by_adapter(adapter));
@@ -1002,7 +1004,7 @@ zfcp_erp_thread_setup(struct zfcp_adapter *adapter)
debug_text_event(adapter->erp_dbf, 5, "a_thset_ok");
}
- return (retval < 0);
+ return IS_ERR(task);
}
/*
@@ -1054,9 +1056,6 @@ zfcp_erp_thread(void *data)
struct zfcp_erp_action *erp_action;
unsigned long flags;
- daemonize("zfcperp%s", zfcp_get_busid_by_adapter(adapter));
- /* Block all signals */
- siginitsetinv(&current->blocked, 0);
atomic_set_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
debug_text_event(adapter->erp_dbf, 5, "a_th_run");
wake_up(&adapter->erp_thread_wqh);
-- ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch modifies the startup of krxtimod, krxiod, and krxsecd
to use kthread_run instead of a combination of kernel_thread
and daemonize making the code slightly simpler and more maintainable.
In addition since by default all signals are ignored when delivered
to a kernel thread the code to flush signals has been removed.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/rxrpc/internal.h | 11 -----------
net/rxrpc/krxiod.c | 16 ++++++++--------
net/rxrpc/krxsecd.c | 16 ++++++++--------
net/rxrpc/krxtimod.c | 15 ++++++---------
4 files changed, 22 insertions(+), 36 deletions(-)
diff --git a/net/rxrpc/internal.h b/net/rxrpc/internal.h
index cc0c579..1dd69aa 100644
--- a/net/rxrpc/internal.h
+++ b/net/rxrpc/internal.h
@@ -49,17 +49,6 @@ __RXACCT_DECL(extern atomic_t rxrpc_message_count);
#define _net(FMT, a...) do { if (rxrpc_knet) knet (FMT , ##a); } while(0)
#endif
-static inline void rxrpc_discard_my_signals(void)
-{
- while (signal_pending(current)) {
- siginfo_t sinfo;
-
- spin_lock_irq(&current->sighand->siglock);
- dequeue_signal(current, &current->blocked, &sinfo);
- spin_unlock_irq(&current->sighand->siglock);
- }
-}
-
/*
* call.c
*/
diff --git a/net/rxrpc/krxiod.c b/net/rxrpc/krxiod.c
index bbbcd6c..c590ccd 100644
--- a/net/rxrpc/krxiod.c
+++ b/net/rxrpc/krxiod.c
@@ -14,6 +14,7 @@
#include <linux/spinlock.h>
#include <linux/init.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <rxrpc/krxiod.h>
#include <rxrpc/transport.h>
#include <rxrpc/peer.h>
@@ -43,8 +44,6 @@ static int rxrpc_krxiod(void *arg)
printk("Started krxiod %d\n",current->pid);
- daemonize("krxiod");
-
/* loop around waiting for work to do */
do {
/* wait for work or to be told to exit */
@@ -57,8 +56,7 @@ static int rxrpc_krxiod(void *arg)
for (;;) {
...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch is a minimal transformation to use the kthread API
doing it's best to preserve the existing logic.
Instead of starting kdvb-ca by calling kernel_thread,
daemonize and sigfillset we kthread_run is used.
Instead of tracking the pid of the running thread we instead
simply keep a flag to indicate that the current thread is
running, as that is all the pid is really used for.
And finally the kill_proc sending signal 0 to the kernel thread to
ensure it is alive before we wait for it to shutdown is removed.
The kthread API does not provide the pid so we don't have that
information readily available and the test is just silly. If there
is no shutdown race the test is a useless confirmation of that the
thread is running. If there is a race the test doesn't fix it and
we should fix the race properly.
Cc: Andrew de Quincey <adq_dvb@lidskialf.net>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/media/dvb/dvb-core/dvb_ca_en50221.c | 46 ++++++++++----------------
1 files changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
index 2a03bf5..b28bc15 100644
--- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
@@ -37,6 +37,7 @@
#include <linux/delay.h>
#include <linux/spinlock.h>
#include <linux/sched.h>
+#include <linux/kthread.h>
#include "dvb_ca_en50221.h"
#include "dvb_ringbuffer.h"
@@ -139,8 +140,8 @@ struct dvb_ca_private {
/* wait queues for read() and write() operations */
wait_queue_head_t wait_queue;
- /* PID of the monitoring thread */
- pid_t thread_pid;
+ /* Flag indicating the monitoring thread is running */
+ int thread_running;
/* Wait queue used when shutting thread down */
wait_queue_head_t thread_queue;
@@ -982,7 +983,6 @@ static void ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted 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. Cc: Paul Mackerras <paulus@samba.org> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- arch/powerpc/platforms/pseries/rtasd.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/platforms/pseries/rtasd.c b/arch/powerpc/platforms/pseries/rtasd.c index 77d0937..919a374 100644 --- a/arch/powerpc/platforms/pseries/rtasd.c +++ b/arch/powerpc/platforms/pseries/rtasd.c @@ -20,6 +20,7 @@ #include <linux/spinlock.h> #include <linux/cpu.h> #include <linux/delay.h> +#include <linux/kthread.h> #include <asm/uaccess.h> #include <asm/io.h> @@ -429,8 +430,6 @@ static int rtasd(void *unused) int event_scan = rtas_token("event-scan"); int rc; - daemonize("rtasd"); - if (event_scan == RTAS_UNKNOWN_SERVICE || get_eventscan_parms() == -1) goto error; @@ -497,7 +496,7 @@ static int __init rtas_init(void) else printk(KERN_ERR "Failed to create error_log proc entry\n"); - if (kernel_thread(rtasd, NULL, CLONE_FS) < 0) + if (IS_ERR(kthread_run(rtasd, NULL, "rtasd"))) printk(KERN_ERR "Failed to start RTAS daemon\n"); return 0; -- 1.5.0.g53756 -
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch modifies the startup of the media_bay_task
to use kthread_run and not a combination of kernel_thread,
deamonize and sigfillset.
In addition since we now always want to ignore signals
the MB_IGNORE_SIGNALS define is removed along with the
test for signal_pending.
The result is slightly simpler code that is more
maintainable.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/mediabay.c | 11 ++---------
1 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/macintosh/mediabay.c b/drivers/macintosh/mediabay.c
index c803d2b..90c853e 100644
--- a/drivers/macintosh/mediabay.c
+++ b/drivers/macintosh/mediabay.c
@@ -20,6 +20,7 @@
#include <linux/stddef.h>
#include <linux/init.h>
#include <linux/ide.h>
+#include <linux/kthread.h>
#include <asm/prom.h>
#include <asm/pgtable.h>
#include <asm/io.h>
@@ -35,7 +36,6 @@
#define MB_DEBUG
-#define MB_IGNORE_SIGNALS
#ifdef MB_DEBUG
#define MBDBG(fmt, arg...) printk(KERN_INFO fmt , ## arg)
@@ -622,11 +622,6 @@ static int media_bay_task(void *x)
{
int i;
- strcpy(current->comm, "media-bay");
-#ifdef MB_IGNORE_SIGNALS
- sigfillset(&current->blocked);
-#endif
-
for (;;) {
for (i = 0; i < media_bay_count; ++i) {
down(&media_bays[i].lock);
@@ -636,8 +631,6 @@ static int media_bay_task(void *x)
}
msleep_interruptible(MB_POLL_DELAY);
- if (signal_pending(current))
- return 0;
}
}
@@ -699,7 +692,7 @@ static int __devinit media_bay_attach(struct macio_dev *mdev, const struct of_de
/* Startup kernel thread */
if (i == 0)
- kernel_thread(media_bay_task, NULL, CLONE_KERNEL);
+ kthread_run(media_bay_task, NULL, "media-bay");
return 0;
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/smbfs/smbiod.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/fs/smbfs/smbiod.c b/fs/smbfs/smbiod.c
index 3e61b44..67176af 100644
--- a/fs/smbfs/smbiod.c
+++ b/fs/smbfs/smbiod.c
@@ -298,8 +298,6 @@ out:
*/
static int smbiod(void *unused)
{
- allow_signal(SIGKILL);
-
VERBOSE("SMB Kernel thread starting (%d) ...\n", current->pid);
for (;;) {
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
thread_run is used intead of kernel_thread, daemonize, and mucking
around blocking signals directly.
CC: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/mtd/mtd_blkdevs.c | 19 +++++--------------
1 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index db7397c..ed71d5e 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -21,6 +21,7 @@
#include <linux/init.h>
#include <linux/mutex.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <asm/uaccess.h>
static LIST_HEAD(blktrans_majors);
@@ -84,17 +85,6 @@ 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) {
@@ -368,6 +358,7 @@ 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
@@ -406,13 +397,13 @@ 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;
- ret = kernel_thread(mtd_blktrans_thread, tr, CLONE_KERNEL);
- if (ret < 0) {
+ task = kthread_run(mtd_blktrans_thread, tr, "%sd", tr->name);
+ if (IS_ERR(task)) {
...Please don't do incomplete transitions like that. We don't really want people to use kthread_run, but not the kthread stopping mechanisms, because people will simply forget about that bit and we'll never get rid of the enormous amount of, erm creativity, in handling kernel thread stopping. This is just the first patch in your series where the thread is mutable, but it equally applies to all following patches where this is the case aswell. -
I don't really care about the creativity. Although it would be nice if it wasn't there. I deliberately left it in so I would be certain my patches were correct. I care about killing the maintenance and forward development roadblocks that are kernel_thread and daemonize. And the user interface problem that is handling signals in kernel threads. Eric -
On Thu, 19 Apr 2007 13:13:22 -0600 Yes, I think that is a practical position, if not an ideal one. MTD (to pick one example) does need to be decruftified: remove r->blkcore_priv->exiting, probably ->blkcore_priv->thread_dead, switch deregister_mtd_blktrans() to use kthread_stop(). But it's a bit much to expect Eric to make that conversion, and to suitably test it. All he can do is to make a best-effort and hope that someone else tests it, which isn't very reliable. This partial patch at least gets us some of the way there, and serves as a gentle reminder to dwmwyouknowwho to finish cleaning this stuff up. I'd be more concerned about a part-conversion in a subsystem which has no identifiable maintainer, because in that case the chances are that we'll just forget about it an the conversion would never be completed. And of course, these are not simply cleanup patches: we actually need to get the kernel threads out of the daemonize() and signalling game to complete the virtualisation work. -
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, ...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;
...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. -
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);
-
...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch starts up khidp using kthread_run instead
of kernel_thread and daemonize, resulting is slightly
simpler and more maintainable code.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/bluetooth/hidp/core.c | 29 ++++++++++++++++-------------
1 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index df2c471..1c9b202 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
#include <linux/init.h>
#include <linux/wait.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <net/sock.h>
#include <linux/input.h>
@@ -531,22 +532,11 @@ static int hidp_session(void *arg)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
- int vendor = 0x0000, product = 0x0000;
wait_queue_t ctrl_wait, intr_wait;
BT_DBG("session %p", session);
- if (session->input) {
- vendor = session->input->id.vendor;
- product = session->input->id.product;
- }
-
- if (session->hid) {
- vendor = session->hid->vendor;
- product = session->hid->product;
- }
- daemonize("khidpd_%04x%04x", vendor, product);
set_user_nice(current, -15);
init_waitqueue_entry(&ctrl_wait, current);
@@ -747,7 +737,9 @@ static inline void hidp_setup_hid(struct hidp_session *session, struct hidp_conn
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
{
+ int vendor = 0x0000, product = 0x0000;
struct hidp_session *session, *s;
+ struct task_struct *task;
int err;
BT_DBG("");
@@ -834,8 +826,19 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
hidp_set_timer(session);
- err = kernel_thread(hidp_session, session, CLONE_KERNEL);
- if (err < 0)
+ if (session->input) {
+ vendor = ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Cc: Neil Brown <neilb@suse.de>
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/nfsd/nfs4state.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 678f3be..3cc8ce4 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1326,8 +1326,6 @@ do_recall(void *__dp)
{
struct nfs4_delegation *dp = __dp;
- daemonize("nfsv4-recall");
-
nfsd4_cb_recall(dp);
return 0;
}
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted This patch starts krfcommd using kthread_run instead of a combination of kernel_thread and daemonize making the code slightly simpler and more maintainable. Cc: Marcel Holtmann <marcel@holtmann.org> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- net/bluetooth/rfcomm/core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index 34f993a..baaad49 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -38,6 +38,7 @@ #include <linux/net.h> #include <linux/mutex.h> #include <linux/freezer.h> +#include <linux/kthread.h> #include <net/sock.h> #include <asm/uaccess.h> @@ -1938,7 +1939,6 @@ static int rfcomm_run(void *unused) atomic_inc(&running); - daemonize("krfcommd"); set_user_nice(current, -10); BT_DBG(""); @@ -2058,7 +2058,7 @@ static int __init rfcomm_init(void) hci_register_cb(&rfcomm_cb); - kernel_thread(rfcomm_run, NULL, CLONE_KERNEL); + kthread_run(rfcomm_run, NULL, "krfcommd"); if (class_create_file(bt_class, &class_attr_rfcomm_dlc) < 0) BT_ERR("Failed to create RFCOMM info file"); -- 1.5.0.g53756 -
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This starts the sparc64 powerd using kthread_run
instead of kernel_thread and daemonize. Making the
code slightly simpler and more maintainable.
In addition the unnecessary flush_signals is removed.
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/sparc64/kernel/power.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/sparc64/kernel/power.c b/arch/sparc64/kernel/power.c
index 699b24b..03feb8b 100644
--- a/arch/sparc64/kernel/power.c
+++ b/arch/sparc64/kernel/power.c
@@ -13,6 +13,7 @@
#include <linux/interrupt.h>
#include <linux/pm.h>
#include <linux/syscalls.h>
+#include <linux/kthread.h>
#include <asm/system.h>
#include <asm/auxio.h>
@@ -81,15 +82,12 @@ static int powerd(void *__unused)
char *argv[] = { "/sbin/shutdown", "-h", "now", NULL };
DECLARE_WAITQUEUE(wait, current);
- daemonize("powerd");
-
add_wait_queue(&powerd_wait, &wait);
again:
for (;;) {
set_task_state(current, TASK_INTERRUPTIBLE);
if (button_pressed)
break;
- flush_signals(current);
schedule();
}
__set_current_state(TASK_RUNNING);
@@ -128,7 +126,9 @@ static int __devinit power_probe(struct of_device *op, const struct of_device_id
poweroff_method = machine_halt; /* able to use the standard halt */
if (has_button_interrupt(irq, op->node)) {
- if (kernel_thread(powerd, NULL, CLONE_FS) < 0) {
+ struct task_struct *task;
+ task = kthread_urn(powerd, NULL, "powerd");
+ if (IS_ERR(task)) {
printk("Failed to start power daemon.\n");
return 0;
}
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
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.
Cc: Darrick J. Wong <djwong@us.ibm.com>
Cc: James Bottomley <James.Bottomley@SteelEye.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/scsi/libsas/sas_scsi_host.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 46ba3a7..7a38ac5 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -40,6 +40,7 @@
#include <linux/blkdev.h>
#include <linux/scatterlist.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
/* ---------- SCSI Host glue ---------- */
@@ -870,7 +871,6 @@ 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);
@@ -891,19 +891,20 @@ static int sas_queue_thread(void *_sas_ha)
int sas_init_queue(struct sas_ha_struct *sas_ha)
{
- int res;
struct scsi_core *core = &sas_ha->core;
+ struct task_struct *task;
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)
+ task = kthread_run(sas_queue_thread, sas_ha,
+ "sas_queue_%d", core->shost->host_no);
+ if (!IS_ERR(task))
wait_for_completion(&queue_th_comp);
- return res < 0 ? res : 0;
+ return IS_ERR(task) ? PTR_ERR(task) : 0;
}
void sas_shutdown_queue(struct sas_ha_struct *sas_ha)
--
1.5.0.g53756
-
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.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pci/hotplug/cpqphp_ctrl.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c b/drivers/pci/hotplug/cpqphp_ctrl.c
index 79ff6b4..c2c06c4 100644
--- a/drivers/pci/hotplug/cpqphp_ctrl.c
+++ b/drivers/pci/hotplug/cpqphp_ctrl.c
@@ -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,
@@ -1746,10 +1747,6 @@ static void pushbutton_helper_thread(unsigned long data)
static int event_thread(void* data)
{
struct controller *ctrl;
- lock_kernel();
- daemonize("phpd_event");
-
- unlock_kernel();
while (1) {
dbg("!!!!event_thread sleeping\n");
@@ -1771,7 +1768,7 @@ static int event_thread(void* data)
int cpqhp_event_start_thread(void)
{
- int pid;
+ struct task_struct *task;
/* initialize our semaphores */
init_MUTEX(&delay_sem);
@@ -1779,12 +1776,11 @@ int cpqhp_event_start_thread(void)
init_MUTEX_LOCKED(&event_exit);
event_finished=0;
- pid = kernel_thread(event_thread, NULL, 0);
- if (pid < 0) {
+ task = kthread_run(event_thread, NULL, "phpd_event");
+ if (IS_ERR(task)) {
err ("Can't start up our event thread\n");
return -1;
}
- dbg("Our event thread pid = %d\n", pid);
return 0;
}
--
1.5.0.g53756
-
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_ ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
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.
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/arm/kernel/ecard.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm/kernel/ecard.c b/arch/arm/kernel/ecard.c
index f1c0fb9..6c15f5f 100644
--- a/arch/arm/kernel/ecard.c
+++ b/arch/arm/kernel/ecard.c
@@ -40,6 +40,7 @@
#include <linux/device.h>
#include <linux/init.h>
#include <linux/mutex.h>
+#include <linux/kthread.h>
#include <asm/dma.h>
#include <asm/ecard.h>
@@ -263,8 +264,6 @@ static int ecard_init_mm(void)
static int
ecard_task(void * unused)
{
- daemonize("kecardd");
-
/*
* Allocate a mm. We're not a lazy-TLB kernel task since we need
* to set page table entries where the user space would be. Note
@@ -1058,13 +1057,14 @@ ecard_probe(int slot, card_type_t type)
*/
static int __init ecard_init(void)
{
- int slot, irqhw, ret;
+ struct task_struct *task;
+ int slot, irqhw;
- ret = kernel_thread(ecard_task, NULL, CLONE_KERNEL);
- if (ret < 0) {
+ task = kthread_run(ecard_task, NULL, "kecardd");
+ if (IS_ERR(task)) {
printk(KERN_ERR "Ecard: unable to create kernel thread: %d\n",
- ret);
- return ret;
+ PTR_ERR(task));
+ return PTR_ERR(task);
}
printk("Probing expansion cards\n");
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted This patch modifies the qeth_recover thread to be started with kthread_run not a combination of kernel_thread and daemonize. Resulting in slightly simpler and more maintainable code. Cc: Frank Pavlic <fpavlic@de.ibm.com> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- drivers/s390/net/qeth_main.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/s390/net/qeth_main.c b/drivers/s390/net/qeth_main.c index ad7792d..8234846 100644 --- a/drivers/s390/net/qeth_main.c +++ b/drivers/s390/net/qeth_main.c @@ -50,6 +50,7 @@ #include <linux/mii.h> #include <linux/rcupdate.h> #include <linux/ethtool.h> +#include <linux/kthread.h> #include <net/arp.h> #include <net/ip.h> @@ -957,7 +958,6 @@ qeth_recover(void *ptr) int rc = 0; card = (struct qeth_card *) ptr; - daemonize("qeth_recover"); QETH_DBF_TEXT(trace,2,"recover1"); QETH_DBF_HEX(trace, 2, &card, sizeof(void *)); if (!qeth_do_run_thread(card, QETH_RECOVER_THREAD)) @@ -1014,7 +1014,7 @@ qeth_start_kernel_thread(struct work_struct *work) card->write.state != CH_STATE_UP) return; if (qeth_do_start_thread(card, QETH_RECOVER_THREAD)) - kernel_thread(qeth_recover, (void *) card, SIGCHLD); + kthread_run(qeth_recover, card, "qeth_recover"); } -- 1.5.0.g53756 -
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch starts kbenpd using kthread_run replacing
a combination of kernel_thread and daemonize. Making
the code a little simpler and more maintainable.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/bluetooth/bnep/core.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index a9f1e88..de3caed 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -32,6 +32,7 @@
#include <linux/module.h>
#include <linux/kernel.h>
+#include <linux/kthread.h>
#include <linux/sched.h>
#include <linux/signal.h>
#include <linux/init.h>
@@ -473,7 +474,6 @@ static int bnep_session(void *arg)
BT_DBG("");
- daemonize("kbnepd %s", dev->name);
set_user_nice(current, -15);
init_waitqueue_entry(&wait, current);
@@ -539,6 +539,7 @@ static struct device *bnep_get_device(struct bnep_session *session)
int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
{
+ struct task_struct *task;
struct net_device *dev;
struct bnep_session *s, *ss;
u8 dst[ETH_ALEN], src[ETH_ALEN];
@@ -598,9 +599,10 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
__bnep_link_session(s);
- err = kernel_thread(bnep_session, s, CLONE_KERNEL);
- if (err < 0) {
+ task = kthread_run(bnep_session, s, "kbnepd %s", dev->name);
+ if (IS_ERR(task)) {
/* Session thread start failed, gotta cleanup. */
+ err = PTR_ERR(task);
unregister_netdev(dev);
__bnep_unlink_session(s);
goto failed;
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Currently md_thread calls allow_signal so it can receive a
SIGKILL but then does nothing with it except flush the
sigkill so that it not can use an interruptible sleep.
This whole dance is silly so remove the unnecessary
and broken signal handling logic.
Cc: Neil Brown <neilb@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/md/md.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1299c23..dfd0cb9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4542,17 +4542,11 @@ static int md_thread(void * arg)
*/
current->flags |= PF_NOFREEZE;
- allow_signal(SIGKILL);
while (!kthread_should_stop()) {
/* We need to wait INTERRUPTIBLE so that
* we don't add to the load-average.
- * That means we need to be sure no signals are
- * pending
*/
- if (signal_pending(current))
- flush_signals(current);
-
wait_event_interruptible_timeout
(thread->wqueue,
test_bit(THREAD_WAKEUP, &thread->flags)
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
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.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/therm_pm72.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/macintosh/therm_pm72.c b/drivers/macintosh/therm_pm72.c
index b002a4b..7e9cbb7 100644
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -121,6 +121,7 @@
#include <linux/reboot.h>
#include <linux/kmod.h>
#include <linux/i2c.h>
+#include <linux/kthread.h>
#include <asm/prom.h>
#include <asm/machdep.h>
#include <asm/io.h>
@@ -161,7 +162,7 @@ static struct slots_pid_state slots_state;
static int state;
static int cpu_count;
static int cpu_pid_type;
-static pid_t ctrl_task;
+static int ctrl_task;
static struct completion ctrl_complete;
static int critical_state;
static int rackmac;
@@ -1779,8 +1780,6 @@ static int call_critical_overtemp(void)
*/
static int main_control_loop(void *x)
{
- daemonize("kfand");
-
DBG("main_control_loop started\n");
down(&driver_lock);
@@ -1859,7 +1858,6 @@ static int main_control_loop(void *x)
machine_power_off();
}
- // FIXME: Deal with signals
elapsed = jiffies - start;
if (elapsed < HZ)
schedule_timeout_interruptible(HZ - elapsed);
@@ -1954,9 +1952,12 @@ static int create_control_loops(void)
*/
static void start_control_loops(void)
{
+ struct task_struct *task;
init_completion(&ctrl_complete);
- ctrl_task = kernel_thread(main_control_loop, NULL, SIGCHLD | CLONE_KERNEL);
+ task = kthread_run(main_control_loop, NULL, "kfand");
+ if (!IS_ERR(task))
+ ctrl_task = 1;
}
/*
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch modifies the kcmptd_ctr_%d daemon using kthread_run
instead of a combination of kernel_thread and daemonize making
the code a little simpler and more maintainable.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/bluetooth/cmtp/core.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index e1b9db9..993303f 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -35,6 +35,7 @@
#include <linux/file.h>
#include <linux/init.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <net/sock.h>
#include <linux/isdn/capilli.h>
@@ -286,7 +287,6 @@ static int cmtp_session(void *arg)
BT_DBG("session %p", session);
- daemonize("kcmtpd_ctr_%d", session->num);
set_user_nice(current, -15);
init_waitqueue_entry(&wait, current);
@@ -329,6 +329,7 @@ static int cmtp_session(void *arg)
int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
{
struct cmtp_session *session, *s;
+ struct task_struct *task;
bdaddr_t src, dst;
int i, err;
@@ -375,8 +376,9 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
__cmtp_link_session(session);
- err = kernel_thread(cmtp_session, session, CLONE_KERNEL);
- if (err < 0)
+ task = kthread_run(cmtp_session, session, "kcmtpd_ctr_%d", session->num);
+ err = PTR_ERR(task);
+ if (IS_ERR(task))
goto unlink;
if (!(session->flags & (1 << CMTP_LOOPBACK))) {
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Start the g4fand using kthread_run not a combination
of kernel_thread and deamonize. This makes the code
a little simpler and more maintainable.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/therm_windtunnel.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index a1d3a98..5d888e7 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -36,6 +36,7 @@
#include <linux/i2c.h>
#include <linux/slab.h>
#include <linux/init.h>
+#include <linux/kthread.h>
#include <asm/prom.h>
#include <asm/machdep.h>
@@ -62,7 +63,6 @@ I2C_CLIENT_INSMOD;
static struct {
volatile int running;
struct completion completion;
- pid_t poll_task;
struct semaphore lock;
struct of_device *of_dev;
@@ -285,7 +285,6 @@ restore_regs( void )
static int
control_loop( void *dummy )
{
- daemonize("g4fand");
down( &x.lock );
setup_hardware();
@@ -323,7 +322,7 @@ do_attach( struct i2c_adapter *adapter )
if( x.thermostat && x.fan ) {
x.running = 1;
init_completion( &x.completion );
- x.poll_task = kernel_thread( control_loop, NULL, SIGCHLD | CLONE_KERNEL );
+ kthread_run( control_loop, NULL, "g4fand");
}
}
return ret;
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted Cc: Neil Brown <neilb@suse.de> Cc: Trond Myklebust <trond.myklebust@fys.uio.no> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/nfs/nfs4state.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 5fffbdf..d16393f 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -775,8 +775,6 @@ static int reclaimer(void *ptr) struct rpc_cred *cred; int status = 0; - allow_signal(SIGKILL); - /* Ensure exclusive access to NFSv4 state */ lock_kernel(); down_write(&clp->cl_sem); -- 1.5.0.g53756 -
From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
kernel/synchro-test.c | 16 ++++++----------
1 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/kernel/synchro-test.c b/kernel/synchro-test.c
index a4747a6..b1d7fd6 100644
--- a/kernel/synchro-test.c
+++ b/kernel/synchro-test.c
@@ -30,6 +30,7 @@
#include <linux/timer.h>
#include <linux/completion.h>
#include <linux/mutex.h>
+#include <linux/kthread.h>
#define MAX_THREADS 64
@@ -224,7 +225,6 @@ static int mutexer(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Mutex%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -246,7 +246,6 @@ static int semaphorer(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Sem%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -268,7 +267,6 @@ static int reader(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Read%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -292,7 +290,6 @@ static int writer(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Write%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -316,7 +313,6 @@ static int downgrader(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Down%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -433,27 +429,27 @@ static int __init do_tests(void)
for (loop = 0; loop < MAX_THREADS; loop++) {
if (loop < nummx) {
init_completion(&mx_comp[loop]);
- kernel_thread(mutexer, (void *) loop, 0);
+ kthread_run(mutexer, (void *) loop, "Mutex%u", loop);
}
if (loop < numsm) {
init_completion(&sm_comp[loop]);
- kernel_thread(semaphorer, (void *) loop, 0);
+ kthread_run(semaphorer, (void *) loop, "Sem%u", loop);
}
if (loop < ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch modifies the startup of kadbprobe to use
kthread_run instead of scheduling a work event which
later calls kernel_thread and in the thread calls
daemonize and blocks signals. kthread_run is simpler
and more maintainable.
The variable pid_t adb_probe_task_pid is replaced by
a struct task_struct variable named adb_probe_task.
Which works equally well with for testing if the current
process is the adb_probe thread, does not get confused
in the presence of a pid namespace and is easier to
compare against current as it is the same type.
The result is code that is slightly simpler and easier
to maintain.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/adb.c | 32 +++++++-------------------------
1 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c
index adfea3c..09c5261 100644
--- a/drivers/macintosh/adb.c
+++ b/drivers/macintosh/adb.c
@@ -35,6 +35,7 @@
#include <linux/spinlock.h>
#include <linux/completion.h>
#include <linux/device.h>
+#include <linux/kthread.h>
#include <asm/uaccess.h>
#include <asm/semaphore.h>
@@ -82,7 +83,7 @@ struct adb_driver *adb_controller;
BLOCKING_NOTIFIER_HEAD(adb_client_list);
static int adb_got_sleep;
static int adb_inited;
-static pid_t adb_probe_task_pid;
+static struct task_struct *adb_probe_task;
static DECLARE_MUTEX(adb_probe_mutex);
static struct completion adb_probe_task_comp;
static int sleepy_trackpad;
@@ -137,8 +138,7 @@ static void printADBreply(struct adb_request *req)
static __inline__ void adb_wait_ms(unsigned int ms)
{
- if (current->pid && adb_probe_task_pid &&
- adb_probe_task_pid == current->pid)
+ if (adb_probe_task == current)
msleep(ms);
else
mdelay(ms);
@@ -245,35 +245,19 @@ static int adb_scan_bus(void)
* This kernel task ...From: Eric W. Biederman <ebiederm@xmission.com> - unquoted
This patch modifies the startup of kafscmd, kafsasyncd, and kafstimod
to use kthread_run instead of a combination of kernel_thread and
daemonize making the code slightly simpler and more maintainable.
In addition since by default all signals are ignored when delivered
to a kernel thread the code to flush signals has been removed.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/afs/cmservice.c | 10 +++++-----
fs/afs/internal.h | 11 -----------
fs/afs/kafsasyncd.c | 17 ++++++-----------
fs/afs/kafstimod.c | 16 ++++++----------
4 files changed, 17 insertions(+), 37 deletions(-)
diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c
index 3d097fd..f7e2355 100644
--- a/fs/afs/cmservice.c
+++ b/fs/afs/cmservice.c
@@ -13,6 +13,7 @@
#include <linux/init.h>
#include <linux/sched.h>
#include <linux/completion.h>
+#include <linux/kthread.h>
#include "server.h"
#include "cell.h"
#include "transport.h"
@@ -120,8 +121,6 @@ static int kafscmd(void *arg)
printk(KERN_INFO "kAFS: Started kafscmd %d\n", current->pid);
- daemonize("kafscmd");
-
complete(&kafscmd_alive);
/* loop around looking for things to attend to */
@@ -133,7 +132,6 @@ static int kafscmd(void *arg)
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
if (!list_empty(&kafscmd_attention_list) ||
- signal_pending(current) ||
kafscmd_die)
break;
@@ -297,8 +295,10 @@ int afscm_start(void)
down_write(&afscm_sem);
if (!afscm_usage) {
- ret = kernel_thread(kafscmd, NULL, 0);
- if (ret < 0)
+ struct task_struct *task;
+ task = kthread_run(kafscmd, NULL, "kafscmd");
+ ret = PTR_ERR(task);
+ if (IS_ERR(task))
goto out;
wait_for_completion(&kafscmd_alive);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 5151d5d..2d667b7 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -40,17 +40,6 @@
...From: Eric W. Biederman <ebiederm@xmission.com>
This patch just trivially replaces kernel_thread and daemonize
with a single call to kthread_run.
CC: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/i386/mach-voyager/voyager_thread.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/i386/mach-voyager/voyager_thread.c b/arch/i386/mach-voyager/voyager_thread.c
index ebfd913..ee23d9b 100644
--- a/arch/i386/mach-voyager/voyager_thread.c
+++ b/arch/i386/mach-voyager/voyager_thread.c
@@ -23,6 +23,7 @@
#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>
@@ -43,7 +44,7 @@ static __u8 set_timeout = 0;
static int __init
voyager_thread_start(void)
{
- if(kernel_thread(thread, NULL, CLONE_KERNEL) < 0) {
+ if (IS_ERR(kthread_run(thread, NULL, "%s", THREAD_NAME))) {
/* This is serious, but not fatal */
printk(KERN_ERR "Voyager: Failed to create system monitor thread!!!\n");
return 1;
@@ -122,8 +123,6 @@ thread(void *unused)
kvoyagerd_running = 1;
- daemonize(THREAD_NAME);
-
set_timeout = 0;
init_timer(&wakeup_timer);
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com>
This patches modifies the pnpbios kernel thread to start
with ktrhead_run not kernel_thread and deamonize. Doing
this makes the code a little simpler and easier to maintain.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pnp/pnpbios/core.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
index c2ed53f..3a201b7 100644
--- a/drivers/pnp/pnpbios/core.c
+++ b/drivers/pnp/pnpbios/core.c
@@ -62,6 +62,7 @@
#include <linux/delay.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <asm/page.h>
#include <asm/desc.h>
@@ -159,9 +160,7 @@ static int pnp_dock_thread(void * unused)
{
static struct pnp_docking_station_info now;
int docked = -1, d = 0;
- daemonize("kpnpbiosd");
- allow_signal(SIGKILL);
- while(!unloading && !signal_pending(current))
+ while (!unloading)
{
int status;
@@ -170,11 +169,8 @@ static int pnp_dock_thread(void * unused)
*/
msleep_interruptible(2000);
- if(signal_pending(current)) {
- if (try_to_freeze())
- continue;
- break;
- }
+ if (try_to_freeze())
+ continue;
status = pnp_bios_dock_station_info(&now);
@@ -582,6 +578,7 @@ subsys_initcall(pnpbios_init);
static int __init pnpbios_thread_init(void)
{
+ struct task_struct *task;
#if defined(CONFIG_PPC_MERGE)
if (check_legacy_ioport(PNPBIOS_BASE))
return 0;
@@ -590,7 +587,8 @@ static int __init pnpbios_thread_init(void)
return 0;
#ifdef CONFIG_HOTPLUG
init_completion(&unload_sem);
- if (kernel_thread(pnp_dock_thread, NULL, CLONE_KERNEL) > 0)
+ task = kthread_run(pnp_dock_thread, NULL, "kpnpbiosd");
+ if (!IS_ERR(task))
unloading = 0;
#endif
return 0;
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com>
Cc: Neil Brown <neilb@suse.de>
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/nfsd/nfs4state.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 678f3be..3cc8ce4 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1326,8 +1326,6 @@ do_recall(void *__dp)
{
struct nfs4_delegation *dp = __dp;
- daemonize("nfsv4-recall");
-
nfsd4_cb_recall(dp);
return 0;
}
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com>
kthread_run replaces kernel_thread and dameonize.
allow_signal is unnecessary and has been removed.
tid_poll was unused and has been removed.
Cc: Jyoti Shah <jshah@us.ibm.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pci/hotplug/ibmphp_hpc.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/hotplug/ibmphp_hpc.c b/drivers/pci/hotplug/ibmphp_hpc.c
index 46abaa8..27e12f1 100644
--- a/drivers/pci/hotplug/ibmphp_hpc.c
+++ b/drivers/pci/hotplug/ibmphp_hpc.c
@@ -34,6 +34,7 @@
#include <linux/pci.h>
#include <linux/init.h>
#include <linux/mutex.h>
+#include <linux/kthread.h>
#include "ibmphp.h"
@@ -101,7 +102,6 @@ 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
// access to data structures
@@ -137,7 +137,6 @@ void __init ibmphp_hpc_initvars (void)
init_MUTEX_LOCKED (&sem_exit);
to_debug = 0;
ibmphp_shutdown = 0;
- tid_poll = 0;
debug ("%s - Exit\n", __FUNCTION__);
}
@@ -1060,12 +1059,8 @@ 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;
}
@@ -1078,17 +1073,18 @@ static int hpc_poll_thread (void *data)
*---------------------------------------------------------------------*/
int __init ibmphp_hpc_start_poll_thread (void)
{
+ struct task_struct *task;
int rc = 0;
debug ("%s - Entry\n", __FUNCTION__);
- tid_poll = kernel_thread (hpc_poll_thread, NULL, 0);
- if (tid_poll < 0) {
+ task = kthread_run(hpc_poll_thread, NULL, ...This patch just trivial converts from calling kernel_thread and daemonize
to just calling kthread_run.
Cc: Andi Kleen <ak@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/i386/kernel/io_apic.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/i386/kernel/io_apic.c b/arch/i386/kernel/io_apic.c
index 24ac67c..84b412a 100644
--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -34,6 +34,7 @@
#include <linux/msi.h>
#include <linux/htirq.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <asm/io.h>
#include <asm/smp.h>
@@ -660,8 +661,6 @@ static int balanced_irq(void *unused)
unsigned long prev_balance_time = jiffies;
long time_remaining = balanced_irq_interval;
- daemonize("kirqd");
-
/* push everything to CPU 0 to give us a starting point. */
for (i = 0 ; i < NR_IRQS ; i++) {
irq_desc[i].pending_mask = cpumask_of_cpu(0);
@@ -721,7 +720,7 @@ static int __init balanced_irq_init(void)
}
printk(KERN_INFO "Starting balanced_irq\n");
- if (kernel_thread(balanced_irq, NULL, CLONE_KERNEL) >= 0)
+ if (!IS_ERR(kthread_run(balanced_irq, NULL, "kirqd")))
return 0;
else
printk(KERN_ERR "balanced_irq_init: failed to spawn balanced_irq");
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com>
This patch starts up khidp using kthread_run instead
of kernel_thread and daemonize, resulting is slightly
simpler and more maintainable code.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/bluetooth/hidp/core.c | 29 ++++++++++++++++-------------
1 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index df2c471..1c9b202 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
#include <linux/init.h>
#include <linux/wait.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <net/sock.h>
#include <linux/input.h>
@@ -531,22 +532,11 @@ static int hidp_session(void *arg)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
- int vendor = 0x0000, product = 0x0000;
wait_queue_t ctrl_wait, intr_wait;
BT_DBG("session %p", session);
- if (session->input) {
- vendor = session->input->id.vendor;
- product = session->input->id.product;
- }
-
- if (session->hid) {
- vendor = session->hid->vendor;
- product = session->hid->product;
- }
- daemonize("khidpd_%04x%04x", vendor, product);
set_user_nice(current, -15);
init_waitqueue_entry(&ctrl_wait, current);
@@ -747,7 +737,9 @@ static inline void hidp_setup_hid(struct hidp_session *session, struct hidp_conn
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
{
+ int vendor = 0x0000, product = 0x0000;
struct hidp_session *session, *s;
+ struct task_struct *task;
int err;
BT_DBG("");
@@ -834,8 +826,19 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
hidp_set_timer(session);
- err = kernel_thread(hidp_session, session, CLONE_KERNEL);
- if (err < 0)
+ if (session->input) {
+ vendor = ...On Thu, 19 Apr 2007 01:58:53 -0600 argh, they're all like this :( It's a shame your changelogs didn't fully spell out the reasons for this conversion. Right now, the maintainers probably think that these are nice-to-have cleanups, not must-have-to-make-virtualisation-work-right fixes. -
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. Cc: Marcel Holtmann <marcel@holtmann.org> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- net/bluetooth/rfcomm/core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index 34f993a..baaad49 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -38,6 +38,7 @@ #include <linux/net.h> #include <linux/mutex.h> #include <linux/freezer.h> +#include <linux/kthread.h> #include <net/sock.h> #include <asm/uaccess.h> @@ -1938,7 +1939,6 @@ static int rfcomm_run(void *unused) atomic_inc(&running); - daemonize("krfcommd"); set_user_nice(current, -10); BT_DBG(""); @@ -2058,7 +2058,7 @@ static int __init rfcomm_init(void) hci_register_cb(&rfcomm_cb); - kernel_thread(rfcomm_run, NULL, CLONE_KERNEL); + kthread_run(rfcomm_run, NULL, "krfcommd"); if (class_create_file(bt_class, &class_attr_rfcomm_dlc) < 0) BT_ERR("Failed to create RFCOMM info file"); -- 1.5.0.g53756 -
On Thu, 19 Apr 2007 01:58:54 -0600
We should remove the file-wide `terminate' and `running' and switch the
thread management over to kthread_run(), kthread_stop() and
kthread_should_stop().
btw, this:
static void rfcomm_worker(void)
{
BT_DBG("");
while (!atomic_read(&terminate)) {
try_to_freeze();
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;
}
appears to have the classic sleep/wakeup bug: if the wakeup happens after
we tested RFCOMM_SCHED_WAKEUP we will miss it.
Easy fix:
From: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
net/bluetooth/rfcomm/core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff -puN net/bluetooth/rfcomm/core.c~rfcomm_worker-fix-wakeup-race net/bluetooth/rfcomm/core.c
--- a/net/bluetooth/rfcomm/core.c~rfcomm_worker-fix-wakeup-race
+++ a/net/bluetooth/rfcomm/core.c
@@ -1855,18 +1855,18 @@ static void rfcomm_worker(void)
while (!atomic_read(&terminate)) {
try_to_freeze();
+ set_current_state(TASK_INTERRUPTIBLE);
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();
}
+ set_current_state(TASK_RUNNING);
/* Process stuff */
clear_bit(RFCOMM_SCHED_WAKEUP, &rfcomm_event);
rfcomm_process_sessions();
}
- set_current_state(TASK_RUNNING);
return;
}
_
(I think it's safer and saner to always run rfcomm_process_sessions() while
in state TASK_RUNNING, not maybe-in-state-TASK_INTERRUPTIBLE)
-
here's an old refreshed one using kthread_stop() and fixing the racy
wakeup.
C.
Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>
---
net/bluetooth/rfcomm/core.c | 61 +++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 40 deletions(-)
Index: 2.6.21-rc6-mm1/net/bluetooth/rfcomm/core.c
===================================================================
--- 2.6.21-rc6-mm1.orig/net/bluetooth/rfcomm/core.c
+++ 2.6.21-rc6-mm1/net/bluetooth/rfcomm/core.c
@@ -38,6 +38,7 @@
#include <linux/net.h>
#include <linux/mutex.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <net/sock.h>
#include <asm/uaccess.h>
@@ -68,7 +69,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);
@@ -1847,28 +1847,6 @@ static inline void rfcomm_process_sessio
rfcomm_unlock();
}
-static void rfcomm_worker(void)
-{
- BT_DBG("");
-
- while (!atomic_read(&terminate)) {
- try_to_freeze();
-
- 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;
@@ -1934,22 +1912,29 @@ 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);
BT_DBG("");
rfcomm_add_listener(BDADDR_ANY);
- rfcomm_worker();
+ while (!kthread_should_stop()) {
+ try_to_freeze();
...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()) ...From: Eric W. Biederman <ebiederm@xmission.com> This patch modifies the qeth_recover thread to be started with kthread_run not a combination of kernel_thread and daemonize. Resulting in slightly simpler and more maintainable code. Cc: Frank Pavlic <fpavlic@de.ibm.com> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- drivers/s390/net/qeth_main.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/s390/net/qeth_main.c b/drivers/s390/net/qeth_main.c index ad7792d..8234846 100644 --- a/drivers/s390/net/qeth_main.c +++ b/drivers/s390/net/qeth_main.c @@ -50,6 +50,7 @@ #include <linux/mii.h> #include <linux/rcupdate.h> #include <linux/ethtool.h> +#include <linux/kthread.h> #include <net/arp.h> #include <net/ip.h> @@ -957,7 +958,6 @@ qeth_recover(void *ptr) int rc = 0; card = (struct qeth_card *) ptr; - daemonize("qeth_recover"); QETH_DBF_TEXT(trace,2,"recover1"); QETH_DBF_HEX(trace, 2, &card, sizeof(void *)); if (!qeth_do_run_thread(card, QETH_RECOVER_THREAD)) @@ -1014,7 +1014,7 @@ qeth_start_kernel_thread(struct work_struct *work) card->write.state != CH_STATE_UP) return; if (qeth_do_start_thread(card, QETH_RECOVER_THREAD)) - kernel_thread(qeth_recover, (void *) card, SIGCHLD); + kthread_run(qeth_recover, card, "qeth_recover"); } -- 1.5.0.g53756 -
From: Eric W. Biederman <ebiederm@xmission.com>
This starts the sparc64 powerd using kthread_run
instead of kernel_thread and daemonize. Making the
code slightly simpler and more maintainable.
In addition the unnecessary flush_signals is removed.
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/sparc64/kernel/power.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/sparc64/kernel/power.c b/arch/sparc64/kernel/power.c
index 699b24b..03feb8b 100644
--- a/arch/sparc64/kernel/power.c
+++ b/arch/sparc64/kernel/power.c
@@ -13,6 +13,7 @@
#include <linux/interrupt.h>
#include <linux/pm.h>
#include <linux/syscalls.h>
+#include <linux/kthread.h>
#include <asm/system.h>
#include <asm/auxio.h>
@@ -81,15 +82,12 @@ static int powerd(void *__unused)
char *argv[] = { "/sbin/shutdown", "-h", "now", NULL };
DECLARE_WAITQUEUE(wait, current);
- daemonize("powerd");
-
add_wait_queue(&powerd_wait, &wait);
again:
for (;;) {
set_task_state(current, TASK_INTERRUPTIBLE);
if (button_pressed)
break;
- flush_signals(current);
schedule();
}
__set_current_state(TASK_RUNNING);
@@ -128,7 +126,9 @@ static int __devinit power_probe(struct of_device *op, const struct of_device_id
poweroff_method = machine_halt; /* able to use the standard halt */
if (has_button_interrupt(irq, op->node)) {
- if (kernel_thread(powerd, NULL, CLONE_FS) < 0) {
+ struct task_struct *task;
+ task = kthread_urn(powerd, NULL, "powerd");
+ if (IS_ERR(task)) {
printk("Failed to start power daemon.\n");
return 0;
}
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:58:39 -0600 I'll fix that up before Dave notices ;) -
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.
Cc: Swen Schillig <swen@vnet.ibm.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/s390/scsi/zfcp_erp.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index 66c0b09..f26536d 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -21,6 +21,7 @@
#define ZFCP_LOG_AREA ZFCP_LOG_AREA_ERP
+#include <linux/kthread.h>
#include "zfcp_ext.h"
static int zfcp_erp_adisc(struct zfcp_port *);
@@ -985,12 +986,13 @@ static void zfcp_erp_action_dismiss(struct zfcp_erp_action *erp_action)
int
zfcp_erp_thread_setup(struct zfcp_adapter *adapter)
{
- int retval = 0;
+ struct task_struct *task;
atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
- retval = kernel_thread(zfcp_erp_thread, adapter, SIGCHLD);
- if (retval < 0) {
+ task = kthread_run(zfcp_erp_thread, adapter,
+ "zfcperp%s", zfcp_get_busid_by_adapter(adapter));
+ if (IS_ERR(task)) {
ZFCP_LOG_NORMAL("error: creation of erp thread failed for "
"adapter %s\n",
zfcp_get_busid_by_adapter(adapter));
@@ -1002,7 +1004,7 @@ zfcp_erp_thread_setup(struct zfcp_adapter *adapter)
debug_text_event(adapter->erp_dbf, 5, "a_thset_ok");
}
- return (retval < 0);
+ return IS_ERR(task);
}
/*
@@ -1054,9 +1056,6 @@ zfcp_erp_thread(void *data)
struct zfcp_erp_action *erp_action;
unsigned long flags;
- daemonize("zfcperp%s", zfcp_get_busid_by_adapter(adapter));
- /* Block all signals */
- siginitsetinv(&current->blocked, 0);
atomic_set_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
debug_text_event(adapter->erp_dbf, 5, "a_th_run");
wake_up(&adapter->erp_thread_wqh);
-- ...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? -
I know and we have it on our schedule, but it's not as easy as it might look like . -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch changes cpqphp to use kthread_run and not
kernel_thread and daemonize to startup and setup
the cpqphp thread.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pci/hotplug/cpqphp_ctrl.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c b/drivers/pci/hotplug/cpqphp_ctrl.c
index 79ff6b4..c2c06c4 100644
--- a/drivers/pci/hotplug/cpqphp_ctrl.c
+++ b/drivers/pci/hotplug/cpqphp_ctrl.c
@@ -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,
@@ -1746,10 +1747,6 @@ static void pushbutton_helper_thread(unsigned long data)
static int event_thread(void* data)
{
struct controller *ctrl;
- lock_kernel();
- daemonize("phpd_event");
-
- unlock_kernel();
while (1) {
dbg("!!!!event_thread sleeping\n");
@@ -1771,7 +1768,7 @@ static int event_thread(void* data)
int cpqhp_event_start_thread(void)
{
- int pid;
+ struct task_struct *task;
/* initialize our semaphores */
init_MUTEX(&delay_sem);
@@ -1779,12 +1776,11 @@ int cpqhp_event_start_thread(void)
init_MUTEX_LOCKED(&event_exit);
event_finished=0;
- pid = kernel_thread(event_thread, NULL, 0);
- if (pid < 0) {
+ task = kthread_run(event_thread, NULL, "phpd_event");
+ if (IS_ERR(task)) {
err ("Can't start up our event thread\n");
return -1;
}
- dbg("Our event thread pid = %d\n", pid);
return 0;
}
--
1.5.0.g53756
-
ok.. I'll rename this to "partially convert" and shall add a note to the changelog, This is another driver which will look a lot nicer when it has been converted to kthread_should_stop() and kthread_stop() -
From: Eric W. Biederman <ebiederm@xmission.com>
thread_run is used intead of kernel_thread, daemonize, and mucking
around blocking signals directly.
CC: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/mtd/mtd_blkdevs.c | 19 +++++--------------
1 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index db7397c..ed71d5e 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -21,6 +21,7 @@
#include <linux/init.h>
#include <linux/mutex.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <asm/uaccess.h>
static LIST_HEAD(blktrans_majors);
@@ -84,17 +85,6 @@ 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) {
@@ -368,6 +358,7 @@ 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
@@ -406,13 +397,13 @@ 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;
- ret = kernel_thread(mtd_blktrans_thread, tr, CLONE_KERNEL);
- if (ret < 0) {
+ task = kthread_run(mtd_blktrans_thread, tr, "%sd", tr->name);
+ if (IS_ERR(task)) {
blk_cleanup_queue(tr->blkcore_priv->rq);
...From: Eric W. Biederman <ebiederm@xmission.com> It is my goal to replace all kernel code that handles signals from user space, calls kernel_thread or calls daemonize. All of which the kthread_api makes unncessary. Handling signals from user space is a maintenance problem becuase using a kernel thread is an implementation detail and if user space cares it does not allow us to change the implementation. Calling daemonize is a problem because it has to undo a continually changing set of state generated by user space, requiring the implemetation to change continually. kernel_thread is a problem because it returns a pid_t value. Numeric pids are inherently racy and in the presence of a pid namespace they are no longer global making them useless for general use in the kernel. So this patch renames the pid member of struct saa7134_thread started and changes it's type from pid_t to int. All it has ever been used for is to detect if the kernel thread is has been started so this works. allow_signal(SIGTERM) and the calls to signal_pending have been removed they are needed for the driver to operation. The startup of tvaudio_thread and tvaudio_thread_dep have been modified to use kthread_run instead of a combination of kernel_thread and daemonize. The result is code that is slightly simpler and more maintainable. Cc: Hartmut Hackmann <hartmut.hackmann@t-online.de> Cc: Mauro Carvalho Chehab <mchehab@infradead.org> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- drivers/media/video/saa7134/saa7134-tvaudio.c | 27 ++++++++++++------------- drivers/media/video/saa7134/saa7134.h | 2 +- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/drivers/media/video/saa7134/saa7134-tvaudio.c b/drivers/media/video/saa7134/saa7134-tvaudio.c index 7b56041..b636cb1 100644 --- a/drivers/media/video/saa7134/saa7134-tvaudio.c +++ b/drivers/media/video/saa7134/saa7134-tvaudio.c @@ -27,6 +27,7 @@ #include <linux/kernel.h> #include <linux/slab.h> ...
On Thu, 19 Apr 2007 01:58:58 -0600 This one also really wants to be converted to full use of the API. ie: use kthread_stop(), kthread_should_stop(), remove all the hand-woven equivalent stuff we have in there. I'll tag this as an -mm-only thing as well, in the hope that someone who can test the changes will be able to find time to address all this. -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch is a minimal transformation to use the kthread API
doing it's best to preserve the existing logic.
Instead of starting kdvb-ca by calling kernel_thread,
daemonize and sigfillset we kthread_run is used.
Instead of tracking the pid of the running thread we instead
simply keep a flag to indicate that the current thread is
running, as that is all the pid is really used for.
And finally the kill_proc sending signal 0 to the kernel thread to
ensure it is alive before we wait for it to shutdown is removed.
The kthread API does not provide the pid so we don't have that
information readily available and the test is just silly. If there
is no shutdown race the test is a useless confirmation of that the
thread is running. If there is a race the test doesn't fix it and
we should fix the race properly.
Cc: Andrew de Quincey <adq_dvb@lidskialf.net>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/media/dvb/dvb-core/dvb_ca_en50221.c | 46 ++++++++++----------------
1 files changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
index 2a03bf5..b28bc15 100644
--- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
@@ -37,6 +37,7 @@
#include <linux/delay.h>
#include <linux/spinlock.h>
#include <linux/sched.h>
+#include <linux/kthread.h>
#include "dvb_ca_en50221.h"
#include "dvb_ringbuffer.h"
@@ -139,8 +140,8 @@ struct dvb_ca_private {
/* wait queues for read() and write() operations */
wait_queue_head_t wait_queue;
- /* PID of the monitoring thread */
- pid_t thread_pid;
+ /* Flag indicating the monitoring thread is running */
+ int thread_running;
/* Wait queue used when shutting thread down */
wait_queue_head_t thread_queue;
@@ -982,7 +983,6 @@ static void ...On Thu, 19 Apr 2007 01:59:04 -0600 urgh, yes, this is just sad. We should convert this driver fully to the kthread API - it will end up much better. I'll queue this up as a -mm-only thing as a gentle reminder that we should do it properly. -
Here's an attempted update to the full kthread API + wake_up_process:
Index: linux-2.6/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
===================================================================
--- linux-2.6.orig/drivers/media/dvb/dvb-core/dvb_ca_en50221.c 2007-04-20 07:25:07.000000000 +0200
+++ linux-2.6/drivers/media/dvb/dvb-core/dvb_ca_en50221.c 2007-04-20 07:35:54.000000000 +0200
@@ -37,6 +37,7 @@
#include <linux/delay.h>
#include <linux/spinlock.h>
#include <linux/sched.h>
+#include <linux/kthread.h>
#include "dvb_ca_en50221.h"
#include "dvb_ringbuffer.h"
@@ -140,13 +141,7 @@ struct dvb_ca_private {
wait_queue_head_t wait_queue;
/* PID of the monitoring thread */
- pid_t thread_pid;
-
- /* Wait queue used when shutting thread down */
- wait_queue_head_t thread_queue;
-
- /* Flag indicating when thread should exit */
- unsigned int exit:1;
+ struct task_struct *thread;
/* Flag indicating if the CA device is open */
unsigned int open:1;
@@ -902,28 +897,10 @@ static void dvb_ca_en50221_thread_wakeup
ca->wakeup = 1;
mb();
- wake_up_interruptible(&ca->thread_queue);
+ wake_up_process(ca->thread);
}
/**
- * Used by the CA thread to determine if an early wakeup is necessary
- *
- * @param ca CA instance.
- */
-static int dvb_ca_en50221_thread_should_wakeup(struct dvb_ca_private *ca)
-{
- if (ca->wakeup) {
- ca->wakeup = 0;
- return 1;
- }
- if (ca->exit)
- return 1;
-
- return 0;
-}
-
-
-/**
* Update the delay used by the thread.
*
* @param ca CA instance.
@@ -982,7 +959,6 @@ static void dvb_ca_en50221_thread_update
static int dvb_ca_en50221_thread(void *data)
{
struct dvb_ca_private *ca = data;
- char name[15];
int slot;
int flags;
int status;
@@ -991,28 +967,17 @@ static int dvb_ca_en50221_thread(void *d
dprintk("%s\n", __FUNCTION__);
- /* setup kernel thread */
- snprintf(name, sizeof(name), "kdvb-ca-%i:%i", ca->dvbdev->adapter->num, ...drivers/media/dvb/dvb-core/dvb_ca_en50221.c | 84 +++--------------- 1 file changed, 16 insertions(+), 68 deletions(-) tasty! -
Indeed ! I have sent a similar patch a few months ago : http://lkml.org/lkml/2007/1/24/178 with a less aggressive diffstat though :) Andrew (de Quincey) just drop mine, if you haven't already done. Christoph's is more recent and looks better. Thanks, C. -
From: Eric W. Biederman <ebiederm@xmission.com>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
kernel/synchro-test.c | 16 ++++++----------
1 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/kernel/synchro-test.c b/kernel/synchro-test.c
index a4747a6..b1d7fd6 100644
--- a/kernel/synchro-test.c
+++ b/kernel/synchro-test.c
@@ -30,6 +30,7 @@
#include <linux/timer.h>
#include <linux/completion.h>
#include <linux/mutex.h>
+#include <linux/kthread.h>
#define MAX_THREADS 64
@@ -224,7 +225,6 @@ static int mutexer(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Mutex%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -246,7 +246,6 @@ static int semaphorer(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Sem%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -268,7 +267,6 @@ static int reader(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Read%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -292,7 +290,6 @@ static int writer(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Write%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -316,7 +313,6 @@ static int downgrader(void *arg)
{
unsigned int N = (unsigned long) arg;
- daemonize("Down%u", N);
set_user_nice(current, 19);
while (atomic_read(&do_stuff)) {
@@ -433,27 +429,27 @@ static int __init do_tests(void)
for (loop = 0; loop < MAX_THREADS; loop++) {
if (loop < nummx) {
init_completion(&mx_comp[loop]);
- kernel_thread(mutexer, (void *) loop, 0);
+ kthread_run(mutexer, (void *) loop, "Mutex%u", loop);
}
if (loop < numsm) {
init_completion(&sm_comp[loop]);
- kernel_thread(semaphorer, (void *) loop, 0);
+ kthread_run(semaphorer, (void *) loop, "Sem%u", loop);
}
if (loop < numrd) {
...From: Eric W. Biederman <ebiederm@xmission.com>
Use kthread_run to start the lcs kernel threads not a
combination of kernel_thread and daemonize. This makes
the code slightly simpler and more maintainable.
Cc: Frank Pavlic <fpavlic@de.ibm.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/s390/net/lcs.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
index 08a994f..0300d87 100644
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -36,6 +36,7 @@
#include <linux/in.h>
#include <linux/igmp.h>
#include <linux/delay.h>
+#include <linux/kthread.h>
#include <net/arp.h>
#include <net/ip.h>
@@ -1248,7 +1249,6 @@ lcs_register_mc_addresses(void *data)
struct in_device *in4_dev;
card = (struct lcs_card *) data;
- daemonize("regipm");
if (!lcs_do_run_thread(card, LCS_SET_MC_THREAD))
return 0;
@@ -1728,11 +1728,10 @@ lcs_start_kernel_thread(struct work_struct *work)
struct lcs_card *card = container_of(work, struct lcs_card, kernel_thread_starter);
LCS_DBF_TEXT(5, trace, "krnthrd");
if (lcs_do_start_thread(card, LCS_RECOVERY_THREAD))
- kernel_thread(lcs_recovery, (void *) card, SIGCHLD);
+ kthread_run(lcs_recovery, card, "lcs_recover");
#ifdef CONFIG_IP_MULTICAST
if (lcs_do_start_thread(card, LCS_SET_MC_THREAD))
- kernel_thread(lcs_register_mc_addresses,
- (void *) card, SIGCHLD);
+ kernel_run(lcs_register_mc_addresses, card, "regipm");
#endif
}
@@ -2232,7 +2231,6 @@ lcs_recovery(void *ptr)
int rc;
card = (struct lcs_card *) ptr;
- daemonize("lcs_recover");
LCS_DBF_TEXT(4, trace, "recover1");
if (!lcs_do_run_thread(card, LCS_RECOVERY_THREAD))
--
1.5.0.g53756
-
ACK for both patches, thank you Eric for the patches, will add them to my patchset for Jeff. Frank -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch modifies the startup of kadbprobe to use
kthread_run instead of scheduling a work event which
later calls kernel_thread and in the thread calls
daemonize and blocks signals. kthread_run is simpler
and more maintainable.
The variable pid_t adb_probe_task_pid is replaced by
a struct task_struct variable named adb_probe_task.
Which works equally well with for testing if the current
process is the adb_probe thread, does not get confused
in the presence of a pid namespace and is easier to
compare against current as it is the same type.
The result is code that is slightly simpler and easier
to maintain.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/adb.c | 32 +++++++-------------------------
1 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c
index adfea3c..09c5261 100644
--- a/drivers/macintosh/adb.c
+++ b/drivers/macintosh/adb.c
@@ -35,6 +35,7 @@
#include <linux/spinlock.h>
#include <linux/completion.h>
#include <linux/device.h>
+#include <linux/kthread.h>
#include <asm/uaccess.h>
#include <asm/semaphore.h>
@@ -82,7 +83,7 @@ struct adb_driver *adb_controller;
BLOCKING_NOTIFIER_HEAD(adb_client_list);
static int adb_got_sleep;
static int adb_inited;
-static pid_t adb_probe_task_pid;
+static struct task_struct *adb_probe_task;
static DECLARE_MUTEX(adb_probe_mutex);
static struct completion adb_probe_task_comp;
static int sleepy_trackpad;
@@ -137,8 +138,7 @@ static void printADBreply(struct adb_request *req)
static __inline__ void adb_wait_ms(unsigned int ms)
{
- if (current->pid && adb_probe_task_pid &&
- adb_probe_task_pid == current->pid)
+ if (adb_probe_task == current)
msleep(ms);
else
mdelay(ms);
@@ -245,35 +245,19 @@ static int adb_scan_bus(void)
* This kernel task handles ...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.
Cc: Darrick J. Wong <djwong@us.ibm.com>
Cc: James Bottomley <James.Bottomley@SteelEye.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/scsi/libsas/sas_scsi_host.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 46ba3a7..7a38ac5 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -40,6 +40,7 @@
#include <linux/blkdev.h>
#include <linux/scatterlist.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
/* ---------- SCSI Host glue ---------- */
@@ -870,7 +871,6 @@ 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);
@@ -891,19 +891,20 @@ static int sas_queue_thread(void *_sas_ha)
int sas_init_queue(struct sas_ha_struct *sas_ha)
{
- int res;
struct scsi_core *core = &sas_ha->core;
+ struct task_struct *task;
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)
+ task = kthread_run(sas_queue_thread, sas_ha,
+ "sas_queue_%d", core->shost->host_no);
+ if (!IS_ERR(task))
wait_for_completion(&queue_th_comp);
- return res < 0 ? res : 0;
+ return IS_ERR(task) ? PTR_ERR(task) : 0;
}
void sas_shutdown_queue(struct sas_ha_struct *sas_ha)
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:58:38 -0600 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 Does that wait_for_completion(&queue_th_comp) actually do anything useful? If so, what is serialising access to the single queue_th_comp? -
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 ...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 -
Should we just remove the relevant code then? Eric -
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.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/therm_pm72.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/macintosh/therm_pm72.c b/drivers/macintosh/therm_pm72.c
index b002a4b..7e9cbb7 100644
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -121,6 +121,7 @@
#include <linux/reboot.h>
#include <linux/kmod.h>
#include <linux/i2c.h>
+#include <linux/kthread.h>
#include <asm/prom.h>
#include <asm/machdep.h>
#include <asm/io.h>
@@ -161,7 +162,7 @@ static struct slots_pid_state slots_state;
static int state;
static int cpu_count;
static int cpu_pid_type;
-static pid_t ctrl_task;
+static int ctrl_task;
static struct completion ctrl_complete;
static int critical_state;
static int rackmac;
@@ -1779,8 +1780,6 @@ static int call_critical_overtemp(void)
*/
static int main_control_loop(void *x)
{
- daemonize("kfand");
-
DBG("main_control_loop started\n");
down(&driver_lock);
@@ -1859,7 +1858,6 @@ static int main_control_loop(void *x)
machine_power_off();
}
- // FIXME: Deal with signals
elapsed = jiffies - start;
if (elapsed < HZ)
schedule_timeout_interruptible(HZ - elapsed);
@@ -1954,9 +1952,12 @@ static int create_control_loops(void)
*/
static void start_control_loops(void)
{
+ struct task_struct *task;
init_completion(&ctrl_complete);
- ctrl_task = kernel_thread(main_control_loop, NULL, SIGCHLD | CLONE_KERNEL);
+ task = kthread_run(main_control_loop, NULL, "kfand");
+ if (!IS_ERR(task))
+ ctrl_task = 1;
}
/*
--
1.5.0.g53756
-
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. -
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.
Cc: Jes Sorensen <jes@sgi.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/ia64/sn/kernel/xpc_main.c | 31 +++++++++++++------------------
1 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/arch/ia64/sn/kernel/xpc_main.c b/arch/ia64/sn/kernel/xpc_main.c
index e336e16..5b53642 100644
--- a/arch/ia64/sn/kernel/xpc_main.c
+++ b/arch/ia64/sn/kernel/xpc_main.c
@@ -56,6 +56,7 @@
#include <linux/reboot.h>
#include <linux/completion.h>
#include <linux/kdebug.h>
+#include <linux/kthread.h>
#include <asm/sn/intr.h>
#include <asm/sn/sn_sal.h>
#include <asm/uaccess.h>
@@ -253,8 +254,6 @@ xpc_hb_checker(void *ignore)
/* 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);
@@ -324,8 +323,6 @@ xpc_hb_checker(void *ignore)
static int
xpc_initiate_discovery(void *ignore)
{
- daemonize(XPC_DISCOVERY_THREAD_NAME);
-
xpc_discovery();
dev_dbg(xpc_part, "discovery thread is exiting\n");
@@ -494,8 +491,6 @@ xpc_activating(void *__partid)
dev_dbg(xpc_part, "bringing partition %d up\n", partid);
- daemonize("xpc%02d", partid);
-
/*
* This thread needs to run at a realtime priority to prevent a
* significant performance degradation.
@@ -559,7 +554,7 @@ xpc_activate_partition(struct xpc_partition *part)
{
partid_t partid = XPC_PARTID(part);
unsigned long irq_flags;
- pid_t pid;
+ struct task_struct *task;
spin_lock_irqsave(&part->act_lock, irq_flags);
@@ -571,9 +566,10 @@ xpc_activate_partition(struct xpc_partition *part)
spin_unlock_irqrestore(&part->act_lock, ...On Thu, 19 Apr 2007 01:58:44 -0600 Another driver which should be fully converted to the kthread API: kthread_stop() and kthread_should_stop(). And according to my logs, this driver was added to the tree more than a year _after_ the kthread interface was made available. This isn't good. -
Andrew, Per my previous response, I'd prefer to have either Russ or Robin ack the patch doesn't break before it's pushed to Linus. I don't know much about the xpmem and I am not comfortable testing it. Cheers, Jes -
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. Thanks, Robin -
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 -
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)); - ...
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 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. Eric -
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 -
I see that the setting of 'xpc_rsvd_page->vars_pa = 0;' in xpc_init() is considered a leak by Christoph (hch), but it really is not. If you look at xpc_rsvd_page_init() where it is set up, you might see that the reserved page is something XPC gets from SAL who created it at system boot time. If XPC is rmmod'd and insmod'd again, it will be handed the same page of memory by SAL. So there is no memory leak here. As for the other suspected leaks mentioned I'm not sure what they could be. It may be the fact that XPC continues coming up when presented with error returns from register_reboot_notifier() and register_die_notifier(). There is no leak in this, just simply a loss of an early notification to other SGI system partitions that this partition is going down. A fact they will sooner or later discover on their own. A notice of this degraded functionality is written to the console. And the likelyhood that these functions should ever return an error is very, very small (currently, a superior approach. I spent a month tracking down a difficult to reproduce problem that ended up being an error return jumping to the wrong label in a goto-based unwind. Problems can arise with either approach. I've also seen the compiler generate less code for the non-goto approach. I'm not a compiler person so I can't explain this, nor can I say that it's always the case, but at one time when I did a bake off between the two approaches the non-goto approach generated less code. Having said this I have no problem with switching to a goto-based unwinding of errors if that is what the community prefers. I personally find it more Christoph is correct in that XPC has a single thread that exists throughout its lifetime, another set of threads that exist for the time that active contact with other XPCs running on other SGI system partitions exists, and finally there is a pool of threads that exist on an as needed basis once a channel connection has been established between two partitions. In ...
Patches are currently under development to allow kthreads to exit before kthread_stop is called. The big thing is that once we allow kernel threads that exited by themselves to be reaped by kthread_stop I'm not certain I understand this requirement. Do you mean block indefinitely Ugh. Can you tell me a little more about the latency issues? Is having a non-halting kthread_create enough to fix this? So you don't have to context switch several times to get the thread running? Or do you need more severe latency reductions? The more severe fix would require some significant changes to copy_process and every architecture would need to be touched to fix up copy_thread. Currently daemonize is a serious maintenance problem. Using daemonize and kernel_thread to create kernel threads is a blocker in implementing the pid namespace because of their use of pid_t. So I am motivated to get this fixed. Eric -
These threads can block waiting for a hardware DMA engine, which has a 28 After placing a message in a local message queue, one SGI system partition will interrupt another to retrieve the message. We need to minimize the time from entering XPC's interrupt handler to the time that the message can be DMA transferred and delivered to the consumer (like XPNET) to I think a non-halting kthread_create() should be sufficient. It is in effect what XPC has now in calling kernel_thread() directly. Taking it one step further, if you added the notion of a thread pool, where upon exit, a thread isn't destroyed but rather is queued ready to This would also address the problems we see with huge pid spaces for kernel threads on our largest machines. In the example from last week, we had 10 threads each on 4096 cpus. If we reworked work_queues to use the kthread_create_nonblocking() thread pool, we could probably collapse the need for having all of those per-task, per-cpu work queues. Dean -
Ok. So this is an interruptible sleep? Do you have any problems being woken up out of that interruptible sleep by kthread_stop? I am in the process of modifying kthread_stop to wake up thread in an A little different but pretty close. We call kthread_create() it prepares everything and places it on a queue and wakes up kthreadd. kthreadd then wakes up and forks the thread. After the thread has finishing setting up it will call complete on a completion so kthread_create can continue on it's merry way but it should not need to go to sleep waiting for someone to call kthread_bind. But if you can live with what I have just described that will be easy to code up. That might happen. So far I am avoiding the notion of a thread pool for as long as I can. There is some sense in it, especially in generalizing the svc thread pool code from nfs. But if I don't have to go there I would Patches have already been sent (and I don't think found problems with) that make kthreadd pid == 2, and they also modify daemonize to reparent to kthreadd instead of init. Eric -
No, the hardware DMA engine's software interface, doesn't sleep nor relinquish the CPU. But there are other spots where we do sleep No, this is fine, just avoid designing the kthread stop mechanism to require a thread being requested to stop to actually stop in some finite amount of time, and that by it not stopping other kthread stop requests are held off. Allow the thread to take as much time as it needs I was aware of this behavior of kthread_create(), which I consider 'halting' in that the thread doing the kthread_create() blocks waiting for kthreadd to get scheduled, call kernel_thread(), and then call complete(). By your mentioning a 'non-halting' kthread_create() I thought you were planning to create a new flavor of kthread_create() that called kernel_thread() directly and reparented the child thread to kthreadd. My mistake. So there will be more overhead (time-wise) for XPC in calling kthread_run() as opposed to it formerly calling kernel_thread() directly. This means that XPC will have to retain its thread pool, but I can understand you not wanting to go there. Thanks, Dean -
Okay, so I need to expand upon Christoph Hellwig's patch so that all the kthread_create()'d threads are kthread_stop()'d. This is easy to do for the XPC thread that exists for the lifetime of XPC, as well as for the threads created to manage the SGI system partitions. XPC has the one discovery thread that is created when XPC is first started and exits as soon as it has finished discovering all existing SGI system partitions. With your forthcoming change to kthread_stop() that will allow it to be called after the thread has exited, doing this one is also easy. Note that the kthread_stop() for this discovery thread won't occur until XPC is rmmod'd. This means that its task_struct will not be freed for possibly a very long time (i.e., weeks). Is that a problem? But then we come to XPC's pool of threads that deliver channel messages to the appropriate consumer (like XPNET) and can block indefinitely. As mentioned earlier there could be hundreds if not thousands of these (our systems keep getting bigger). So now requiring a kthread_stop() for each one of these becomes more of a problem, as it is a lot of task_struct pointers to maintain. Currently, XPC maintains these threads via a wait_event_interruptible_exclusive() queue so that it can wakeup as many or as few as needed at any given moment by calling wake_up_nr(). When XPC is rmmod'd, a flag is set which causes them to exit and wake_up_all() is called. Therefore XPC dosen't need to remember their pids or task_struct pointers. So what would you suggest we do for this pool of threads? Is there any way to have a version of kthread_create() that doesn't require a matching kthread_stop()? Or add a kthread_not_stopping() that does the put_task_struct() call, so as to eliminate the need for calling kthread_stop()? Or should we reconsider the kthread pool approach (and get XPC out of the thread management business altogether)? Robin Holt is putting together a proposal for how one could do a kthread pool, it should provide ...
As long as there is only one, not really. It would be good if we could
get rid of it though.
The practical problem is the race with rmmod, in particular if someone
calls rmmod while this thread is still running.
If I get clever I think this is likely solvable with something like.
kthread_maybe_stop(struct task_struct **loc)
{
struct task_struct *tsk;
tsk = xchg(loc, NULL);
if (tsk)
kthread_stop(tsk);
}
kthread_stop_self(struct task_struct **loc, int exit_code)
{
struct task_struct *tsk;
tsk = xchg(loc, NULL);
if (tsk)
put_task_struct(tsk);
do_exit(tsk);
}
I'm not quite convinced that is a common enough paradigm to implement
Good question.
The whole concept of something that feels like a core part of the
Yes. I was thinking calling it kthread_orphan or something like that.
We can't make anything like that the default, because of the modular
Eric
-
I guess I'm not seeing the race with rmmod that you're talking about? In XPC's case, rmmod calls xpc_exit() which currently does a wait_for_completion() on the discovery thread and on the other thread mentioned above. These will be changed to kthread_stop() calls. And if the discovery thread has already exited the kthread_stop() will return immediately and if not it will wait until the discovery thread has exited. rmmod won't return from xpc_exit() until both threads have exited. Any thought as to when the changes to kthread_stop() that allow it to be Again, when xpc_exit() is called by rmmod it waits for XPC's pool of threads to exit before it returns, so not a problem. Any thought as to when kthread_orphan() will get into the -mm tree? Once kthread_stop() is changed and kthread_orphan() added I can proceed with Robin has changed his mind about tieing in the management of a pool of threads with the kthread API, so there won't be the fore mentioned proposal. Thanks, Dean -
Acked-by: Dean Nelson <dcn@sgi.com>
Andrew, I've tested Eric's patch in conjunction with a fix from Christoph
Lameter, which you've already got (it cleaned up a couple of compiler errors),
and the following patch that I'd like added (it cleans up a couple of
compiler warning errors and makes a few cosmetic changes).
Thanks,
Dean
Signed-off-by: Dean Nelson <dcn@sgi.com>
Index: mm-tree/arch/ia64/sn/kernel/xpc_main.c
===================================================================
--- mm-tree.orig/arch/ia64/sn/kernel/xpc_main.c 2007-04-25 14:04:51.701213426 -0500
+++ mm-tree/arch/ia64/sn/kernel/xpc_main.c 2007-04-26 06:29:02.447330438 -0500
@@ -568,7 +568,6 @@
task = kthread_run(xpc_activating, (void *) ((u64) partid),
"xpc%02d", partid);
-
if (unlikely(IS_ERR(task))) {
spin_lock_irqsave(&part->act_lock, irq_flags);
part->act_state = XPC_P_INACTIVE;
@@ -808,7 +807,6 @@
int ignore_disconnecting)
{
unsigned long irq_flags;
- pid_t pid;
u64 args = XPC_PACK_ARGS(ch->partid, ch->number);
struct xpc_partition *part = &xpc_partitions[ch->partid];
struct task_struct *task;
@@ -840,7 +838,7 @@
(void) xpc_part_ref(part);
xpc_msgqueue_ref(ch);
- task = kthread_run(xpc_daemonize_kthread, args,
+ task = kthread_run(xpc_daemonize_kthread, (void *) args,
"xpc%02dc%d", ch->partid, ch->number);
if (IS_ERR(task)) {
/* the fork failed */
@@ -1381,7 +1379,8 @@
* activate based on info provided by SAL. This new thread is short
* lived and will exit once discovery is complete.
*/
- task = kthread_run(xpc_initiate_discovery, NULL, XPC_DISCOVERY_THREAD_NAME);
+ task = kthread_run(xpc_initiate_discovery, NULL,
+ XPC_DISCOVERY_THREAD_NAME);
if (IS_ERR(task)) {
dev_err(xpc_part, "failed while forking discovery thread\n");
-
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.
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/powerpc/platforms/pseries/eeh_event.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/eeh_event.c b/arch/powerpc/platforms/pseries/eeh_event.c
index 221dec8..fe7c2e0 100644
--- a/arch/powerpc/platforms/pseries/eeh_event.c
+++ b/arch/powerpc/platforms/pseries/eeh_event.c
@@ -23,6 +23,7 @@
#include <linux/mutex.h>
#include <linux/pci.h>
#include <linux/workqueue.h>
+#include <linux/kthread.h>
#include <asm/eeh_event.h>
#include <asm/ppc-pci.h>
@@ -59,7 +60,6 @@ static int eeh_event_handler(void * dummy)
struct eeh_event *event;
struct pci_dn *pdn;
- daemonize ("eehd");
set_current_state(TASK_INTERRUPTIBLE);
spin_lock_irqsave(&eeh_eventlist_lock, flags);
@@ -105,7 +105,7 @@ static int eeh_event_handler(void * dummy)
*/
static void eeh_thread_launcher(struct work_struct *dummy)
{
- if (kernel_thread(eeh_event_handler, NULL, CLONE_KERNEL) < 0)
+ if (IS_ERR(kthread_run(eeh_event_handler, NULL, "eehd")))
printk(KERN_ERR "Failed to start EEH daemon\n");
}
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:58:45 -0600 You're making me look at a lot of things which I'd prefer not to have This one kicks off a kernel thread in response to each "PCI error event", and that kernel thread hangs about for one hour then exits. One wonders what happens if we get 1,000,000 of these events per second. -
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. -
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 -
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 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). Ben. -
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 -
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). Ben. -
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 -
The EEH code can't be modular, and wouldn't make any sense to be modular, since it's part of the infrastructure for accessing PCI devices. Paul. -
Agreed. However most kthread users are modular and make sense to be so we need to design to handle modular users. I don't think the idiom of go fire off a thread to handle something is specific to non-modular users. Eric -
Which is, btw, I think a total misdesign of our module stuff, but heh, I remember that lead to some flamewars back then... Like anything else, modules should have separated the entrypoints for - Initiating a removal request - Releasing the module The former is use did "rmmod", can unregister things from subsystems, etc... (and can file if the driver decides to refuse removal requests when it's busy doing things or whatever policy that module wants to implement). The later is called when all references to the modules have been dropped, it's a bit like the kref "release" (and could be implemented as one). If we had done that (simple) thing back then, module refcounting would have been much less of a problem... I remember some reasons why that was veto'ed but I didn't and still don't agree. Ben. -
The basic point is because a thread can terminate sooner if we have an explicit request to stop, we need that in the design. Because we need to find the threads to request that they stop we need to have some way to track them. Since we need to have some way to track them having an explicit data structure that the callers manage seems to make sense. Eric -
Oh sure, I wasn't arguing against that at all... It might be handy to have a release() callback (optional) that gets called after the kthread stops/exits, once we know the data structure isn't going to be used anymore (if practical to implement, depends on your approach). Ben. -
On Sun, 22 Apr 2007 19:00:46 -0700, "Eric Hopper" There was a thread the other day, that talked about Reiser4. It took a while but I have found it (actually two) http://lkml.org/lkml/2007/4/5/360 http://lkml.org/lkml/2007/4/9/4 You may want to check them out. -- lkml777@123mail.org -- http://www.fastmail.fm - Access your email from home and the web -
I did. That whole thread is some guy spouting off a ludicrous Bonnie++ benchmark showing that compressing long strings of 0s results in things taking up very little space and being very fast. Such things will produce lots of flames and no useful information whatsoever as is evinced by the half conspiracy theory, half truth the thread degenerated into in the second message you linked to. *sigh*, --=20 Eric Hopper (http://www.omnifarious.org/~hopper/)
On Tue, 24 Apr 2007 15:00:42 +1000, That sounds quite similar to the problems we have with kobject refcounting vs. module unloading. The patchset I posted at http://marc.info/?l=linux-kernel&m=117679014404994&w=2 exposes the refcount of the kobject embedded in the module. Maybe the kthread code could use that reference as well? -
It would be nice to have threads that can be "fired off" from an interrupt context. That would simplify the EEH code slightly (removing a few dozen lines of code that do this bounce). I presume that various device drivers might find this useful as well. --linas -
What specifically has to be done to reap a kernel thread? Are you concerned about the number of threads, or about having zombies hanging around? Paul. -
I'm mostly concerned about number of threads and possible leakage of threads. Linas already explained it's not a problem in this case, so it's covered. -
For the patch that touched arch/powerpc/platforms/pseries/eeh_event.c, I ran a variety of tests, and couldn't see/find/evoke any adverse effects, so .. Yes, exactly; all I really want is to start a thread from an interrupt context, and pass a structure to it. This is pretty much all that arch/powerpc/platforms/pseries/eeh_event.c is trying to do, and little else. --linas -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch modifies the startup of krxtimod, krxiod, and krxsecd
to use kthread_run instead of a combination of kernel_thread
and daemonize making the code slightly simpler and more maintainable.
In addition since by default all signals are ignored when delivered
to a kernel thread the code to flush signals has been removed.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/rxrpc/internal.h | 11 -----------
net/rxrpc/krxiod.c | 16 ++++++++--------
net/rxrpc/krxsecd.c | 16 ++++++++--------
net/rxrpc/krxtimod.c | 15 ++++++---------
4 files changed, 22 insertions(+), 36 deletions(-)
diff --git a/net/rxrpc/internal.h b/net/rxrpc/internal.h
index cc0c579..1dd69aa 100644
--- a/net/rxrpc/internal.h
+++ b/net/rxrpc/internal.h
@@ -49,17 +49,6 @@ __RXACCT_DECL(extern atomic_t rxrpc_message_count);
#define _net(FMT, a...) do { if (rxrpc_knet) knet (FMT , ##a); } while(0)
#endif
-static inline void rxrpc_discard_my_signals(void)
-{
- while (signal_pending(current)) {
- siginfo_t sinfo;
-
- spin_lock_irq(&current->sighand->siglock);
- dequeue_signal(current, &current->blocked, &sinfo);
- spin_unlock_irq(&current->sighand->siglock);
- }
-}
-
/*
* call.c
*/
diff --git a/net/rxrpc/krxiod.c b/net/rxrpc/krxiod.c
index bbbcd6c..c590ccd 100644
--- a/net/rxrpc/krxiod.c
+++ b/net/rxrpc/krxiod.c
@@ -14,6 +14,7 @@
#include <linux/spinlock.h>
#include <linux/init.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <rxrpc/krxiod.h>
#include <rxrpc/transport.h>
#include <rxrpc/peer.h>
@@ -43,8 +44,6 @@ static int rxrpc_krxiod(void *arg)
printk("Started krxiod %d\n",current->pid);
- daemonize("krxiod");
-
/* loop around waiting for work to do */
do {
/* wait for work or to be told to exit */
@@ -57,8 +56,7 @@ static int rxrpc_krxiod(void *arg)
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
...Again, please drop in favour of my RxRPC patches. David -
What is the ETA on your patches? Eric -
That depends on Dave Miller now, I think. I'm assuming they need to go through the network GIT tree to get to Linus. Certainly Andrew Morton seems to think so. David -
Ok. I don't see any patches in -mm so I was assuming these patches have not been queued up anywhere. As long as these things happen in a reasonably timely manner so I can remove the export of kernel_thread and kill daemonize I really don't care. If you have written them they are quite likely better then my minimal patches which were quite restrained because of my limited ability to test these things. Eric -
They haven't been quite yet. Is it your intention to kill these features in 2.6.22? David -
That is my goal, and I have patches that should do it. We will see what happens. Eric -
From: David Howells <dhowells@redhat.com> I applied already the patches I thought were appropriate, you had some crypto layer changes that you need to work out with Herbert Xu before the rest can be applied. Let me know what the status is of that stuff. Either way, I don't think it should block Eric's work here. If his stuff is more ready first, it should go in and then you have a little bit of patch merging to do that's all. -
He has already fixed it by using the scatterlist interface for now. So the last set of patches he posted is ready for merging into net-2.6.22. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -
Should the rest of it go via Andrew's tree then? David -
From: David Howells <dhowells@redhat.com> Now that Herbert cleared up the crypto layer issues the only problem left is that there are generic changes in there which are not strictly networking but which your subsequent networking changes depend upon. This is a mess, and makes merging your work into the net-2.6.22 tree more difficult. Is it possible for your changes to be purely networking and not need those changes outside of the networking? I guess one of them was just a symbol export which I could add to the net-2.6.22 tree, but weren't there some more involved non-networking bits in there? -
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.
And one that AFS depends on:
(3) Cache the key in nameidata. I still don't have Al's agreement on this,
but it's purely caching, so I could drop that patch for the moment and
excise the stuff that uses it from my AFS patches if that would help.
Do you class the AFS patches as "networking changes"?
Do you want me to consolidate my patches to make things simpler for you?
Do you want me to rebase my patches onto net-2.6.22?
I have the following patches, in order, available now, though I haven't yet
released the last few (they can all be downloaded from my RH people pages):
move-skb-generic.diff (you've got this)
timers.diff
keys.diff
af_rxrpc.diff
afs-cleanup.diff
af_rxrpc-kernel.diff
af_rxrpc-afs.diff
af_rxrpc-delete-old.diff
af_rxrpc-own-workqueues.diff
af_rxrpc-fixes.diff
afs-callback-wq.diff
afs-vlocation.diff
afs-multimount.diff
afs-rxrpc-key.diff
afs-nameidata-key.diff
afs-security.diff
afs-doc.diff
netlink-support-MSG_TRUNC.diff (you've got this)
afs-get-capabilities.diff
afs-initcallbackstate3.diff
afs-dir-write-support.diff
David
-
On Fri, 20 Apr 2007 11:41:46 +0100 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. -
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. -
Sorry, I should have been more clear. I meant the assumption that we only
But if del_timer() returns 0, then there may be a problem. We can't tell the
difference between the following two cases:
(1) The timer hadn't been started.
(2) The timer had been started, has expired and is no longer pending, but
another CPU is running its handler routine.
try_to_del_timer_sync() _does_, however, distinguish between these cases: the
first is the 0 return, the second is the -1 return, and the case where it
dequeued the timer is the 1 return.
BTW, can a timer handler be preempted? I assume not... But it can be delayed
by interrupt processing.
David
-
Of course, del_timer() and del_timer_sync() are different. What I meant the latter buys nothing for cancel_delayed_work() (which in fact could be named try_to_cancel_delayed_work()). Let's look at (2). cancel_delayed_work() (on top of del_timer()) returns 0, and this is correct, we failed to cancel the timer, and we don't know whether work->func() finished, or not. The current code uses del_timer_sync(). It will also return 0. However, it will spin waiting for timer->function() to complete. So we are just wasting CPU. I guess I misunderstood you. Perhaps, you propose a new helper which use try_to_del_timer_sync(), yes? Unless I missed something, this doesn't help. Because the return value == -1 should be treated as 0. We failed to stop the timer, and we can't free dwork. IOW, currently we should do: if (!cancel_delayed_work(dwork)) cancel_work_sync(dwork)); The same if we use del_timer(). If we use try_to_del_timer_sync(), if (cancel_delayed_work(dwork) <= 0) cancel_work_sync(dwork)); (of course, dwork shouldn't re-arm itself). No, it can't be preempted, it runs in softirq context. Oleg. -
That's my objection to using cancel_delayed_work() as it stands, although in most cases it's a relatively minor waste of time. However, if the timer expiry routine gets interrupted then it may not be so minor... So, yes, I'm Consider how I'm using try_to_cancel_delayed_work(): I use this when I want to queue a delayed work item with a particular timeout (usually for immediate processing), but the work item may already be pending. If try_to_cancel_delayed_work() returns 0 or 1 (not pending or pending but dequeued) then I can go ahead and just schedule the work item (I'll be holding a lock to prevent anyone else from interfering). However, if try_to_cancel_delayed_work() returns -1 then there's no usually no point attempting to schedule the work item because I know the timer expiry handler is doing that or going to do that. The code looks like this in pretty much all cases: if (try_to_cancel_delayed_work(&afs_server_reaper) >= 0) schedule_delayed_work(&afs_server_reaper, 0); And so could well be packaged into a convenience routine and placed in workqueue.[ch]. However, this would still concern Dave Miller as my patches would still be altering non-net stuff or depending on non-net patches he doesn't have in his GIT tree. Using cancel_delayer_work() instead would be acceptable, functionally, as that just waits till the -1 return case no longer holds true, and so always returns 0 or 1. In RxRPC, this is only used to cancel a pair global delayed work items in the rmmod path, and so the inefficiency of cancel_delayed_work() is something I can live with, though it's something I'd want to reduce in the longer term. In AFS, this is not only used in object destruction paths, but is also used to cancel the callback timer and initiate synchronisation processing with immediate effect. David -
Aha, now I see what you mean. However. Why the code above is better then
cancel_delayed_work(&afs_server_reaper);
schedule_delayed_work(&afs_server_reaper, 0);
? (I assume we already changed cancel_delayed_work() to use del_timer).
If delayed_work_timer_fn() is not running - both variants (let's denote them
as 1 and 2) do the same.
Now suppose that delayed_work_timer_fn() is running.
1: lock_timer_base(), return -1, skip schedule_delayed_work().
2: check timer_pending(), return 0, call schedule_delayed_work(),
return immediately because test_and_set_bit(WORK_STRUCT_PENDING)
fails.
So I still don't think try_to_del_timer_sync() can help in this particular
case.
To some extent, try_to_cancel_delayed_work is
int try_to_cancel_delayed_work(dwork)
{
ret = cancel_delayed_work(dwork);
if (!ret && work_pending(&dwork->work))
ret = -1;
return ret;
}
iow, work_pending() looks like a more "precise" indication that work->func()
is going to run soon.
Oleg.
-
I didn't say I necessarily agreed that this was a good idea. I just meant that I agree that it will waste CPU. You must still audit all uses of Because calling schedule_delayed_work() is a waste of CPU if the timer expiry handler is currently running at this time as *that* is going to also schedule I don't see what you're illustrating here. Are these meant to be two steps in Ah, but the timer routine may try to set the work item pending flag *after* the work_pending() check you have here. Furthermore, it would be better to avoid the work_pending() check entirely because that check involves interacting with atomic ops done on other CPUs. try_to_del_timer_sync() returning -1 tells us without a shadow of a doubt that the work item is either scheduled now or will be scheduled very shortly, thus allowing us to avoid having to do it ourself. David -
Sure, I'll grep for cancel_delayed_work(). But unless I missed something, this change should be completely transparent for all users. Otherwise, it Yes. But otoh, try_to_del_timer_sync() is a waste of CPU compared to del_timer(), two alternate steps. 1 means if (try_to_cancel_delayed_work()) schedule_delayed_work(); 2 means cancel_delayed_work(); Sure, the implementation of try_to_cancel_delayed_work() above is just for First, this is very unlikely event, delayed_work_timer_fn() is very fast unless interrupted. _PENDING flag won't be cleared until this work is executed by run_workqueue(). In generak, work_pending() after del_timer() is imho better way to avoid the unneeded schedule_delayed_work(). But again, I can't undertand the win for that particular case. Oleg. -
I guess you will have to make sure that cancel_delayed_work() is always
followed by a flush of the workqueue, otherwise you might get this situation:
CPU 0 CPU 1
=============================== =======================
<timer expires>
cancel_delayed_work(x) == 0 -->delayed_work_timer_fn(x)
kfree(x); -->do_IRQ()
y = kmalloc(); // reuses x
<--do_IRQ()
__queue_work(x)
--- OOPS ---
That's my main concern. If you are certain that can't happen, then fair
enough.
Note that although you can call cancel_delayed_work() from within a work item
handler, you can't then follow it up with a flush as it's very likely to
I suppose that's true. As previously stated, my main objection to del_timer()
is the fact that it doesn't tell you if the timer expiry function is still
running.
Can you show me a patch illustrating exactly how you want to change
cancel_delayed_work()? I can't remember whether you've done so already, but
if you have, I can't find it. Is it basically this?:
static inline int cancel_delayed_work(struct delayed_work *work)
{
int ret;
- ret = del_timer_sync(&work->timer);
+ ret = del_timer(&work->timer);
if (ret)
work_release(&work->work);
return ret;
}
I was thinking this situation might be a problem:
CPU 0 CPU 1
=============================== =======================
<timer expires>
cancel_delayed_work(x) == 0 -->delayed_work_timer_fn(x)
schedule_delayed_work(x,0) -->do_IRQ()
<keventd scheduled>
x->work()
<--do_IRQ()
__queue_work(x)
Good point. I don't think that's a problem because cancel_delayed_work()
Yeah, I guess so.
Okay, you've convinced me, I think - provided you consider the case I
outlinded at the top of this email.
If you give me a patch to alter cancel_delayed_work(), I'll substitute it for
mine and use that that instead. Dave Miller will just have to live with that
patch being there:-)
David
-
Yes sure. Note that this is documented: /* * Kill off a pending schedule_delayed_work(). Note that the work callback * function may still be running on return from cancel_delayed_work(). Run * flush_workqueue() or cancel_work_sync() to wait on it. */ This comment is not very precise though. If the work doesn't re-arm itself, we need cancel_work_sync() only if cancel_delayed_work() returns 0. So there is no difference with the proposed change. Except, return value == 0 means: currently (del_timer_sync): callback may still be running or scheduled with del_timer: may still be running, or scheduled, or will be scheduled right now. Yes, exactly. The patch is trivial, but I need some time to write the Yes, I think this should be OK. schedule_delayed_work() will notice _PENDING and abort, so the last "x->work()" doesn't happen. What can happen is <timer expires> cancel_delayed_work(x) == 0 -->delayed_work_timer_fn(x) __queue_work(x) <keventd scheduled> x->work() schedule_delayed_work(x,0) <the work is scheduled again> , so we can have an "unneeded schedule", but this is very unlikely. Oleg. -
No, it isn't documented. It says that the *work* callback may be running, but does not mention the timer callback. However, just looking at the cancellation function source made it clear that this would wait for the timer handler to return first. However, is it worth just making cancel_delayed_work() a void function and not returning anything? I'm not sure the return value is very useful. David -
cancel_rearming_delayed_work() needs this, tty_io.c, probably somebody else. Oleg. -
Yeah... If you could amend that as part of your patch, that'd be great. David -
See my latest patchset release. I've reduced the dependencies on
non-networking changes to:
(1) Oleg Nesterov's patch to change cancel_delayed_work() to use del_timer()
rather than del_timer_sync() [patch 02/16].
This patch can be discarded without compilation failure at the expense of
making AFS slightly less efficient. It also makes AF_RXRPC slightly less
efficient, but only in the rmmod path.
(2) A symbol export in the keyring stuff plus a proliferation of the types
available in the struct key::type_data union [patch 03/16]. This does
not conflict with any other patches that I know about.
(3) A symbol export in the timer stuff [patch 04/16].
Everything else that remains after the reduction is confined to the AF_RXRPC
or AFS code, save for a couple of networking patches in my patchset that you
already have and I just need to make the thing compile.
I'm not sure that I can make the AF_RXRPC patches totally independent of the
AFS patches as the two sets need to interleave since the last AF_RXRPC patch
deletes the old RxRPC code - which the old AFS code depends on.
David
-
On Thu, 19 Apr 2007 10:32:38 +0100 Do those patches convert all this code over to full use of the kthread API? Because it seems that a conversion would be straightforward, and is needed. -
No. They delete all that code entirely and use workqueues instead. So, I suppose merging Eric's patches first should be a simple matter of just deleting his revised code instead. David -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch modifies the startup of kafscmd, kafsasyncd, and kafstimod
to use kthread_run instead of a combination of kernel_thread and
daemonize making the code slightly simpler and more maintainable.
In addition since by default all signals are ignored when delivered
to a kernel thread the code to flush signals has been removed.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/afs/cmservice.c | 10 +++++-----
fs/afs/internal.h | 11 -----------
fs/afs/kafsasyncd.c | 17 ++++++-----------
fs/afs/kafstimod.c | 16 ++++++----------
4 files changed, 17 insertions(+), 37 deletions(-)
diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c
index 3d097fd..f7e2355 100644
--- a/fs/afs/cmservice.c
+++ b/fs/afs/cmservice.c
@@ -13,6 +13,7 @@
#include <linux/init.h>
#include <linux/sched.h>
#include <linux/completion.h>
+#include <linux/kthread.h>
#include "server.h"
#include "cell.h"
#include "transport.h"
@@ -120,8 +121,6 @@ static int kafscmd(void *arg)
printk(KERN_INFO "kAFS: Started kafscmd %d\n", current->pid);
- daemonize("kafscmd");
-
complete(&kafscmd_alive);
/* loop around looking for things to attend to */
@@ -133,7 +132,6 @@ static int kafscmd(void *arg)
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
if (!list_empty(&kafscmd_attention_list) ||
- signal_pending(current) ||
kafscmd_die)
break;
@@ -297,8 +295,10 @@ int afscm_start(void)
down_write(&afscm_sem);
if (!afscm_usage) {
- ret = kernel_thread(kafscmd, NULL, 0);
- if (ret < 0)
+ struct task_struct *task;
+ task = kthread_run(kafscmd, NULL, "kafscmd");
+ ret = PTR_ERR(task);
+ if (IS_ERR(task))
goto out;
wait_for_completion(&kafscmd_alive);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 5151d5d..2d667b7 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -40,17 +40,6 @@
#define ...Please drop this patch for the moment as I have my own patches to convert them to keventd-type threads, in addition to implementing a host of other changes. David -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch modifies the kcmptd_ctr_%d daemon using kthread_run
instead of a combination of kernel_thread and daemonize making
the code a little simpler and more maintainable.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/bluetooth/cmtp/core.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index e1b9db9..993303f 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -35,6 +35,7 @@
#include <linux/file.h>
#include <linux/init.h>
#include <linux/freezer.h>
+#include <linux/kthread.h>
#include <net/sock.h>
#include <linux/isdn/capilli.h>
@@ -286,7 +287,6 @@ static int cmtp_session(void *arg)
BT_DBG("session %p", session);
- daemonize("kcmtpd_ctr_%d", session->num);
set_user_nice(current, -15);
init_waitqueue_entry(&wait, current);
@@ -329,6 +329,7 @@ static int cmtp_session(void *arg)
int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
{
struct cmtp_session *session, *s;
+ struct task_struct *task;
bdaddr_t src, dst;
int i, err;
@@ -375,8 +376,9 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
__cmtp_link_session(session);
- err = kernel_thread(cmtp_session, session, CLONE_KERNEL);
- if (err < 0)
+ task = kthread_run(cmtp_session, session, "kcmtpd_ctr_%d", session->num);
+ err = PTR_ERR(task);
+ if (IS_ERR(task))
goto unlink;
if (!(session->flags & (1 << CMTP_LOOPBACK))) {
--
1.5.0.g53756
-
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. Cc: Paul Mackerras <paulus@samba.org> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- arch/powerpc/platforms/pseries/rtasd.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/platforms/pseries/rtasd.c b/arch/powerpc/platforms/pseries/rtasd.c index 77d0937..919a374 100644 --- a/arch/powerpc/platforms/pseries/rtasd.c +++ b/arch/powerpc/platforms/pseries/rtasd.c @@ -20,6 +20,7 @@ #include <linux/spinlock.h> #include <linux/cpu.h> #include <linux/delay.h> +#include <linux/kthread.h> #include <asm/uaccess.h> #include <asm/io.h> @@ -429,8 +430,6 @@ static int rtasd(void *unused) int event_scan = rtas_token("event-scan"); int rc; - daemonize("rtasd"); - if (event_scan == RTAS_UNKNOWN_SERVICE || get_eventscan_parms() == -1) goto error; @@ -497,7 +496,7 @@ static int __init rtas_init(void) else printk(KERN_ERR "Failed to create error_log proc entry\n"); - if (kernel_thread(rtasd, NULL, CLONE_FS) < 0) + if (IS_ERR(kthread_run(rtasd, NULL, "rtasd"))) printk(KERN_ERR "Failed to start RTAS daemon\n"); return 0; -- 1.5.0.g53756 -
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. -
From: Eric W. Biederman <ebiederm@xmission.com>
This patch modifies the startup of the media_bay_task
to use kthread_run and not a combination of kernel_thread,
deamonize and sigfillset.
In addition since we now always want to ignore signals
the MB_IGNORE_SIGNALS define is removed along with the
test for signal_pending.
The result is slightly simpler code that is more
maintainable.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/mediabay.c | 11 ++---------
1 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/macintosh/mediabay.c b/drivers/macintosh/mediabay.c
index c803d2b..90c853e 100644
--- a/drivers/macintosh/mediabay.c
+++ b/drivers/macintosh/mediabay.c
@@ -20,6 +20,7 @@
#include <linux/stddef.h>
#include <linux/init.h>
#include <linux/ide.h>
+#include <linux/kthread.h>
#include <asm/prom.h>
#include <asm/pgtable.h>
#include <asm/io.h>
@@ -35,7 +36,6 @@
#define MB_DEBUG
-#define MB_IGNORE_SIGNALS
#ifdef MB_DEBUG
#define MBDBG(fmt, arg...) printk(KERN_INFO fmt , ## arg)
@@ -622,11 +622,6 @@ static int media_bay_task(void *x)
{
int i;
- strcpy(current->comm, "media-bay");
-#ifdef MB_IGNORE_SIGNALS
- sigfillset(&current->blocked);
-#endif
-
for (;;) {
for (i = 0; i < media_bay_count; ++i) {
down(&media_bays[i].lock);
@@ -636,8 +631,6 @@ static int media_bay_task(void *x)
}
msleep_interruptible(MB_POLL_DELAY);
- if (signal_pending(current))
- return 0;
}
}
@@ -699,7 +692,7 @@ static int __devinit media_bay_attach(struct macio_dev *mdev, const struct of_de
/* Startup kernel thread */
if (i == 0)
- kernel_thread(media_bay_task, NULL, CLONE_KERNEL);
+ kthread_run(media_bay_task, NULL, "media-bay");
return 0;
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:58:50 -0600 Looks OK - there's no way of stopping the kernel thread anyway. It appears that nobody has tried to use this driver at the same time as software-suspend. At least, not successfully. A strategic try_to_freeze() should fix it. This will become (a little) more serious when cpu hotplug is switched to use the process freezer, and perhaps it breaks kprobes already. -
I'll dig a box with that hardware and do some tests, but it looks nice. Thanks Eric ! There should be no problem with cpu hotplug, the only machines using the media bay driver are old Apple laptops with only one CPU and no HW threads. Ben. -
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.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/bluetooth/bnep/core.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index a9f1e88..de3caed 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -32,6 +32,7 @@
#include <linux/module.h>
#include <linux/kernel.h>
+#include <linux/kthread.h>
#include <linux/sched.h>
#include <linux/signal.h>
#include <linux/init.h>
@@ -473,7 +474,6 @@ static int bnep_session(void *arg)
BT_DBG("");
- daemonize("kbnepd %s", dev->name);
set_user_nice(current, -15);
init_waitqueue_entry(&wait, current);
@@ -539,6 +539,7 @@ static struct device *bnep_get_device(struct bnep_session *session)
int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
{
+ struct task_struct *task;
struct net_device *dev;
struct bnep_session *s, *ss;
u8 dst[ETH_ALEN], src[ETH_ALEN];
@@ -598,9 +599,10 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
__bnep_link_session(s);
- err = kernel_thread(bnep_session, s, CLONE_KERNEL);
- if (err < 0) {
+ task = kthread_run(bnep_session, s, "kbnepd %s", dev->name);
+ if (IS_ERR(task)) {
/* Session thread start failed, gotta cleanup. */
+ err = PTR_ERR(task);
unregister_netdev(dev);
__bnep_unlink_session(s);
goto failed;
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:58:51 -0600
while (!atomic_read(&s->killed)) {
It's unusual to have a kernel thread which has a space in its name. That
could trip up infufficient-defensive userspace tools.
-
yes. we need something like :
- while (!atomic_read(&s->killed)) {
+ while (1) {
try_to_freeze();
set_current_state(TASK_INTERRUPTIBLE);
+ if (atomic_read(&s->killed))
+ break;
+
but we can't just change it, can we ? i could be used by a user space tool
to check if the thread is running.
C.
-
here's the refreshed version not taking into account the space in its
kernel thread name.
C.
Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>
---
net/bluetooth/bnep/core.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
Index: 2.6.21-rc6-mm1/net/bluetooth/bnep/core.c
===================================================================
--- 2.6.21-rc6-mm1.orig/net/bluetooth/bnep/core.c
+++ 2.6.21-rc6-mm1/net/bluetooth/bnep/core.c
@@ -47,6 +47,7 @@
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/skbuff.h>
+#include <linux/kthread.h>
#include <asm/unaligned.h>
@@ -473,16 +474,18 @@ static int bnep_session(void *arg)
BT_DBG("");
- daemonize("kbnepd %s", dev->name);
set_user_nice(current, -15);
init_waitqueue_entry(&wait, current);
add_wait_queue(sk->sk_sleep, &wait);
- while (!atomic_read(&s->killed)) {
+ while (1) {
try_to_freeze();
set_current_state(TASK_INTERRUPTIBLE);
+ if (atomic_read(&s->killed))
+ break;
+
// RX
while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
skb_orphan(skb);
@@ -539,6 +542,7 @@ static struct device *bnep_get_device(st
int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
{
+ struct task_struct *task;
struct net_device *dev;
struct bnep_session *s, *ss;
u8 dst[ETH_ALEN], src[ETH_ALEN];
@@ -598,9 +602,10 @@ int bnep_add_connection(struct bnep_conn
__bnep_link_session(s);
- err = kernel_thread(bnep_session, s, CLONE_KERNEL);
- if (err < 0) {
- /* Session thread start failed, gotta cleanup. */
+ task = kthread_run(bnep_session, s, "kbnepd %s", dev->name);
+ if (IS_ERR(task)) {
+ /* Session thread start failed, gotta cleanup. */
+ err = PTR_ERR(task);
unregister_netdev(dev);
__bnep_unlink_session(s);
goto failed;
-
Hello, But all kernel threads are supposed to be only *in-kernel* implementation details. Isn't a userspace tool whose behaviour relies on the existence (or even the knowledge of the existence) of any Yes, so although userspace shouldn't be bothering with kernel threads in the first place, that does not mean that such tools do not exist. So we'll have to live with this (unfortunate) naming for some time, till we can get rid of it later. Which is similar to the habit of some kernel threads in there that actually *do* want to export the knowledge of their existence (and even a signals-based interface!) to userspace. Eric did receive some nacks on his patches that tried to remove the signals business from kernel threads on this account, but perhaps that too is something that we could get rid of later (hopefully by that time those using signals in kernel threads would have realized their folly and shifted to something else :-) Satyam -
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. -
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 ...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 -
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 This is not true anymore, see another patch from Eric kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Oleg. -
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 -
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
-
Speaking about helpers, could we also add kthread_start(), which should be used instead of direct wake_up_process() ? Not that it is terribly important, but still. Note that "kthread_create() pins the task_struct" allows us to cleanup the code. Look at this ugly "wait_to_die:" label in migration_thread(). Is is because migration_thread() can't exit until CPU_DEAD reaps it. Other reasons were already solved by kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Oleg. -
I *am* already confused... a driver of mine does:
static __init int thkd_init(void)
{
touch_task = kthread_run(touch_thread, Device, "thkd");
...
}
and
static __exit void thkd_exit(void)
{
kthread_stop(touch_task);
/* I bet something is missing */
}
now what good would kthread_run do me?
Jan
--
-
Please read the kerneldoc documentation for kthread_create -
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.
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
arch/arm/kernel/ecard.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm/kernel/ecard.c b/arch/arm/kernel/ecard.c
index f1c0fb9..6c15f5f 100644
--- a/arch/arm/kernel/ecard.c
+++ b/arch/arm/kernel/ecard.c
@@ -40,6 +40,7 @@
#include <linux/device.h>
#include <linux/init.h>
#include <linux/mutex.h>
+#include <linux/kthread.h>
#include <asm/dma.h>
#include <asm/ecard.h>
@@ -263,8 +264,6 @@ static int ecard_init_mm(void)
static int
ecard_task(void * unused)
{
- daemonize("kecardd");
-
/*
* Allocate a mm. We're not a lazy-TLB kernel task since we need
* to set page table entries where the user space would be. Note
@@ -1058,13 +1057,14 @@ ecard_probe(int slot, card_type_t type)
*/
static int __init ecard_init(void)
{
- int slot, irqhw, ret;
+ struct task_struct *task;
+ int slot, irqhw;
- ret = kernel_thread(ecard_task, NULL, CLONE_KERNEL);
- if (ret < 0) {
+ task = kthread_run(ecard_task, NULL, "kecardd");
+ if (IS_ERR(task)) {
printk(KERN_ERR "Ecard: unable to create kernel thread: %d\n",
- ret);
- return ret;
+ PTR_ERR(task));
+ return PTR_ERR(task);
}
printk("Probing expansion cards\n");
--
1.5.0.g53756
-
Looks good. Given that this is non-modular and there's no exit function there is no need for further action. -
From: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/smbfs/smbiod.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/fs/smbfs/smbiod.c b/fs/smbfs/smbiod.c
index 3e61b44..67176af 100644
--- a/fs/smbfs/smbiod.c
+++ b/fs/smbfs/smbiod.c
@@ -298,8 +298,6 @@ out:
*/
static int smbiod(void *unused)
{
- allow_signal(SIGKILL);
-
VERBOSE("SMB Kernel thread starting (%d) ...\n", current->pid);
for (;;) {
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:59:03 -0600 Why is it unnecessary? afaict we can presently terminate smbiod with a SIGKILL, and this change will alter (ie: break) that behaviour? -
From: Eric W. Biederman <ebiederm@xmission.com> To start the nfsv4-delegreturn thread this patch uses kthread_run instead of a combination of kernel_thread and daemonize. In addition allow_signal(SIGKILL) is removed from the expire delegations thread. Cc: Neil Brown <neilb@suse.de> Cc: Trond Myklebust <trond.myklebust@fys.uio.no> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/nfs/delegation.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-) diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index 841c99a..7b9b88c 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -232,7 +232,6 @@ int nfs_do_expire_all_delegations(void *ptr) struct nfs_delegation *delegation; struct inode *inode; - allow_signal(SIGKILL); restart: spin_lock(&clp->cl_lock); if (test_bit(NFS4CLNT_STATE_RECOVER, &clp->cl_state) != 0) @@ -310,8 +309,6 @@ static int recall_thread(void *data) struct nfs_inode *nfsi = NFS_I(inode); struct nfs_delegation *delegation; - daemonize("nfsv4-delegreturn"); - nfs_msync_inode(inode); down_read(&clp->cl_sem); down_write(&nfsi->rwsem); @@ -350,18 +347,18 @@ int nfs_async_inode_return_delegation(struct inode *inode, const nfs4_stateid *s .inode = inode, .stateid = stateid, }; - int status; + struct task_struct *task; init_completion(&data.started); __module_get(THIS_MODULE); - status = kernel_thread(recall_thread, &data, CLONE_KERNEL); - if (status < 0) + task = kthread_run(recall_thread, &data, "nfsv4-delegreturn"); + if (IS_ERR(task)) goto out_module_put; wait_for_completion(&data.started); return data.result; out_module_put: module_put(THIS_MODULE); - return status; + return PTR_ERR(task); } /* -- 1.5.0.g53756 -
Again vetoed, for the same reason. Trond -
From: Eric W. Biederman <ebiederm@xmission.com>
Start the g4fand using kthread_run not a combination
of kernel_thread and deamonize. This makes the code
a little simpler and more maintainable.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/macintosh/therm_windtunnel.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
index a1d3a98..5d888e7 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -36,6 +36,7 @@
#include <linux/i2c.h>
#include <linux/slab.h>
#include <linux/init.h>
+#include <linux/kthread.h>
#include <asm/prom.h>
#include <asm/machdep.h>
@@ -62,7 +63,6 @@ I2C_CLIENT_INSMOD;
static struct {
volatile int running;
struct completion completion;
- pid_t poll_task;
struct semaphore lock;
struct of_device *of_dev;
@@ -285,7 +285,6 @@ restore_regs( void )
static int
control_loop( void *dummy )
{
- daemonize("g4fand");
down( &x.lock );
setup_hardware();
@@ -323,7 +322,7 @@ do_attach( struct i2c_adapter *adapter )
if( x.thermostat && x.fan ) {
x.running = 1;
init_completion( &x.completion );
- x.poll_task = kernel_thread( control_loop, NULL, SIGCHLD | CLONE_KERNEL );
+ kthread_run( control_loop, NULL, "g4fand");
}
}
return ret;
--
1.5.0.g53756
-
On Thu, 19 Apr 2007 01:58:48 -0600 I had a bit of trouble reviewing this one because I was laughing so hard at the attempted coding-style in that driver. Oh well. I continue creeping into Christoph's camp - there's quite a bit of open-coded gunk which would go away if we were to teach this driver about kthread_should_stop() and kthread_stop(), and the conversion looks awfully easy to do. It's a shame to stop here. Oh well, I guess at least this is some forward progress. -
My main problem with touching that driver is that I don't have the hardware to test. I'll try to find a user to play the ginea pig. Ben. -
From: Eric W. Biederman <ebiederm@xmission.com>
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.
CC: Scott Murray <scottm@somanetworks.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/pci/hotplug/cpci_hotplug_core.c | 22 +++++++---------------
1 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index 6845515..c620c7e 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -33,6 +33,7 @@
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/smp_lock.h>
+#include <linux/kthread.h>
#include <asm/atomic.h>
#include <linux/delay.h>
#include "cpci_hotplug.h"
@@ -521,17 +522,13 @@ 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))
+ if (thread_finished)
break;
do {
rc = check_slots();
@@ -562,12 +559,8 @@ poll_thread(void *data)
{
int rc;
- lock_kernel();
- daemonize("cpci_hp_polld");
- unlock_kernel();
-
while (1) {
- if (thread_finished || signal_pending(current))
+ if (thread_finished)
break;
if (controller->ops->query_enum()) {
do {
@@ -592,7 +585,7 @@ poll_thread(void *data)
static int
cpci_start_thread(void)
{
- int pid;
+ struct task_struct *task;
/* initialize our semaphores */
init_MUTEX_LOCKED(&event_semaphore);
@@ -600,14 +593,13 @@ cpci_start_thread(void)
thread_finished = 0;
if (controller->irq)
- pid = ...From: Eric W. Biederman <ebiederm@xmission.com>
Start the reclaimer thread using kthread_run instead
of a combination of kernel_thread and daemonize.
The small amount of signal handling code is also removed
as it makes no sense and is a maintenance problem to handle
signals in kernel threads.
Cc: Neil Brown <neilb@suse.de>
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/lockd/clntlock.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index f4d45d4..83591f6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -9,6 +9,7 @@
#include <linux/module.h>
#include <linux/types.h>
#include <linux/time.h>
+#include <linux/kthread.h>
#include <linux/nfs_fs.h>
#include <linux/sunrpc/clnt.h>
#include <linux/sunrpc/svc.h>
@@ -153,7 +154,7 @@ nlmclnt_recovery(struct nlm_host *host)
if (!host->h_reclaiming++) {
nlm_get_host(host);
__module_get(THIS_MODULE);
- if (kernel_thread(reclaimer, host, CLONE_KERNEL) < 0)
+ if (IS_ERR(kthread_run(reclaimer, host, "%s-reclaim", host->h_name)))
module_put(THIS_MODULE);
}
}
@@ -166,9 +167,6 @@ reclaimer(void *ptr)
struct file_lock *fl, *next;
u32 nsmstate;
- daemonize("%s-reclaim", host->h_name);
- allow_signal(SIGKILL);
-
down_write(&host->h_rwsem);
/* This one ensures that our parent doesn't terminate while the
@@ -193,8 +191,6 @@ restart:
list_del_init(&fl->fl_u.nfs_fl.list);
/* Why are we leaking memory here? --okir */
- if (signalled())
- continue;
if (nlmclnt_reclaim(host, fl) != 0)
continue;
list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
--
1.5.0.g53756
-
Vetoed. Removing stuff just because it doesn't make sense to you is not acceptable. Signal handling in reclaimer threads is there in order to allow administrators to deal with the case where the server never comes up again. Trond -
Doesn't unmount handle that? Regardless kernel threads should be an implementation detail not a part of the user interface. If kernel threads are part of the user interface it makes them very hard to change. So it isn't that it doesn't make sense to me it is that it looks fundamentally broken and like a maintenance nightmare. I would rather kill kernel threads then try and simulate them when the kernel implementation has changed and kernel threads are not visible. If I could be convinced that signal handling in kernel threads is not something that will impede code modifications and refactoring I would have less of a problem, and might not care. 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? Eric -
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? Trond -
Do they actually always disappear, or do we keep them in the init_pid_namespace? -- Dave -
In the init pid namespace but not in any of it's children. Eric -
On Thu, 19 Apr 2007 17:19:24 -0400 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 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? -
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 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(). These mechanisms need to be replaced _before_ we start shooting down sigallow() etc in the kernel. Trond -
Possibly I just hadn't looked close enough. The signals looked like 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 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 -
From: Eric W. Biederman <ebiederm@xmission.com>
Currently md_thread calls allow_signal so it can receive a
SIGKILL but then does nothing with it except flush the
sigkill so that it not can use an interruptible sleep.
This whole dance is silly so remove the unnecessary
and broken signal handling logic.
Cc: Neil Brown <neilb@suse.de>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
drivers/md/md.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1299c23..dfd0cb9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4542,17 +4542,11 @@ static int md_thread(void * arg)
*/
current->flags |= PF_NOFREEZE;
- allow_signal(SIGKILL);
while (!kthread_should_stop()) {
/* We need to wait INTERRUPTIBLE so that
* we don't add to the load-average.
- * That means we need to be sure no signals are
- * pending
*/
- if (signal_pending(current))
- flush_signals(current);
-
wait_event_interruptible_timeout
(thread->wqueue,
test_bit(THREAD_WAKEUP, &thread->flags)
--
1.5.0.g53756
-
From: Eric W. Biederman <ebiederm@xmission.com>
Modify startup of ipvs sync threads to use kthread_run
instead of a weird combination of calling kernel_thread
to start a fork_sync_thread whose hole purpose in life was
to call kernel_thread again starting the actually sync thread
which called daemonize.
To use kthread_run I had to move the name calcuation from
sync_thread into start_sync_thread resulting in a small
amount of code motion.
The result is simpler and more maintainable piece of code.
Cc: Wensong Zhang <wensong@linux-vs.org>
Cc: Julian Anastasov <ja@ssi.bg>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/ipv4/ipvs/ip_vs_sync.c | 49 ++++++++++---------------------------------
1 files changed, 12 insertions(+), 37 deletions(-)
diff --git a/net/ipv4/ipvs/ip_vs_sync.c b/net/ipv4/ipvs/ip_vs_sync.c
index 7ea2d98..c4be9dc 100644
--- a/net/ipv4/ipvs/ip_vs_sync.c
+++ b/net/ipv4/ipvs/ip_vs_sync.c
@@ -29,6 +29,7 @@
#include <linux/in.h>
#include <linux/igmp.h> /* for ip_mc_join_group */
#include <linux/udp.h>
+#include <linux/kthread.h>
#include <net/ip.h>
#include <net/sock.h>
@@ -750,34 +751,23 @@ static int sync_thread(void *startup)
DECLARE_WAITQUEUE(wait, current);
mm_segment_t oldmm;
int state;
- const char *name;
/* increase the module use count */
ip_vs_use_count_inc();
- if (ip_vs_sync_state & IP_VS_STATE_MASTER && !sync_master_pid) {
+ if (ip_vs_sync_state & IP_VS_STATE_MASTER && !sync_master_pid)
state = IP_VS_STATE_MASTER;
- name = "ipvs_syncmaster";
- } else if (ip_vs_sync_state & IP_VS_STATE_BACKUP && !sync_backup_pid) {
+ else if (ip_vs_sync_state & IP_VS_STATE_BACKUP && !sync_backup_pid)
state = IP_VS_STATE_BACKUP;
- name = "ipvs_syncbackup";
- } else {
+ else {
IP_VS_BUG();
ip_vs_use_count_dec();
return -EINVAL;
}
- daemonize(name);
-
oldmm = get_fs();
set_fs(KERNEL_DS);
- /* Block all signals ...Thanks Eric, I'll review this and get back to you shortly. -- Horms H: http://www.vergenet.net/~horms/ W: http://www.valinux.co.jp/en/ -
On Thu, 19 Apr 2007 18:04:36 +0900 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. -
From: Eric W. Biederman <ebiederm@xmission.com> Cc: Neil Brown <neilb@suse.de> Cc: Trond Myklebust <trond.myklebust@fys.uio.no> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/nfs/nfs4state.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 5fffbdf..d16393f 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -775,8 +775,6 @@ static int reclaimer(void *ptr) struct rpc_cred *cred; int status = 0; - allow_signal(SIGKILL); - /* Ensure exclusive access to NFSv4 state */ lock_kernel(); down_write(&clp->cl_sem); -- 1.5.0.g53756 -
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; } @@ ...
