Re: [PATCH 1/3] kthread: update s390 cmm driver to use kthread

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Christoph Hellwig (hch@infradead.org):
> On Thu, Aug 24, 2006 at 04:22:42PM -0500, Serge E. Hallyn wrote:
> > Update the s390 cooperative memory manager, which can be a module,
> > to use kthread rather than kernel_thread, whose EXPORT is deprecated.
> > 
> > This patch compiles and boots fine, but I don't know how to really
> > test the driver.
> 
> NACK.  Please do a real conversion to the kthread paradigm instead of
> doctoring around the trivial bits that could be changed with a script.
> 
> Please use kthread_should_stop() and remove the cmm_thread_wait
> waitqueue in favour of wake_up_process.  The timer useage could
> probably be replaced with smart usage of schedule_timeout().
> Also the code seems to miss a proper thread termination on module
> removal.

Ok, the patch in -mm does kthread_stop() on module_exit, but still uses
the timer and cmm_thread_wait.  

I'm not clear what the timer is actually trying to do, or why there is a
separate cmm_pages_target and cmm_timed_pages_target.  So I'm sure the
below patch on top of -mm2 is wrong (it compiles, but I just noticed
2.6.18-rc4-mm2 doesn't boot without this patch either) but hopefully
Heiko or Martin can tell me what would be the right way, or implement
it?

thanks,
-serge

Subject: [PATCH] s390: stop using cmm_thread_wait and cmm_timer in cmm

Update cmm to stop using cmm_thread_wait or cmm_timer.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 arch/s390/mm/cmm.c |   67 +++++++++-------------------------------------------
 1 files changed, 12 insertions(+), 55 deletions(-)

4182dbd937e5084db4cfd63193ff93267ea8042e
diff --git a/arch/s390/mm/cmm.c b/arch/s390/mm/cmm.c
index 9b62157..ecd237a 100644
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -48,11 +48,6 @@ static struct cmm_page_array *cmm_timed_
 static DEFINE_SPINLOCK(cmm_lock);
 
 static struct task_struct *cmm_thread_ptr;
-static wait_queue_head_t cmm_thread_wait;
-static struct timer_list cmm_timer;
-
-static void cmm_timer_fn(unsigned long);
-static void cmm_set_timer(void);
 
 static long
 cmm_strtoul(const char *cp, char **endp)
@@ -157,18 +152,10 @@ static struct notifier_block cmm_oom_nb 
 static int
 cmm_thread(void *dummy)
 {
-	int rc;
+	while (!kthread_should_stop()) {
 
-	while (1) {
-		rc = wait_event_interruptible(cmm_thread_wait,
-			(cmm_pages != cmm_pages_target ||
-			 cmm_timed_pages != cmm_timed_pages_target ||
-			 kthread_should_stop()));
-		if (kthread_should_stop() || rc == -ERESTARTSYS) {
-			cmm_pages_target = cmm_pages;
-			cmm_timed_pages_target = cmm_timed_pages;
+		if (kthread_should_stop())
 			break;
-		}
 		if (cmm_pages_target > cmm_pages) {
 			if (cmm_alloc_pages(1, &cmm_pages, &cmm_page_list))
 				cmm_pages_target = cmm_pages;
@@ -183,48 +170,19 @@ cmm_thread(void *dummy)
 			cmm_free_pages(1, &cmm_timed_pages,
 			       	       &cmm_timed_page_list);
 		}
-		if (cmm_timed_pages > 0 && !timer_pending(&cmm_timer))
-			cmm_set_timer();
+
+		__set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(cmm_timeout_seconds*HZ);
 	}
+
 	return 0;
 }
 
 static void
 cmm_kick_thread(void)
 {
-	wake_up(&cmm_thread_wait);
-}
-
-static void
-cmm_set_timer(void)
-{
-	if (cmm_timed_pages_target <= 0 || cmm_timeout_seconds <= 0) {
-		if (timer_pending(&cmm_timer))
-			del_timer(&cmm_timer);
-		return;
-	}
-	if (timer_pending(&cmm_timer)) {
-		if (mod_timer(&cmm_timer, jiffies + cmm_timeout_seconds*HZ))
-			return;
-	}
-	cmm_timer.function = cmm_timer_fn;
-	cmm_timer.data = 0;
-	cmm_timer.expires = jiffies + cmm_timeout_seconds*HZ;
-	add_timer(&cmm_timer);
-}
-
-static void
-cmm_timer_fn(unsigned long ignored)
-{
-	long nr;
-
-	nr = cmm_timed_pages_target - cmm_timeout_pages;
-	if (nr < 0)
-		cmm_timed_pages_target = 0;
-	else
-		cmm_timed_pages_target = nr;
-	cmm_kick_thread();
-	cmm_set_timer();
+	if (cmm_thread_ptr)
+		wake_up_process(cmm_thread_ptr);
 }
 
 void
@@ -258,7 +216,6 @@ cmm_set_timeout(long nr, long seconds)
 {
 	cmm_timeout_pages = nr;
 	cmm_timeout_seconds = seconds;
-	cmm_set_timer();
 }
 
 static inline int
@@ -450,12 +407,12 @@ cmm_init (void)
 	rc = register_oom_notifier(&cmm_oom_nb);
 	if (rc < 0)
 		goto out_oom_notify;
-	init_waitqueue_head(&cmm_thread_wait);
-	init_timer(&cmm_timer);
 	cmm_thread_ptr = kthread_run(cmm_thread, NULL, "cmmthread");
-	rc = IS_ERR(cmm_thread_ptr) ? PTR_ERR(cmm_thread_ptr) : 0;
-	if (!rc)
+	if (IS_ERR(cmm_thread_ptr)) {
+		rc = PTR_ERR(cmm_thread_ptr);
+		cmm_thread_ptr = NULL;
 		goto out;
+	}
 	/*
 	 * kthread_create failed. undo all the stuff from above again.
 	 */
-- 
1.1.6
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/