kthread: saa7134-tvaudio.c

Replace kernel_thread() with kthread_run() since kernel_thread() is deprecated in drivers/modules.

Note that this driver, like a few others, allows SIGTERM. Not sure if that is affected by conversion to kthread. Appreciate any comments on that.

 2 files changed, 17 insertions(+), 20 deletions(-)

Index: lx26-18-rc5/drivers/media/video/saa7134/saa7134.h
===================================================================

drivers/media/video/saa7134/saa7134.h +1 -3

@@ -311,10 +311,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: lx26-18-rc5/drivers/media/video/saa7134/saa7134-tvaudio.c
===================================================================

drivers/media/video/saa7134/saa7134-tvaudio.c +16 -17

@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/smp_lock.h>
+#include <linux/kthread.h>
 #include <asm/div64.h>
 
 #include "saa7134-reg.h"

@@ -357,7 +358,7 @@ static int tvaudio_sleep(struct saa7134_
 	DECLARE_WAITQUEUE(wait, current);
 
 	add_wait_queue(&dev->thread.wq, &wait);
-	if (dev->thread.scan1 == dev->thread.scan2 && !dev->thread.shutdown) {
+	if (dev->thread.scan1 == dev->thread.scan2 && !kthread_should_stop()) {
 		if (timeout < 0) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			schedule();

@@ -525,7 +526,7 @@ static int tvaudio_thread(void *data)
 	allow_signal(SIGTERM);
 	for (;;) {
 		tvaudio_sleep(dev,-1);
-		if (dev->thread.shutdown || signal_pending(current))
+		if (kthread_should_stop() || signal_pending(current))
 			goto done;
 
 	restart:

@@ -633,7 +634,7 @@ static int tvaudio_thread(void *data)
 		for (;;) {
 			if (tvaudio_sleep(dev,5000))
 				goto restart;
-			if (dev->thread.shutdown || signal_pending(current))
+			if (kthread_should_stop() || signal_pending(current))
 				break;
 			if (UNSET == dev->thread.mode) {
 				rx = tvaudio_getstereo(dev,&tvaudio[i]);

@@ -649,7 +650,6 @@ static int tvaudio_thread(void *data)
 	}
 
  done:
-	complete_and_exit(&dev->thread.exit, 0);
 	return 0;
 }
 

@@ -798,7 +798,6 @@ static int tvaudio_thread_ddep(void *dat
 	struct saa7134_dev *dev = data;
 	u32 value, norms, clock;
 
-	daemonize("%s", dev->name);
 	allow_signal(SIGTERM);
 
 	clock = saa7134_boards[dev->board].audio_clock;

@@ -812,7 +811,7 @@ static int tvaudio_thread_ddep(void *dat
 
 	for (;;) {
 		tvaudio_sleep(dev,-1);
-		if (dev->thread.shutdown || signal_pending(current))
+		if (kthread_should_stop() || signal_pending(current))
 			goto done;
 
 	restart:

@@ -894,7 +893,6 @@ static int tvaudio_thread_ddep(void *dat
 	}
 
  done:
-	complete_and_exit(&dev->thread.exit, 0);
 	return 0;
 }
 

@@ -1004,15 +1002,16 @@ int saa7134_tvaudio_init2(struct saa7134
 		break;
 	}
 
-	dev->thread.pid = -1;
+	dev->thread.task = NULL;
 	if (my_thread) {
 		/* start tvaudio thread */
 		init_waitqueue_head(&dev->thread.wq);
-		init_completion(&dev->thread.exit);
-		dev->thread.pid = kernel_thread(my_thread,dev,0);
-		if (dev->thread.pid < 0)
+		dev->thread.task = kthread_run(my_thread,dev,dev->name);
+		if (IS_ERR(dev->thread.task)) {
 			printk(KERN_WARNING "%s: kernel_thread() failed\n",
-			       dev->name);
+			                              dev->name);
+			dev->thread.task = NULL;
+		}
 		saa7134_tvaudio_do_scan(dev);
 	}
 

@@ -1023,10 +1022,10 @@ int saa7134_tvaudio_init2(struct saa7134
 int saa7134_tvaudio_fini(struct saa7134_dev *dev)
 {
 	/* shutdown tvaudio thread */
-	if (dev->thread.pid >= 0) {
-		dev->thread.shutdown = 1;
-		wake_up_interruptible(&dev->thread.wq);
-		wait_for_completion(&dev->thread.exit);
+	if (dev->thread.task) {
+		/* kthread_stop() wakes up the thread */
+		kthread_stop(dev->thread.task);
+		dev->thread.task = NULL;
 	}
 	saa_andorb(SAA7134_ANALOG_IO_SELECT, 0x07, 0x00); /* LINE1 */
 	return 0;

@@ -1034,7 +1033,7 @@ int saa7134_tvaudio_fini(struct saa7134_
 
 int saa7134_tvaudio_do_scan(struct saa7134_dev *dev)
 {
-	if (dev->thread.pid >= 0) {
+	if (dev->thread.task) {
 		dev->thread.mode = UNSET;
 		dev->thread.scan2++;
 		wake_up_interruptible(&dev->thread.wq);

So in general, yes, the driver should be converted to the kthread API - this is a requirement for virtualisation, but I forget why, and that's the "standard" way of doing it.

Quoted 3 lines · Andrew Morton

So in general, yes, the driver should be converted to the kthread API - this is a requirement for virtualisation, but I forget why, and that's the "standard" way of doing it.

With the kthread api new kernel threads are started as children of keventd in well defined circumstances. If you don't do this kernel threads can wind up sharing weird parts of a parent process's resources and locking resources in the kernel long past the time when they are actually used by anything a user space process can kill.

We have actually witnessed this problem with the kernels filesystem mount namespace. Mostly daemonize in the kernel unshares everything that could be a problem but the problem is sufficiently subtle it makes more sense to the change kernel threads. So these weird and subtle dependencies go away.

So in essence the container work needs the new kthread api for the same reasons everyone else does it is just more pronounced in that case.

Quoted 13 lines

With the kthread api new kernel threads are started as children of keventd in well defined circumstances. If you don't do this kernel threads can wind up sharing weird parts of a parent process's resources and locking resources in the kernel long past the time when they are actually used by anything a user space process can kill.

We have actually witnessed this problem with the kernels filesystem mount namespace. Mostly daemonize in the kernel unshares everything that could be a problem but the problem is sufficiently subtle it makes more sense to the change kernel threads. So these weird and subtle dependencies go away.

Quoted 3 lines

So in essence the container work needs the new kthread api for the same reasons everyone else does it is just more pronounced in that case.

That plus the obvious bit. For the pid namespace we have to declare war on people storing a pid_t values. Either converting them to struct pid * or removing them entirely. Doing the kernel_thread to kthread conversion removes them entirely.

Quoted 16 lines

Quoted 15 lines

With the kthread api new kernel threads are started as children of keventd in well defined circumstances. If you don't do this kernel threads can wind up sharing weird parts of a parent process's resources and locking resources in the kernel long past the time when they are actually used by anything a user space process can kill.

We have actually witnessed this problem with the kernels filesystem mount namespace. Mostly daemonize in the kernel unshares everything that could be a problem but the problem is sufficiently subtle it makes more sense to the change kernel threads. So these weird and subtle dependencies go away.

So in essence the container work needs the new kthread api for the same reasons everyone else does it is just more pronounced in that case.

Quoted 4 lines

That plus the obvious bit. For the pid namespace we have to declare war on people storing a pid_t values. Either converting them to struct pid * or removing them entirely. Doing the kernel_thread to kthread conversion removes them entirely.

we've started that war, won a few battles but some drivers need more work that a simple replace. If we could give some priorities, it would help to focus on the most important ones. check out the list bellow.

From what I have seen you can shorten the list by several entries by removing

fs/lockd/clntlock.c
fs/nfs/delegation.c
net/sunrpc/svc.c

arch/arm/kernel/ecard.c
arch/i386/kernel/apm.c
arch/i386/kernel/io_apic.c
arch/i386/mach-voyager/voyager_thread.c
arch/ia64/sn/kernel/xpc_main.c
arch/mips/au1000/db1x00/mirage_ts.c
arch/mips/kernel/apm.c
arch/parisc/kernel/process.c
arch/powerpc/platforms/pseries/eeh_event.c
arch/powerpc/platforms/pseries/rtasd.c
arch/s390/mm/cmm.c
arch/sparc64/kernel/power.c

drivers/base/firmware_class.c
drivers/block/loop.c
drivers/ieee1394/nodemgr.c
drivers/macintosh/adb.c
drivers/macintosh/mediabay.c
drivers/macintosh/therm_pm72.c
drivers/macintosh/therm_windtunnel.c
drivers/media/dvb/dvb-core/dvb_ca_en50221.c
drivers/media/dvb/dvb-core/dvb_frontend.c
drivers/media/dvb/ttpci/av7110.c
drivers/media/video/saa7134/saa7134-tvaudio.c
drivers/media/video/tvaudio.c
drivers/mmc/mmc_queue.c
drivers/mtd/mtd_blkdevs.c
drivers/net/wireless/airo.c
drivers/pci/hotplug/cpci_hotplug_core.c
drivers/pci/hotplug/cpqphp_ctrl.c
drivers/pci/hotplug/ibmphp_hpc.c
drivers/pci/hotplug/pciehp_ctrl.c
drivers/pnp/pnpbios/core.c
drivers/s390/net/lcs.c
drivers/s390/net/qeth_main.c
drivers/s390/scsi/zfcp_erp.c
drivers/usb/atm/usbatm.c
drivers/usb/storage/libusual.c

fs/afs/cmservice.c
fs/afs/kafsasyncd.c
fs/afs/kafstimod.c
fs/cifs/connect.c
fs/jffs2/background.c
fs/jffs/inode-v23.c
fs/lockd/clntlock.c
fs/nfs/delegation.c

init/do_mounts_initrd.c
kernel/kmod.c
kernel/stop_machine.c

net/bluetooth/bnep/core.c
net/bluetooth/cmtp/core.c
net/bluetooth/hidp/core.c
net/bluetooth/rfcomm/core.c
net/core/pktgen.c
net/ipv4/ipvs/ip_vs_sync.c
net/rxrpc/krxiod.c
net/rxrpc/krxsecd.c
net/rxrpc/krxtimod.c
net/sunrpc/svc.c
_______________________________________________
Containers mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/containers

Quoted 7 lines

Quoted 4 lines

That plus the obvious bit. For the pid namespace we have to declare war on people storing a pid_t values. Either converting them to struct pid * or removing them entirely. Doing the kernel_thread to kthread conversion removes them entirely.

we've started that war, won a few battles but some drivers need more work that a simple replace. If we could give some priorities, it would help to focus on the most important ones. check out the list bellow.

Sure, I think I can help.

There are a couple of test I can think of that should help.
1) Is the pid value stored. If not a pid namespace won't affect it's normal operation.

2) Is this thread started during kernel boot before this thread could have a user space parent. If it can't have a user space parent then it can't take a reference to user space resources.

3) Can the code be compiled modular and will it break when we stop exporting kernel_thread.

4) How frequently is this thing used. The more common code is probably in better shape and more likely to get a good maintainer response, and we care more :)

irqbalanced from arch/i386/kernel/io_apic.c should be safe to leave alone because it doesn't store a pid_t, it is started during boot, and it can't be compiled modular.

Quoted 1 line

From what I have seen you can shorten the list by several entries by removing

code like irqbalanced that can't possibly cause us any problems. kvoyagerd from arch/i386/mach-voyager/voyager_thread.c is another one.

The first on my personal hit list is nfs.

Quoted 3 lines

fs/lockd/clntlock.c
fs/nfs/delegation.c
net/sunrpc/svc.c

Because it does store pid_t values, it isn't started during kernel boot, it can be compiled modular, and people use it all of the time.

I do agree from what I have seen, that changing idioms to the kthread way of doing things isn't simply a matter of substitute and replace which is unfortunate. Although the biggest hurdle seems to be to teach kernel threads to communicate with something besides signals. Which is a general help anyway.

Unfortunately I'm distracted at the moment so I haven't gone through the entire list but I hope this helps.

Eric W. Biederman wrote:

[ ... ]

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

That plus the obvious bit. For the pid namespace we have to declare war on people storing a pid_t values. Either converting them to struct pid * or removing them entirely. Doing the kernel_thread to kthread conversion removes them entirely.

we've started that war, won a few battles but some drivers need more work that a simple replace. If we could give some priorities, it would help to focus on the most important ones. check out the list bellow.

Sure, I think I can help.

Quoted 3 lines

There are a couple of test I can think of that should help.
1) Is the pid value stored. If not a pid namespace won't affect it's normal operation.

I've extracted this list from a table which includes a pid cache column. this pid cache column is not complete yet. I'd be nice if we could use a wiki to maintain this table, the existing openvz or vserver wiki ?

Quoted 7 lines

I do agree from what I have seen, that changing idioms to the kthread way of doing things isn't simply a matter of substitute and replace which is unfortunate. Although the biggest hurdle seems to be to teach kernel threads to communicate with something besides signals. Which is a general help anyway.

Unfortunately I'm distracted at the moment so I haven't gone through the entire list but I hope this helps.

I've extracted this list from a table which includes a pid cache column. this pid cache column is not complete yet. I'd be nice if we could use a wiki to maintain this table, the existing openvz or vserver wiki ?

Quoted 7 lines · On Tue, 29 Aug 2006 14:15:55 -0700 Sukadev Bhattiprolu <[email protected]>…

Replace kernel_thread() with kthread_run() since kernel_thread() is deprecated in drivers/modules.

Note that this driver, like a few others, allows SIGTERM. Not sure if that is affected by conversion to kthread. Appreciate any comments on that.

hm, I think this driver needs more help.

- It shouldn't be using signals at all, really. Signals are for userspace IPC. The kernel internally has better/richer/faster/tighter ways of inter-thread communication.

- saa7134_tvaudio_fini()-versus-tvaudio_sleep() looks racy:

	if (dev->thread.scan1 == dev->thread.scan2 && !dev->thread.shutdown) {
		if (timeout < 0) {
			set_current_state(TASK_INTERRUPTIBLE);
			schedule();

If the wakeup happens after the test of dev->thread.shutdown, that sleep will be permanent.

So in general, yes, the driver should be converted to the kthread API - this is a requirement for virtualisation, but I forget why, and that's the "standard" way of doing it.

- The signal stuff should go away if at all possible.

Quoted 32 lines · Andrew Morton

Quoted 7 lines · On Tue, 29 Aug 2006 14:15:55 -0700 Sukadev Bhattiprolu <[email protected]>…

Replace kernel_thread() with kthread_run() since kernel_thread() is deprecated in drivers/modules.

Note that this driver, like a few others, allows SIGTERM. Not sure if that is affected by conversion to kthread. Appreciate any comments on that.

hm, I think this driver needs more help.

- It shouldn't be using signals at all, really. Signals are for userspace IPC. The kernel internally has better/richer/faster/tighter ways of inter-thread communication.

- saa7134_tvaudio_fini()-versus-tvaudio_sleep() looks racy:

	if (dev->thread.scan1 == dev->thread.scan2 && !dev->thread.shutdown) {
		if (timeout < 0) {
			set_current_state(TASK_INTERRUPTIBLE);
			schedule();

If the wakeup happens after the test of dev->thread.shutdown, that sleep will be permanent.

So in general, yes, the driver should be converted to the kthread API - this is a requirement for virtualisation, but I forget why, and that's the "standard" way of doing it.

Quoted 1 line

- The signal stuff should go away if at all possible.

The thread of this driver allows SIGTERM for some obscure reason. Not sure why, I didn't find anything relying on it.

could we just remove the allow_signal() ?

On Wed, 30 Aug 2006 18:30:27 +0200
Cedric Le Goater <[email protected]> wrote:

Quoted 5 lines

The thread of this driver allows SIGTERM for some obscure reason. Not sure why, I didn't find anything relying on it.

could we just remove the allow_signal() ?

I hope so. However I have a bad feeling that the driver wants to accept signals from userspace. Hopefully Mauro & co will be able to clue us in.