[RFC] [PATCH] UBI: convert to kthread API

[RFC] [PATCH] UBI: convert to kthread API

[Alexander Schmidt]

Hi Artem

UBI should use the kthread API, which makes completions and signal
handling go away.

I took Christoph on CC, I hope he has some comments on this.

Regards,
Alex

Signed-off-by: Alexander Schmidt <alexs@linux.vnet.ibm.com>
---
 drivers/mtd/ubi/background.c |   71 ++++++++++---------------------------------
 drivers/mtd/ubi/build.c      |    4 --
 drivers/mtd/ubi/ubi.h        |    5 ---
 3 files changed, 18 insertions(+), 62 deletions(-)

--- dedekind-ubi-2.6.orig/drivers/mtd/ubi/background.c
+++ dedekind-ubi-2.6/drivers/mtd/ubi/background.c
@@ -27,6 +27,7 @@
  */
 
 #include <linux/freezer.h>
+#include <linux/kthread.h>
 #include "ubi.h"
 
 /* Background thread name pattern */
@@ -160,23 +161,6 @@ void ubi_bgt_disable(struct ubi_info *ub
 }
 
 /**
- * ubi_bgt_kill_thread - kill the background thread.
- *
- * @ubi: the UBI device description object
- *
- * This function kills the background thread for UBI device defined by @ubi.
- * This function also makes sure all the pending tasks are done.
- */
-void ubi_bgt_kill_thread(struct ubi_info *ubi)
-{
-	dbg_bgt("disable \"%s\"", ubi->bgt.bgt_name);
-	if (ubi->bgt.task) {
-		send_sig(SIGKILL, ubi->bgt.task, 1);
-		wait_for_completion(&ubi->bgt.thread_stop);
-	}
-}
-
-/**
  * ubi_bgt_do_work - do one pending work.
  *
  * @ubi: the UBI device description object
@@ -235,46 +219,22 @@ static int ubi_thread(void *u)
 	int failures = 0;
 	struct ubi_info *ubi = u;
 
-	daemonize(ubi->bgt.bgt_name);
-	allow_signal(SIGKILL);
-	allow_signal(SIGSTOP);
-
 	ubi_msg("background thread \"%s\" started, PID %d",
 		ubi->bgt.bgt_name, current->pid);
 
-	ubi->bgt.task = current;
-	complete(&ubi->bgt.thread_start);
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule();
-
 	for (;;) {
-		cond_resched();
-
 		if (unlikely(!ubi->bgt.enabled) ||
 			     list_empty(&ubi->bgt.pending_works)) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			schedule();
 		}
 
+		if (kthread_should_stop())
+			goto out;
+
 		if (try_to_freeze())
 			continue;
 
-		while (signal_pending(current)) {
-			siginfo_t info;
-			unsigned long nr;
-
-			nr = dequeue_signal_lock(current, &current->blocked,
-						 &info);
-			if (nr == SIGKILL)
-				goto out;
-			if (nr == SIGSTOP) {
-				ubi->bgt.enabled = !ubi->bgt.enabled;
-				ubi_msg("%s the background thread",
-					ubi->bgt.enabled ? "enable" :
-							   "disable");
-			}
-		}
-
 		spin_lock(&ubi->bgt.lock);
 		while (ubi->bgt.pending_works_count > 0 &&
 		       likely(ubi->bgt.enabled)) {
@@ -297,7 +257,6 @@ static int ubi_thread(void *u)
 					ubi_msg("%d consecutive failures, "
 						"disable the background thread",
 						BGT_MAX_FAILURES);
-					ubi_bgt_disable(ubi);
 					ubi_eba_ro_mode(ubi);
 					break;
 				} else
@@ -307,6 +266,8 @@ static int ubi_thread(void *u)
 			spin_lock(&ubi->bgt.lock);
 		}
 		spin_unlock(&ubi->bgt.lock);
+
+		cond_resched();
 	}
 
 out:
@@ -330,7 +291,7 @@ out:
 	}
 	spin_unlock(&ubi->bgt.lock);
 
-	complete_and_exit(&ubi->bgt.thread_stop, 0);
+	return 0;
 }
 
 /**
@@ -344,12 +305,9 @@ out:
 int ubi_bgt_init(struct ubi_info *ubi)
 {
 	int err;
-	pid_t pid;
 
 	dbg_bgt("initialize the UBI background thread unit");
 
-	init_completion(&ubi->bgt.thread_start);
-	init_completion(&ubi->bgt.thread_stop);
 	INIT_LIST_HEAD(&ubi->bgt.pending_works);
 	spin_lock_init(&ubi->bgt.lock);
 	mutex_init(&ubi->bgt.wrk_mutex);
@@ -359,14 +317,15 @@ int ubi_bgt_init(struct ubi_info *ubi)
 		return -ENOMEM;
 	sprintf(ubi->bgt.bgt_name, BGT_NAME_PATTERN, ubi->ubi_num);
 
-	pid = kernel_thread(ubi_thread, ubi, CLONE_FS | CLONE_FILES);
-	if (pid < 0) {
-		err = pid;
-		ubi_err("cannot spawn \"%s\", error %d", ubi->bgt.bgt_name, err);
+	ubi->bgt.task = kthread_create(ubi_thread, ubi, ubi->bgt.bgt_name);
+	if (IS_ERR(ubi->bgt.task)) {
+		err = PTR_ERR(ubi->bgt.task);
+		ubi_err("cannot spawn \"%s\", error %d", ubi->bgt.bgt_name,
+			err);
 		goto out_name;
 	}
 
-	wait_for_completion(&ubi->bgt.thread_start);
+	wake_up_process(ubi->bgt.task);
 	dbg_bgt("the UBI background thread unit is initialized");
 	return 0;
 
@@ -382,6 +341,10 @@ out_name:
  */
 void ubi_bgt_close(struct ubi_info *ubi)
 {
+	dbg_bgt("disable \"%s\"", ubi->bgt.bgt_name);
+	if (ubi->bgt.task)
+		kthread_stop(ubi->bgt.task);
+
 	dbg_bgt("close the UBI background thread unit");
 
 	ubi_assert(!ubi->bgt.enabled);
--- dedekind-ubi-2.6.orig/drivers/mtd/ubi/build.c
+++ dedekind-ubi-2.6/drivers/mtd/ubi/build.c
@@ -123,12 +123,11 @@ static void detach_mtd_dev(struct ubi_in
 
 	dbg_bld("detaching mtd%d from ubi%d", mtd_num, ubi_num);
 
-	ubi_bgt_kill_thread(ubi);
+	ubi_bgt_close(ubi);
 	ubi_uif_close(ubi);
 	ubi_eba_close(ubi);
 	ubi_wl_close(ubi);
 	ubi_vmt_close(ubi);
-	ubi_bgt_close(ubi);
 	ubi_io_close(ubi);
 	kfree(ubis[ubi_num]);
 	ubis[ubi_num] = NULL;
@@ -276,7 +275,6 @@ out_detach:
 	ubi_wl_close(ubi);
 	ubi_vmt_close(ubi);
 out_bgt:
-	ubi_bgt_kill_thread(ubi);
 	ubi_bgt_close(ubi);
 out_io:
 	ubi_io_close(ubi);
--- dedekind-ubi-2.6.orig/drivers/mtd/ubi/ubi.h
+++ dedekind-ubi-2.6/drivers/mtd/ubi/ubi.h
@@ -239,8 +239,6 @@ struct ubi_bgt_work {
  * @enabled: if the background thread is enabled
  * @task: a pointer to the &struct task_struct of the background thread
  * @bgt_name: the background thread name
- * @thread_start: used to synchronize with starting of the background thread
- * @thread_stop: used to synchronize with killing of the background thread
  * @wrk_mutex: serializes execution if background works
  */
 struct ubi_bgt_info {
@@ -251,8 +249,6 @@ struct ubi_bgt_info {
 	int enabled;                      /* public  */
 	struct task_struct *task;         /* private */
 	char *bgt_name;                   /* public  */
-	struct completion thread_start;   /* private */
-	struct completion thread_stop;    /* private */
 	struct mutex wrk_mutex;           /* private */
 };
 
@@ -551,7 +547,6 @@ int ubi_bgt_reschedule(struct ubi_info *
 int ubi_bgt_do_work(struct ubi_info *ubi);
 int ubi_bgt_enable(struct ubi_info *ubi);
 void ubi_bgt_disable(struct ubi_info *ubi);
-void ubi_bgt_kill_thread(struct ubi_info *ubi);
 int ubi_bgt_init(struct ubi_info *ubi);
 void ubi_bgt_close(struct ubi_info *ubi);
 

Re: [RFC] [PATCH] UBI: convert to kthread API

[Artem Bityutskiy]

Hello Alexander,

On Tue, 2007-02-27 at 14:50 +0100, Alexander Schmidt wrote:
> UBI should use the kthread API, which makes completions and signal
> handling go away.

how feasible and possible is to get rid of this UBI unit altogether?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [RFC] [PATCH] UBI: convert to kthread API

[Josh Boyer]

On Tue, 2007-02-27 at 19:47 +0200, Artem Bityutskiy wrote:
> Hello Alexander,
> 
> On Tue, 2007-02-27 at 14:50 +0100, Alexander Schmidt wrote:
> > UBI should use the kthread API, which makes completions and signal
> > handling go away.
> 
> how feasible and possible is to get rid of this UBI unit altogether?

Depends I suppose.  Is it going to make a large runtime functionality or
performance impact if a background thread isn't running?

josh

Re: [RFC] [PATCH] UBI: convert to kthread API

[Artem Bityutskiy]

On Tue, 2007-02-27 at 12:04 -0600, Josh Boyer wrote:
> On Tue, 2007-02-27 at 19:47 +0200, Artem Bityutskiy wrote:
> > Hello Alexander,
> > 
> > On Tue, 2007-02-27 at 14:50 +0100, Alexander Schmidt wrote:
> > > UBI should use the kthread API, which makes completions and signal
> > > handling go away.
> > 
> > how feasible and possible is to get rid of this UBI unit altogether?
> 
> Depends I suppose.  Is it going to make a large runtime functionality or
> performance impact if a background thread isn't running?

Sorry for vagueness, I do not mean to remove th background _process_, we
really need it. I meant to remove the UBI unit source-wise and use the
kthread calls directly. I am busy with other stuff right now and wanted
Alexander to check how much ugliness or niceness we would introduce with
that change.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [RFC] [PATCH] UBI: convert to kthread API

[Alexander Schmidt]

On Tuesday 27 February 2007, Artem Bityutskiy wrote:
> On Tue, 2007-02-27 at 12:04 -0600, Josh Boyer wrote:
> > On Tue, 2007-02-27 at 19:47 +0200, Artem Bityutskiy wrote:
> > > Hello Alexander,
> > > 
> > > On Tue, 2007-02-27 at 14:50 +0100, Alexander Schmidt wrote:
> > > > UBI should use the kthread API, which makes completions and signal
> > > > handling go away.
> > > 
> > > how feasible and possible is to get rid of this UBI unit altogether?
> > 
> > Depends I suppose.  Is it going to make a large runtime functionality or
> > performance impact if a background thread isn't running?
> 
> Sorry for vagueness, I do not mean to remove th background _process_, we
> really need it. I meant to remove the UBI unit source-wise and use the
> kthread calls directly. I am busy with other stuff right now and wanted
> Alexander to check how much ugliness or niceness we would introduce with
> that change.

Basically, with using the kthread api, it is still neccessary to have the
following functions:

1) start the thread with kthread_create()
2) a main loop for the thread
3) enqueue work in the pending work list
4) kill/stop the thread

As 3) is only done by the wear leveling unit, the code could be moved
there, IMO. Starting and stoping the thread could also be done in the
wl init and close functions, so it seems sensible to move everything to
the wear leveling unit.

A point that causes complexity in the code is that we have two ways of
stopping the thread:

1) If ubi goes into read only mode or if the thread is disabled via sysfs,
the thread is put to sleep, pending works are not finished
2) If ubi is shut down, the thread finishes all pending works and then
exits.

An enhancement here would be to remove the sysfs functionality, and make
the thread exit when going in ro_mode, while still not finishing the
pending work (it is not possible to leave ro_mode during runtime, right?)

Regards,
Alex

Re: [RFC] [PATCH] UBI: convert to kthread API

[Artem Bityutskiy]

On Wed, 2007-02-28 at 09:15 +0100, Alexander Schmidt wrote:
> Basically, with using the kthread api, it is still neccessary to have the
> following functions:
> 
> 1) start the thread with kthread_create()
> 2) a main loop for the thread
> 3) enqueue work in the pending work list
> 4) kill/stop the thread
> 
> As 3) is only done by the wear leveling unit, the code could be moved
> there, IMO. Starting and stoping the thread could also be done in the
> wl init and close functions, so it seems sensible to move everything to
> the wear leveling unit.
> 
> A point that causes complexity in the code is that we have two ways of
> stopping the thread:
> 
> 1) If ubi goes into read only mode or if the thread is disabled via sysfs,
> the thread is put to sleep, pending works are not finished
> 2) If ubi is shut down, the thread finishes all pending works and then
> exits.
> 
> An enhancement here would be to remove the sysfs functionality, and make
> the thread exit when going in ro_mode, while still not finishing the
> pending work (it is not possible to leave ro_mode during runtime, right?)

