On Fri, 4 May 2007, Christoph Hellwig wrote:

> On Fri, Apr 27, 2007 at 06:07:49PM -0400, Scott Murray wrote:
> > 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.
> 
> 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 || signal_pending(current))
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+		if (kthread_should_stop())
 			break;
 		do {
 			rc = check_slots();
@@ -541,18 +534,17 @@ event_thread(void *data)
 			} else if (rc < 0) {
 				dbg("%s - error checking slots", __FUNCTION__);
 				thread_finished = 1;
-				break;
+				goto out;
 			}
-		} while (atomic_read(&extracting) && !thread_finished);
-		if (thread_finished)
+		} while (atomic_read(&extracting) && !kthread_should_stop());
+		if (kthread_should_stop())
 			break;
 
 		/* Re-enable ENUM# interrupt */
 		dbg("%s - re-enabling irq", __FUNCTION__);
 		controller->ops->enable_irq();
 	}
-	dbg("%s - event thread signals exit", __FUNCTION__);
-	up(&thread_exit);
+ out:
 	return 0;
 }
 
@@ -562,12 +554,8 @@ poll_thread(void *data)
 {
 	int rc;
 
-	lock_kernel();
-	daemonize("cpci_hp_polld");
-	unlock_kernel();
-
 	while (1) {
-		if (thread_finished || signal_pending(current))
+		if (kthread_should_stop() || signal_pending(current))
 			break;
 		if (controller->ops->query_enum()) {
 			do {
@@ -578,48 +566,36 @@ poll_thread(void *data)
 				} else if (rc < 0) {
 					dbg("%s - error checking slots", __FUNCTION__);
 					thread_finished = 1;
-					break;
+					goto out;
 				}
-			} while (atomic_read(&extracting) && !thread_finished);
+			} while (atomic_read(&extracting) && !kthread_should_stop());
 		}
 		msleep(100);
 	}
-	dbg("poll thread signals exit");
-	up(&thread_exit);
+ out:
 	return 0;
 }
 
 static int
 cpci_start_thread(void)
 {
-	int pid;
-
-	/* initialize our semaphores */
-	init_MUTEX_LOCKED(&event_semaphore);
-	init_MUTEX_LOCKED(&thread_exit);
-	thread_finished = 0;
-
 	if (controller->irq)
-		pid = kernel_thread(event_thread, NULL, 0);
+		cpci_thread = kthread_run(event_thread, NULL, "cpci_hp_eventd");
 	else
-		pid = kernel_thread(poll_thread, NULL, 0);
-	if (pid < 0) {
+		cpci_thread = kthread_run(poll_thread, NULL, "cpci_hp_polld");
+	if (IS_ERR(cpci_thread)) {
 		err("Can't start up our thread");
-		return -1;
+		return PTR_ERR(cpci_thread);
 	}
-	dbg("Our thread pid = %d", pid);
+	thread_finished = 0;
 	return 0;
 }
 
 static void
 cpci_stop_thread(void)
 {
+	kthread_stop(cpci_thread);
 	thread_finished = 1;
-	dbg("thread finish command given");
-	if (controller->irq)
-		up(&event_semaphore);
-	dbg("wait for thread to exit");
-	down(&thread_exit);
 }
 
 int
 

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

On Mon, 7 May 2007 14:18:29 -0400 (EDT)
Scott Murray <scottm@somanetworks.com> wrote:

> On Fri, 4 May 2007, Christoph Hellwig wrote:
> 
> > On Fri, Apr 27, 2007 at 06:07:49PM -0400, Scott Murray wrote:
> > > 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.
> > 
> > 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.

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 ;)
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, 9 May 2007 16:24:30 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 7 May 2007 14:18:29 -0400 (EDT)
> Scott Murray <scottm@somanetworks.com> wrote:
> 
> > On Fri, 4 May 2007, Christoph Hellwig wrote:
> > 
> > > On Fri, Apr 27, 2007 at 06:07:49PM -0400, Scott Murray wrote:
> > > > 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.
> > > 
> > > 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.
> 
> 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 ;)
> 

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
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Thu, 10 May 2007 14:29:29 -0400 (EDT) Scott Murray <scottm@somanetworks.com> wrote:

> > or
> > 
> > 	you->kristen->Len->mainline
> >                        |
> >                        v
> >                       -mm
> > 
> > or something else.
> 
> 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.

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.
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, 9 May 2007, Andrew Morton wrote:

> On Mon, 7 May 2007 14:18:29 -0400 (EDT)
> Scott Murray <scottm@somanetworks.com> wrote:
> 
> > On Fri, 4 May 2007, Christoph Hellwig wrote:
> > 
> > > On Fri, Apr 27, 2007 at 06:07:49PM -0400, Scott Murray wrote:
> > > > 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.
> > > 
> > > 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.
> 
> 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.

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
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, May 02, 2007 at 09:44:11AM -0600, Eric W. Biederman wrote:
> Dean Nelson <dcn@sgi.com> writes:
> > On Thu, Apr 26, 2007 at 01:11:15PM -0600, Eric W. Biederman wrote:
> >>
> >> Ok.    Because of the module unloading issue, and because we don't have
> >> a lot of these threads running around, the current plan is to fix
> >> thread_create and kthread_stop so that they must always be paired,
> >> and so that kthread_stop will work correctly if the task has already
> >> exited.
> >>
> >> Basically that just involves calling get_task_struct in kthread_create
> >> and put_task_struct in kthread_stop.
> >
> > 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?
> 
> 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.

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
called for a kthread that has already exited will get into the -mm tree?

> > 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()?
> 
> 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
> remove problem, but it's not to hard.

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
a patch to change XPC to use the kthread API.

> > 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 a bit more justification for going down that road.

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
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers