Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Christoph Hellwig wrote on 2006-06-17:
> Below is a draft patch to convert it to the kthread API and replace the
> reset_sem with a simple wake_up_process scheme.  I removed the down_trylock
> loop there which I think should be fine because we get the wakeup again
> ASAP, but please double-check.
[...]
[in nodemgr_host_thread(), top of event loop]
> @@ -1579,15 +1573,14 @@
>  		unsigned int generation = 0;
>  		int i;
>  
> -		if (down_interruptible(&hi->reset_sem) ||
> -		    down_interruptible(&nodemgr_serialize)) {
> +		if (down_interruptible(&nodemgr_serialize)) {

This won't work. "down_interruptible(&hi->reset_sem)" was there to put 
the thread to sleep after it did its work, until the next bus reset. Now 
the event loop would be entered continuously without an actual bus reset 
event.

(nodemgr_serialize is just a mutex disguised as a semaphore which 
prevents multiple nodemgr host threads to enter their event loop 
concurrently.)

>  			if (try_to_freeze())
>  				continue;
>  			printk("NodeMgr: received unexpected signal?!\n" );
>  			break;
>  		}
>  
> -		if (hi->kill_me) {
> +		if (kthread_should_stop()) {
>  			up(&nodemgr_serialize);
>  			break;
>  		}
> @@ -1608,13 +1601,8 @@
>  			 * returning bogus data. */
>  			generation = get_hpsb_generation(host);
>  
> -			/* If we get a reset before we are done waiting, then
> -			 * start the the waiting over again */
> -			while (!down_trylock(&hi->reset_sem))
> -				i = 0;
[...]

Another minor issue: This check cannot be removed without replacement. 
However we could implement this check easily without a counting 
semaphore. (We could check the bus generation before and after the sleep.)

I will try to rework this patch and split it into more patches: One 
which converts nodemgr to the kthread API but keeps the reset_sem, one 
patch which gets rid of the counting semaphore reset_sem, one patch 
which converts nodemgr_serialize to a mutex.

BTW, it may be possible to remove nodemgr_serialize too. There are other 
exclusion mechanisms in nodemgr.c which should already prevent most if 
not all undesirable concurrency, notably the semaphores 
dev->bus->subsys.rwsem and class->subsys.rwsem.
-- 
Stefan Richter
-=====-=-==- -==- =---=
http://arcgraph.de/sr/