My unsorted points:

A. Well, at the moment we switch to RO mode if we hit an error and do
not know how to handle it. And yes, at the moment it is forever, but
later we may want to add a sysfs entry to leave the RO mode, for example
jwb already wants it.

B. Also, at the moment UBI does not work if the bgt is killed, but this
is not nice and will be fixed at some point - we should be able to do
everything if the thread is killed.

C. The stuff which stops the thread is _debugging_ stuff and you may
remove it if it causes troubles. I will add something like "do not spawn
the thread" debugging option later when I need it.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [RFC] [PATCH] UBI: convert to kthread API

[Alexander Schmidt]

On Wednesday 28 February 2007, Artem Bityutskiy wrote:
> C. The stuff which stops the thread is _debugging_ stuff and you may
> remove it if it causes troubles. I will add something like "do not spawn
> the thread" debugging option later when I need it.

I have finished a patch which removes the sysfs write function and has
some further simpifications. The code is still in its own background unit,
so please comment on moving it or not. I will send "try 2" then.

Regards,
Alex

Re: [RFC] [PATCH] UBI: convert to kthread API

[Artem Bityutskiy]

On Wed, 2007-02-28 at 15:48 +0100, Alexander Schmidt wrote:
> On Wednesday 28 February 2007, Artem Bityutskiy wrote:
> > C. The stuff which stops the thread is _debugging_ stuff and you may
> > remove it if it causes troubles. I will add something like "do not spawn
> > the thread" debugging option later when I need it.
> 
> I have finished a patch which removes the sysfs write function and has
> some further simpifications. The code is still in its own background unit,
> so please comment on moving it or not. I will send "try 2" then.

I don't get it what you mean, just send the patch I take a look.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)