Re: [PATCH 1/15] EDAC: switch to kthread_ API

Re: [PATCH 1/15] EDAC: switch to kthread_ API

[Doug Thompson]

Originally we used a timeout, but when I added the PCI Parity scanning,
that broke while trying to get a PCI spinlock (via a PCI API call)
during the timer interrupt time. I then added the current kernel thread
model and a first attempt. We subsequently received input for the
kthread_* model which is were this patch came from.

Currently the timer event code performs two operations: 

  1) ECC polling and 
  2) PCI parity polling. 

I want to split those from each other, so each can have a seperate cycle
rate (also adding a sysfs cycle control for the PCI parity timing in
addition to the existing ECC cycle control).

One of the thoughts I have had in this refactoring is to utilize worker
queue processing to do the work (and bypass the spinlock issue) which is
triggered by the timer event for PCI parity polling and thus also the
ECC polling for uniformity.

Thoughts are welcome

doug thompson



On Fri, 2006-03-03 at 09:16 +0000, Arjan van de Ven  wrote:
> On Thu, 2006-03-02 at 18:30 -0800, Andrew Morton wrote:
> > Dave Peterson <dsp@llnl.gov> wrote:
> > >
> > >   		schedule_timeout((HZ * poll_msec) / 1000);
> > >   		try_to_freeze();
> > >  +		__set_current_state(TASK_RUNNING);
> > 
> > schedule() and schedule_timeout*() always return in state TASK_RUNNING, so
> > I'll take that out of there.
> > 
> > We might as well use schedule_timeout_interruptible(), too.  As a bonus, we
> > get to delete that spelling mistake ;)
> 
> 
> or even msleep variant ;)
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by xPML, a groundbreaking scripting language
> that extends applications into web and mobile media. Attend the live webcast
> and join the prime developer group breaking into this new coding territory!
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
> _______________________________________________
> bluesmoke-devel mailing list
> bluesmoke-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bluesmoke-devel

Re: [PATCH 1/15] EDAC: switch to kthread_ API

[Dave Peterson]

On Friday 03 March 2006 07:57, Doug Thompson wrote:
> Currently the timer event code performs two operations:
>
>   1) ECC polling and
>   2) PCI parity polling.
>
> I want to split those from each other, so each can have a seperate cycle
> rate (also adding a sysfs cycle control for the PCI parity timing in
> addition to the existing ECC cycle control).

Yes, this sounds like a good idea.  Using schedule_delayed_work() to
independently implement each polling cycle, we should be able to get
rid of the EDAC kernel thread.

Re: [PATCH 1/15] EDAC: switch to kthread_ API

[Andrew Morton]

Dave Peterson <dsp@llnl.gov> wrote:
>
> On Friday 03 March 2006 07:57, Doug Thompson wrote:
> > Currently the timer event code performs two operations:
> >
> >   1) ECC polling and
> >   2) PCI parity polling.
> >
> > I want to split those from each other, so each can have a seperate cycle
> > rate (also adding a sysfs cycle control for the PCI parity timing in
> > addition to the existing ECC cycle control).
> 
> Yes, this sounds like a good idea.  Using schedule_delayed_work() to
> independently implement each polling cycle, we should be able to get
> rid of the EDAC kernel thread.

That would suit.  One needs to be quite careful about killing everything
off in the close->rmmod side of things, especially if the delayed work
handler re-arms itself.  We have a pending (and possibly executing) timer
handler to worry about, then a pending (and possibly executing) keventd
handler to worry about.

We've got this cancel_rearming_delayed_work() thing - I was never terribly
happy about it, but I think it works.

Re: [PATCH 1/15] EDAC: switch to kthread_ API

[Arjan van de Ven]

On Thu, 2006-03-02 at 18:30 -0800, Andrew Morton wrote:
> Dave Peterson <dsp@llnl.gov> wrote:
> >
> >   		schedule_timeout((HZ * poll_msec) / 1000);
> >   		try_to_freeze();
> >  +		__set_current_state(TASK_RUNNING);
> 
> schedule() and schedule_timeout*() always return in state TASK_RUNNING, so
> I'll take that out of there.
> 
> We might as well use schedule_timeout_interruptible(), too.  As a bonus, we
> get to delete that spelling mistake ;)


or even msleep variant ;)

Re: [PATCH 1/15] EDAC: switch to kthread_ API

[Andrew Morton]

Dave Peterson <dsp@llnl.gov> wrote:
>
>   		schedule_timeout((HZ * poll_msec) / 1000);
>   		try_to_freeze();
>  +		__set_current_state(TASK_RUNNING);

schedule() and schedule_timeout*() always return in state TASK_RUNNING, so
I'll take that out of there.

We might as well use schedule_timeout_interruptible(), too.  As a bonus, we
get to delete that spelling mistake ;)


--- devel/drivers/edac/edac_mc.c~edac-switch-to-kthread_-api-tidy	2006-03-02 18:27:56.000000000 -0800
+++ devel-akpm/drivers/edac/edac_mc.c	2006-03-02 18:27:56.000000000 -0800
@@ -2042,13 +2042,9 @@ static int edac_kernel_thread(void *arg)
 	while (!kthread_should_stop()) {
 		do_edac_check();
 
-		/* ensure we are interruptable */
-		set_current_state(TASK_INTERRUPTIBLE);
-
 		/* goto sleep for the interval */
-		schedule_timeout((HZ * poll_msec) / 1000);
+		schedule_timeout_interruptible((HZ * poll_msec) / 1000);
 		try_to_freeze();
-		__set_current_state(TASK_RUNNING);
 	}
 
 	return 0;
_

[PATCH 1/15] EDAC: switch to kthread_ API

[Dave Peterson]

This patch was originally posted by Christoph Hellwig (see
http://lkml.org/lkml/2006/2/14/331):

"Christoph Hellwig" <hch@lst.de> wrote:
> Use the kthread_ API instead of opencoding lots of hairy code for kernel
> thread creation and teardown, including tasklist_lock abuse.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Signed-Off-By: David S. Peterson <dsp@llnl.gov> <dave_peterson@pobox.com>
---

Index: linux-2.6.16-rc5-edac/drivers/edac/edac_mc.c
===================================================================
--- linux-2.6.16-rc5-edac.orig/drivers/edac/edac_mc.c	2006-02-27 15:13:54.000000000 -0800
+++ linux-2.6.16-rc5-edac/drivers/edac/edac_mc.c	2006-02-27 16:58:40.000000000 -0800
@@ -29,6 +29,7 @@
 #include <linux/list.h>
 #include <linux/sysdev.h>
 #include <linux/ctype.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <asm/page.h>
@@ -58,6 +59,8 @@ static atomic_t pci_parity_count = ATOMI
 static DECLARE_MUTEX(mem_ctls_mutex);
 static struct list_head mc_devices = LIST_HEAD_INIT(mc_devices);
 
+static struct task_struct *edac_thread;
+
 /* Structure of the whitelist and blacklist arrays */
 struct edac_pci_device_list {
 	unsigned int  vendor;		/* Vendor ID */
@@ -2027,7 +2030,6 @@ static inline void check_mc_devices (voi
  */
 static void do_edac_check(void)
 {
-
 	debugf3("MC: " __FILE__ ": %s()\n", __func__);
 
 	check_mc_devices();
@@ -2035,50 +2037,10 @@ static void do_edac_check(void)
 	do_pci_parity_check();
 }
 
-
-/*
- * EDAC thread state information
- */
-struct bs_thread_info
-{
-	struct task_struct *task;
-	struct completion *event;
-	char *name;
-	void (*run)(void);
-};
-
-static struct bs_thread_info bs_thread;
-
-/*
- *  edac_kernel_thread
- *      This the kernel thread that processes edac operations
- *      in a normal thread environment
- */
 static int edac_kernel_thread(void *arg)
 {
-	struct bs_thread_info *thread = (struct bs_thread_info *) arg;
-
-	/* detach thread */
-	daemonize(thread->name);
-
-	current->exit_signal = SIGCHLD;
-	allow_signal(SIGKILL);
-	thread->task = current;
-
-	/* indicate to starting task we have started */
-	complete(thread->event);
-
-	/* loop forever, until we are told to stop */
-	while(thread->run != NULL) {
-		void (*run)(void);
-
-		/* call the function to check the memory controllers */
-		run = thread->run;
-		if (run)
-			run();
-
-		if (signal_pending(current))
-			flush_signals(current);
+	while (!kthread_should_stop()) {
+		do_edac_check();
 
 		/* ensure we are interruptable */
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -2086,11 +2048,9 @@ static int edac_kernel_thread(void *arg)
 		/* goto sleep for the interval */
 		schedule_timeout((HZ * poll_msec) / 1000);
 		try_to_freeze();
+		__set_current_state(TASK_RUNNING);
 	}
 
-	/* notify waiter that we are exiting */
-	complete(thread->event);
-
 	return 0;
 }
 
@@ -2100,9 +2060,6 @@ static int edac_kernel_thread(void *arg)
  */
 static int __init edac_mc_init(void)
 {
-	int ret;
-	struct completion event;
-
 	printk(KERN_INFO "MC: " __FILE__ " version " EDAC_MC_VERSION "\n");
 
 	/*
@@ -2130,24 +2087,15 @@ static int __init edac_mc_init(void)
 		return -ENODEV;
 	}
 
-	/* Create our kernel thread */
-	init_completion(&event);
-	bs_thread.event = &event;
-	bs_thread.name = "kedac";
-	bs_thread.run = do_edac_check;
-
 	/* create our kernel thread */
-	ret = kernel_thread(edac_kernel_thread, &bs_thread, CLONE_KERNEL);
-	if (ret < 0) {
+	edac_thread = kthread_run(edac_kernel_thread, NULL, "kedac");
+	if (IS_ERR(edac_thread)) {
 		/* remove the sysfs entries */
 		edac_sysfs_memctrl_teardown();
 		edac_sysfs_pci_teardown();
-		return -ENOMEM;
+		return PTR_ERR(edac_thread);
 	}
 
-	/* wait for our kernel theard ack that it is up and running */
-	wait_for_completion(&event);
-
 	return 0;
 }
 
@@ -2158,21 +2106,9 @@ static int __init edac_mc_init(void)
  */
 static void __exit edac_mc_exit(void)
 {
-	struct completion event;
-
 	debugf0("MC: " __FILE__ ": %s()\n", __func__);
 
-	init_completion(&event);
-	bs_thread.event = &event;
-
-	/* As soon as ->run is set to NULL, the task could disappear,
-	 * so we need to hold tasklist_lock until we have sent the signal
-	 */
-	read_lock(&tasklist_lock);
-	bs_thread.run = NULL;
-	send_sig(SIGKILL, bs_thread.task, 1);
-	read_unlock(&tasklist_lock);
-	wait_for_completion(&event);
+	kthread_stop(edac_thread);
 
         /* tear down the sysfs device */
 	edac_sysfs_memctrl_teardown();