During driver initialization if the driver has an expensive
initialization routine usbatm starts a separate kernel thread for it.

In the driver cleanup routine the code waits to ensure the
initialization routine has finished.

Switching to the kthread api allowed some of the thread management
code to be removed.

In addition the kill_proc(SIGTERM, ...) in usbatm_usb_disconnect was
removed because it was absolutely pointless.  The kernel thread did
not handle SIGTERM or any pending signals, so despite marking the
signal as pending it would never have been handled. 

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/usb/atm/usbatm.c |   24 ++++++------------------
 drivers/usb/atm/usbatm.h |    2 --
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index ec63b0e..e6cd5e4 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -81,6 +81,7 @@
 #include <linux/stat.h>
 #include <linux/timer.h>
 #include <linux/wait.h>
+#include <linux/kthread.h>
 
 #ifdef VERBOSE_DEBUG
 static int usbatm_print_packet(const unsigned char *data, int len);
@@ -999,35 +1000,26 @@ static int usbatm_do_heavy_init(void *arg)
 	struct usbatm_data *instance = arg;
 	int ret;
 
-	daemonize(instance->driver->driver_name);
-	allow_signal(SIGTERM);
-	instance->thread_pid = current->pid;
-
-	complete(&instance->thread_started);
-
 	ret = instance->driver->heavy_init(instance, instance->usb_intf);
 
 	if (!ret)
 		ret = usbatm_atm_init(instance);
 
-	mutex_lock(&instance->serialize);
-	instance->thread_pid = -1;
-	mutex_unlock(&instance->serialize);
 
 	complete_and_exit(&instance->thread_exited, ret);
 }
 
 static int usbatm_heavy_init(struct usbatm_data *instance)
 {
-	int ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_KERNEL);
-
-	if (ret < 0) {
+	struct task_struct *thread;
+	thread = kthread_run(usbatm_do_heavy_init, instance,
+				instance->driver->driver_name);
+	if (IS_ERR(thread)) {
+		int ret = PTR_ERR(thread);
 		usb_err(instance, "%s: failed to create kernel_thread (%d)!\n", __func__, ret);
 		return ret;
 	}
 
-	wait_for_completion(&instance->thread_started);
-
 	return 0;
 }
 
@@ -1109,8 +1101,6 @@ int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id,
 	kref_init(&instance->refcount);		/* dropped in usbatm_usb_disconnect */
 	mutex_init(&instance->serialize);
 
-	instance->thread_pid = -1;
-	init_completion(&instance->thread_started);
 	init_completion(&instance->thread_exited);
 
 	INIT_LIST_HEAD(&instance->vcc_list);
@@ -1272,8 +1262,6 @@ void usbatm_usb_disconnect(struct usb_interface *intf)
 
 	mutex_lock(&instance->serialize);
 	instance->disconnected = 1;
-	if (instance->thread_pid >= 0)
-		kill_proc(instance->thread_pid, SIGTERM, 1);
 	mutex_unlock(&instance->serialize);
 
 	wait_for_completion(&instance->thread_exited);
diff --git a/drivers/usb/atm/usbatm.h b/drivers/usb/atm/usbatm.h
index ff8551e..ab42355 100644
--- a/drivers/usb/atm/usbatm.h
+++ b/drivers/usb/atm/usbatm.h
@@ -176,8 +176,6 @@ struct usbatm_data {
 	int disconnected;
 
 	/* heavy init */
-	int thread_pid;
-	struct completion thread_started;
 	struct completion thread_exited;
 
 	/* ATM device */
-- 
1.4.4.1.g278f

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Cedric Le Goater <clg@fr.ibm.com> writes:

> Eric W. Biederman wrote:
>> During driver initialization if the driver has an expensive
>> initialization routine usbatm starts a separate kernel thread for it.
>> 
>> In the driver cleanup routine the code waits to ensure the
>> initialization routine has finished.
>> 
>> Switching to the kthread api allowed some of the thread management
>> code to be removed.
>> 
>> In addition the kill_proc(SIGTERM, ...) in usbatm_usb_disconnect was
>> removed because it was absolutely pointless.  The kernel thread did
>> not handle SIGTERM or any pending signals, so despite marking the
>> signal as pending it would never have been handled. 
>
> are you sure that the heavy_init() routines don't handle pending 
> signals. they do firmware loading, etc. ?

Well I just took a quick look through them to be certain
and I don't see anything that would.  Even inside of the guts of
request firmware.  So I'm pretty certain that SIGTERM was something
originally copied from another kernel_thread implementation and
wound up being dead code.

Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Eric W. Biederman wrote:
> During driver initialization if the driver has an expensive
> initialization routine usbatm starts a separate kernel thread for it.
> 
> In the driver cleanup routine the code waits to ensure the
> initialization routine has finished.
> 
> Switching to the kthread api allowed some of the thread management
> code to be removed.
> 
> In addition the kill_proc(SIGTERM, ...) in usbatm_usb_disconnect was
> removed because it was absolutely pointless.  The kernel thread did
> not handle SIGTERM or any pending signals, so despite marking the
> signal as pending it would never have been handled. 

are you sure that the heavy_init() routines don't handle pending 
signals. they do firmware loading, etc. ?

> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  drivers/usb/atm/usbatm.c |   24 ++++++------------------
>  drivers/usb/atm/usbatm.h |    2 --
>  2 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index ec63b0e..e6cd5e4 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -81,6 +81,7 @@
>  #include <linux/stat.h>
>  #include <linux/timer.h>
>  #include <linux/wait.h>
> +#include <linux/kthread.h>
>  
>  #ifdef VERBOSE_DEBUG
>  static int usbatm_print_packet(const unsigned char *data, int len);
> @@ -999,35 +1000,26 @@ static int usbatm_do_heavy_init(void *arg)
>  	struct usbatm_data *instance = arg;
>  	int ret;
>  
> -	daemonize(instance->driver->driver_name);
> -	allow_signal(SIGTERM);
> -	instance->thread_pid = current->pid;
> -
> -	complete(&instance->thread_started);
> -
>  	ret = instance->driver->heavy_init(instance, instance->usb_intf);
>  
>  	if (!ret)
>  		ret = usbatm_atm_init(instance);
>  
> -	mutex_lock(&instance->serialize);
> -	instance->thread_pid = -1;
> -	mutex_unlock(&instance->serialize);
>  
>  	complete_and_exit(&instance->thread_exited, ret);
>  }
>  
>  static int usbatm_heavy_init(struct usbatm_data *instance)
>  {
> -	int ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_KERNEL);
> -
> -	if (ret < 0) {
> +	struct task_struct *thread;
> +	thread = kthread_run(usbatm_do_heavy_init, instance,
> +				instance->driver->driver_name);
> +	if (IS_ERR(thread)) {
> +		int ret = PTR_ERR(thread);
>  		usb_err(instance, "%s: failed to create kernel_thread (%d)!\n", __func__, ret);
>  		return ret;
>  	}
>  
> -	wait_for_completion(&instance->thread_started);
> -
>  	return 0;
>  }
>  
> @@ -1109,8 +1101,6 @@ int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id,
>  	kref_init(&instance->refcount);		/* dropped in usbatm_usb_disconnect */
>  	mutex_init(&instance->serialize);
>  
> -	instance->thread_pid = -1;
> -	init_completion(&instance->thread_started);
>  	init_completion(&instance->thread_exited);
>  
>  	INIT_LIST_HEAD(&instance->vcc_list);
> @@ -1272,8 +1262,6 @@ void usbatm_usb_disconnect(struct usb_interface *intf)
>  
>  	mutex_lock(&instance->serialize);
>  	instance->disconnected = 1;
> -	if (instance->thread_pid >= 0)
> -		kill_proc(instance->thread_pid, SIGTERM, 1);
>  	mutex_unlock(&instance->serialize);
>  
>  	wait_for_completion(&instance->thread_exited);
> diff --git a/drivers/usb/atm/usbatm.h b/drivers/usb/atm/usbatm.h
> index ff8551e..ab42355 100644
> --- a/drivers/usb/atm/usbatm.h
> +++ b/drivers/usb/atm/usbatm.h
> @@ -176,8 +176,6 @@ struct usbatm_data {
>  	int disconnected;
>  
>  	/* heavy init */
> -	int thread_pid;
> -	struct completion thread_started;
>  	struct completion thread_exited;
>  
>  	/* ATM device */

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Duncan Sands <baldrick@free.fr> writes:

> Hi Eric, thanks for looking into this.
>
>> During driver initialization if the driver has an expensive
>> initialization routine usbatm starts a separate kernel thread for it.
>> 
>> In the driver cleanup routine the code waits to ensure the
>> initialization routine has finished.
>> 
>> Switching to the kthread api allowed some of the thread management
>> code to be removed.
>> 
>> In addition the kill_proc(SIGTERM, ...) in usbatm_usb_disconnect was
>> removed because it was absolutely pointless.  The kernel thread did
>> not handle SIGTERM or any pending signals, so despite marking the
>> signal as pending it would never have been handled.

So first thank you for the review.

> This is wrong, the signal is used.  Let me explain the context, then
> why signals are important.  USB ATM modem drivers register themselves
> with the usbatm core, which organizes the interaction between the USB
> layer, the ATM layer and the modem driver.  Some modems require
> initialization that cannot be performed in the USB probe method.
> When I say "cannot" here, you need to understand that this is mainly
> about quality of service, though there are some correctness issues:
> initializing these modems takes typically 5 seconds or more.  If the
> initialization was done in probe, all other USB device initialization/
> disconnection would have to wait for it to finish (USB probe/disconnect
> is globally serialized, being run from the khubd kernel thread).  This
> is unacceptable, so usbatm provides an easy way to have the extra
> initialization run from within it's own kernel thread: the modem driver
> registers a heavy_init method with usbatm; at the end of probe, heavy_init
> is run in its own kernel thread.  In fact, I've been asked by Alan Stern
> to generalize this functionality into the USB core itself, since something
> like this is needed by a pile of USB drivers.
>
> An important consideration: what if heavy_init is still running
> when the modem is disconnected?  The disconnect method cannot exit
> until the kernel thread has exited; horrible mayhem could result
> otherwise.  Thus disconnect has to wait for the kernel thread to
> finish.  That means that the whole USB subsystem has to wait for
> the kernel thread to exit.  This is problematic, from a quality
> of service point of view, if heavy_init takes a long time to
> finish.  For example, the following line is executed by the
> heavy_init in speedtch.c:
>
>         msleep_interruptible(1000);
>
> This is relatively mild, but already shows the problem: disconnect
> can take more than one second to exit.  There are much worse cases
> (more on this later).
>
> In short, the usbatm core needs a way to tell the heavy_init method
> that the game is up (due to disconnect).  I chose to have it send
> a signal to the kernel thread.  This seemed to be the simplest way.
> If the sending of a signal is removed, something else will have to
> replace it.  Before I discuss how the signal is handled by existing
> heavy_init methods, I should point out that even if none of the
> existing heavy_init methods made any use of the signal, it would
> still be wrong to remove it: a not-yet written heavy_init might well
> need to use it.  But in fact the existing heavy_init routines do
> make use of the signal.
>
> For example, consider speedtch_upload_firmware in speedtch.c.  It
> does two things: it sends a bunch of urbs to the modem, and it
> performs the above msleep_interruptible.  If disconnect is called,
> any urbs in progress promptly fail and any newly submitted urbs fail
> at once; thus the only thing that can take an appreciable amount of
> time is the msleep_interruptible.  But this will also exit at once
> because of the signal sent by usbatm during disconnect.  So, in this
> case, the signal reduces the maximum time the USB subsystem is blocked
> in disconnect from one second to zero seconds.
>
> Now consider firmware loading, a nasty case.  This is also done in
> heavy_init, and can take an infinite amount of time if the firmware is
> not found (the user can choose an infinite timeout); the default timeout
> of 10 seconds is already plenty long.  Firmware loading also needs to exit
> at once if the modem is disconnected.  You may well wonder how
> speedtch_heavy_init arranges to cancel firmware loading when the signal
> comes in.  The answer is that it doesn't cancel it.  But this is not a
> reason to remove the sending of the signal, it is a reason to improve
> speedtch_heavy_init.  This is not so easy, because the firmware subsystem
> doesn't give the kernel any way of cancelling a firmware load once started,
> which is why it doesn't happen right now.
>
> Once you accept that a signal needs to be sent, you can't remove all
> those completions etc that your patch deleted, because it introduces
> races: they are there to make sure that (a) signals are unblocked in the
> thread before the signal can possibly be sent, and (b) the signal is not
> sent to the wrong thread if the kernel thread exits at a badly chosen
> moment and the pid is recycled.  I believe the current setup is race
> free, but please don't hesitate to correct me.  If you want to get rid
> of the pid and instead use a pointer to the thread then avoiding race (b)
> becomes even more important, since then rather than shooting down the wrong
> thread you send a signal to a thread which no longer exists, surely a fatal
> mistake.

Actually I don't accept that a signal needs to be sent.  I do accept
that the message needs to be delivered to stop things early.

The paradigm in a kthread world for waking up kernel threads is by
calling kthread_stop, and then for testing if a kernel thread should
stop is by calling kthread_should_stop.

Especially if you are looking at generalizing this code over all of
usb it should probably be using the current kernel best practices.

There is still an issue with msleep here that I completely concede.
In particular neither msleep nor msleep interruptible will actually be
awoken by kthread_stop.  So it looks like we need a msleep_kthread
that will won't go back to sleep if after kthread_stop wakes it up.
Still unless I am blind that looks like a very minor change from where
we are now. 

I think the reduction in complexity and the increase in uniformity
is most likely worth it.

If all else fails I'm happy with something simpler like Cedric's
patch which takes care of the things that I currently have a problem
with, but I'm willing to work through this to make it a through
cleanup.

Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Thu, 14 Dec 2006, Eric W. Biederman wrote:

> Actually I don't accept that a signal needs to be sent.  I do accept
> that the message needs to be delivered to stop things early.
> 
> The paradigm in a kthread world for waking up kernel threads is by
> calling kthread_stop, and then for testing if a kernel thread should
> stop is by calling kthread_should_stop.
> 
> Especially if you are looking at generalizing this code over all of
> usb it should probably be using the current kernel best practices.
> 
> There is still an issue with msleep here that I completely concede.
> In particular neither msleep nor msleep interruptible will actually be
> awoken by kthread_stop.  So it looks like we need a msleep_kthread
> that will won't go back to sleep if after kthread_stop wakes it up.
> Still unless I am blind that looks like a very minor change from where
> we are now. 

Something else to think about.  I've got a driver that starts up a kernel 
thread which calls vfs_read() and vfs_write() and relies on signals to 
interrupt the I/O operations when necessary.  Perhaps this approach is 
fundamentally wrong, but I'm not sure how else to do it.

Alan Stern

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Hi Eric, thanks for looking into this.

> During driver initialization if the driver has an expensive
> initialization routine usbatm starts a separate kernel thread for it.
> 
> In the driver cleanup routine the code waits to ensure the
> initialization routine has finished.
> 
> Switching to the kthread api allowed some of the thread management
> code to be removed.
> 
> In addition the kill_proc(SIGTERM, ...) in usbatm_usb_disconnect was
> removed because it was absolutely pointless.  The kernel thread did
> not handle SIGTERM or any pending signals, so despite marking the
> signal as pending it would never have been handled.

This is wrong, the signal is used.  Let me explain the context, then
why signals are important.  USB ATM modem drivers register themselves
with the usbatm core, which organizes the interaction between the USB
layer, the ATM layer and the modem driver.  Some modems require
initialization that cannot be performed in the USB probe method.
When I say "cannot" here, you need to understand that this is mainly
about quality of service, though there are some correctness issues:
initializing these modems takes typically 5 seconds or more.  If the
initialization was done in probe, all other USB device initialization/
disconnection would have to wait for it to finish (USB probe/disconnect
is globally serialized, being run from the khubd kernel thread).  This
is unacceptable, so usbatm provides an easy way to have the extra
initialization run from within it's own kernel thread: the modem driver
registers a heavy_init method with usbatm; at the end of probe, heavy_init
is run in its own kernel thread.  In fact, I've been asked by Alan Stern
to generalize this functionality into the USB core itself, since something
like this is needed by a pile of USB drivers.

An important consideration: what if heavy_init is still running
when the modem is disconnected?  The disconnect method cannot exit
until the kernel thread has exited; horrible mayhem could result
otherwise.  Thus disconnect has to wait for the kernel thread to
finish.  That means that the whole USB subsystem has to wait for
the kernel thread to exit.  This is problematic, from a quality
of service point of view, if heavy_init takes a long time to
finish.  For example, the following line is executed by the
heavy_init in speedtch.c:

        msleep_interruptible(1000);

This is relatively mild, but already shows the problem: disconnect
can take more than one second to exit.  There are much worse cases
(more on this later).

In short, the usbatm core needs a way to tell the heavy_init method
that the game is up (due to disconnect).  I chose to have it send
a signal to the kernel thread.  This seemed to be the simplest way.
If the sending of a signal is removed, something else will have to
replace it.  Before I discuss how the signal is handled by existing
heavy_init methods, I should point out that even if none of the
existing heavy_init methods made any use of the signal, it would
still be wrong to remove it: a not-yet written heavy_init might well
need to use it.  But in fact the existing heavy_init routines do
make use of the signal.

For example, consider speedtch_upload_firmware in speedtch.c.  It
does two things: it sends a bunch of urbs to the modem, and it
performs the above msleep_interruptible.  If disconnect is called,
any urbs in progress promptly fail and any newly submitted urbs fail
at once; thus the only thing that can take an appreciable amount of
time is the msleep_interruptible.  But this will also exit at once
because of the signal sent by usbatm during disconnect.  So, in this
case, the signal reduces the maximum time the USB subsystem is blocked
in disconnect from one second to zero seconds.

Now consider firmware loading, a nasty case.  This is also done in
heavy_init, and can take an infinite amount of time if the firmware is
not found (the user can choose an infinite timeout); the default timeout
of 10 seconds is already plenty long.  Firmware loading also needs to exit
at once if the modem is disconnected.  You may well wonder how
speedtch_heavy_init arranges to cancel firmware loading when the signal
comes in.  The answer is that it doesn't cancel it.  But this is not a
reason to remove the sending of the signal, it is a reason to improve
speedtch_heavy_init.  This is not so easy, because the firmware subsystem
doesn't give the kernel any way of cancelling a firmware load once started,
which is why it doesn't happen right now.

Once you accept that a signal needs to be sent, you can't remove all
those completions etc that your patch deleted, because it introduces
races: they are there to make sure that (a) signals are unblocked in the
thread before the signal can possibly be sent, and (b) the signal is not
sent to the wrong thread if the kernel thread exits at a badly chosen
moment and the pid is recycled.  I believe the current setup is race
free, but please don't hesitate to correct me.  If you want to get rid
of the pid and instead use a pointer to the thread then avoiding race (b)
becomes even more important, since then rather than shooting down the wrong
thread you send a signal to a thread which no longer exists, surely a fatal
mistake.

So it's a NACK for your current patch I'm afraid.

Best wishes,

Duncan Sands.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

> Well I just took a quick look through them to be certain
> and I don't see anything that would.  Even inside of the guts of
> request firmware.  So I'm pretty certain that SIGTERM was something
> originally copied from another kernel_thread implementation and
> wound up being dead code.

Not at all, it was all written from scratch (so now you know who to
blame :) ).  And the signal *is* used, as explained in my reply to
your original email.

Ciao,

Duncan.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Hi Duncan,

Duncan Sands wrote:
>> Well I just took a quick look through them to be certain
>> and I don't see anything that would.  Even inside of the guts of
>> request firmware.  So I'm pretty certain that SIGTERM was something
>> originally copied from another kernel_thread implementation and
>> wound up being dead code.
> 
> Not at all, it was all written from scratch (so now you know who to
> blame :) ).  And the signal *is* used, as explained in my reply to
> your original email.

Here's one I have been keeping for a while. Nothing really fancy : 
basic replacement of kernel_thread and removal of the start
completion which is now covered in kthread.

Cheers,

C.

Subject: replace kernel_thread() with kthread_run() in usbatm

From: Cedric Le Goater <clg@fr.ibm.com>

This patch replaces the kernel_thread() with kthread_run() since
kernel_thread() is deprecated in drivers/modules.

Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>
Cc: Duncan Sands <duncan.sands@free.fr>
Cc: linux-usb-users@lists.sourceforge.net
Cc: linux-usb-devel@lists.sourceforge.net

---
 drivers/usb/atm/usbatm.c |   26 ++++++++++++--------------
 drivers/usb/atm/usbatm.h |    3 +--
 2 files changed, 13 insertions(+), 16 deletions(-)

Index: 2.6.19-mm1/drivers/usb/atm/usbatm.c
===================================================================
--- 2.6.19-mm1.orig/drivers/usb/atm/usbatm.c
+++ 2.6.19-mm1/drivers/usb/atm/usbatm.c
@@ -81,6 +81,7 @@
 #include <linux/stat.h>
 #include <linux/timer.h>
 #include <linux/wait.h>
+#include <linux/kthread.h>
 
 #ifdef VERBOSE_DEBUG
 static int usbatm_print_packet(const unsigned char *data, int len);
@@ -999,11 +1000,7 @@ static int usbatm_do_heavy_init(void *ar
 	struct usbatm_data *instance = arg;
 	int ret;
 
-	daemonize(instance->driver->driver_name);
 	allow_signal(SIGTERM);
-	instance->thread_pid = current->pid;
-
-	complete(&instance->thread_started);
 
 	ret = instance->driver->heavy_init(instance, instance->usb_intf);
 
@@ -1011,7 +1008,7 @@ static int usbatm_do_heavy_init(void *ar
 		ret = usbatm_atm_init(instance);
 
 	mutex_lock(&instance->serialize);
-	instance->thread_pid = -1;
+	instance->thread_task = NULL;
 	mutex_unlock(&instance->serialize);
 
 	complete_and_exit(&instance->thread_exited, ret);
@@ -1019,15 +1016,17 @@ static int usbatm_do_heavy_init(void *ar
 
 static int usbatm_heavy_init(struct usbatm_data *instance)
 {
-	int ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_KERNEL);
+	instance->thread_task = kthread_run(usbatm_do_heavy_init, instance,
+					    instance->driver->driver_name);
 
-	if (ret < 0) {
-		usb_err(instance, "%s: failed to create kernel_thread (%d)!\n", __func__, ret);
+	if (IS_ERR(instance->thread_task)) {
+		int ret = PTR_ERR(instance->thread_task);
+		usb_err(instance, "%s: failed to create kthread (%d)!\n",
+			__func__, ret);
+		instance->thread_task = NULL;
 		return ret;
 	}
 
-	wait_for_completion(&instance->thread_started);
-
 	return 0;
 }
 
@@ -1109,8 +1108,7 @@ int usbatm_usb_probe(struct usb_interfac
 	kref_init(&instance->refcount);		/* dropped in usbatm_usb_disconnect */
 	mutex_init(&instance->serialize);
 
-	instance->thread_pid = -1;
-	init_completion(&instance->thread_started);
+	instance->thread_task = NULL;
 	init_completion(&instance->thread_exited);
 
 	INIT_LIST_HEAD(&instance->vcc_list);
@@ -1272,8 +1270,8 @@ void usbatm_usb_disconnect(struct usb_in
 
 	mutex_lock(&instance->serialize);
 	instance->disconnected = 1;
-	if (instance->thread_pid >= 0)
-		kill_proc(instance->thread_pid, SIGTERM, 1);
+	if (instance->thread_task)
+		send_sig(SIGTERM, instance->thread_task, 1);
 	mutex_unlock(&instance->serialize);
 
 	wait_for_completion(&instance->thread_exited);
Index: 2.6.19-mm1/drivers/usb/atm/usbatm.h
===================================================================
--- 2.6.19-mm1.orig/drivers/usb/atm/usbatm.h
+++ 2.6.19-mm1/drivers/usb/atm/usbatm.h
@@ -176,8 +176,7 @@ struct usbatm_data {
 	int disconnected;
 
 	/* heavy init */
-	int thread_pid;
-	struct completion thread_started;
+	struct task_struct *thread_task;
 	struct completion thread_exited;
 
 	/* ATM device */
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Hi Cedric,

> Here's one I have been keeping for a while. Nothing really fancy : 
> basic replacement of kernel_thread and removal of the start
> completion which is now covered in kthread.

the signal needs to be unblocked before the start completion,
since otherwise the signal might be sent while signals are
blocked (though this is extremely unlikely).  So you can't
rely on kthread's start completion I'm afraid, because that
happens before the unblocking.

Ciao,

Duncan.

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Hi Eric,

> > ...
> > Once you accept that a signal needs to be sent, you can't remove all
> > those completions etc that your patch deleted, because it introduces
> > races: they are there to make sure that (a) signals are unblocked in the
> > thread before the signal can possibly be sent, and (b) the signal is not
> > sent to the wrong thread if the kernel thread exits at a badly chosen
> > moment and the pid is recycled.  I believe the current setup is race
> > free, but please don't hesitate to correct me.  If you want to get rid
> > of the pid and instead use a pointer to the thread then avoiding race (b)
> > becomes even more important, since then rather than shooting down the wrong
> > thread you send a signal to a thread which no longer exists, surely a fatal
> > mistake.
> 
> Actually I don't accept that a signal needs to be sent.  I do accept
> that the message needs to be delivered to stop things early.

I'm not in love with signals either, however...

> The paradigm in a kthread world for waking up kernel threads is by
> calling kthread_stop, and then for testing if a kernel thread should
> stop is by calling kthread_should_stop.

I considered this, but rejected it because of this comment:

 * kthread_stop - stop a thread created by kthread_create().
 * ... Your threadfn() must not call do_exit()
 * itself if you use this function! ...

and this one:

 * ... @threadfn can either call do_exit() directly if it is a
 * standalone thread for which noone will call kthread_stop(), or
 * return when 'kthread_should_stop()' is true (which means
 * kthread_stop() has been called).

Most of the time the kernel thread starts, performs heavy_init,
and exits.  The above comments seem to imply that it is wrong
to call do_exit if kthread_stop might be called, and wrong to
return if kthread_stop has not been called.  This seems to exclude
the case where kthread_stop is sometimes, but not always, called,
and the thread sometimes exits without kthread_stop having been
called.  But perhaps I misunderstood, since it seems there is kthread
code to handle the case of a threadfn that returns without kthread_stop
having been called, witness this comment:
        /* It might have exited on its own, w/o kthread_stop.  Check. */
It's still not clear to me, so if you can enlighten me, please do!

> Especially if you are looking at generalizing this code over all of
> usb it should probably be using the current kernel best practices.
> 
> There is still an issue with msleep here that I completely concede.
> In particular neither msleep nor msleep interruptible will actually be
> awoken by kthread_stop.  So it looks like we need a msleep_kthread
> that will won't go back to sleep if after kthread_stop wakes it up.
> Still unless I am blind that looks like a very minor change from where
> we are now. 

Sure.

> I think the reduction in complexity and the increase in uniformity
> is most likely worth it.
> 
> If all else fails I'm happy with something simpler like Cedric's
> patch which takes care of the things that I currently have a problem
> with, but I'm willing to work through this to make it a through
> cleanup.

You have a problem with the pid, right?  Well, that is easily
cured in itself.  I'll spin a patch for it a bit later, unless
someone else gets there first.  And if you can confirm that kthread_stop
can be used in this situation (i.e. thread can spontaneously return
without kthread_stop) then I'm happy to convert everyone over to checking
kthread_should_stop.

Ciao,

Duncan.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

I've added Christoph to Cc: for his expertise in kthread conversions. 

> ...
>
> You have a problem with the pid, right?  Well, that is easily
> cured in itself.  I'll spin a patch for it a bit later, unless
> someone else gets there first.  And if you can confirm that kthread_stop
> can be used in this situation (i.e. thread can spontaneously return
> without kthread_stop) then I'm happy to convert everyone over to checking
> kthread_should_stop.

C.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Grr.  I missed this message first time through, the copy
addressed to me did not make only the mailing list copy made
it :(

Duncan Sands <baldrick@free.fr> writes:


> I'm not in love with signals either, however...
>
>> The paradigm in a kthread world for waking up kernel threads is by
>> calling kthread_stop, and then for testing if a kernel thread should
>> stop is by calling kthread_should_stop.
>
> I considered this, but rejected it because of this comment:
>
>  * kthread_stop - stop a thread created by kthread_create().
>  * ... Your threadfn() must not call do_exit()
>  * itself if you use this function! ...
>
> and this one:
>
>  * ... @threadfn can either call do_exit() directly if it is a
>  * standalone thread for which noone will call kthread_stop(), or
>  * return when 'kthread_should_stop()' is true (which means
>  * kthread_stop() has been called).
>
> Most of the time the kernel thread starts, performs heavy_init,
> and exits.  The above comments seem to imply that it is wrong
> to call do_exit if kthread_stop might be called, and wrong to
> return if kthread_stop has not been called.  This seems to exclude
> the case where kthread_stop is sometimes, but not always, called,
> and the thread sometimes exits without kthread_stop having been
> called.  But perhaps I misunderstood, since it seems there is kthread
> code to handle the case of a threadfn that returns without kthread_stop
> having been called, witness this comment:
>         /* It might have exited on its own, w/o kthread_stop.  Check. */
> It's still not clear to me, so if you can enlighten me, please do!

This is a good point.  I was going to say we could work around
this with checks in usbatm_do_heavy_init but that appears racy.

I guess I need to think a little more.  I remember seeing this
and not worry about it because SIGTERM didn't seem to
be caught, so it didn't appear needed.

>> I think the reduction in complexity and the increase in uniformity
>> is most likely worth it.
>> 
>> If all else fails I'm happy with something simpler like Cedric's
>> patch which takes care of the things that I currently have a problem
>> with, but I'm willing to work through this to make it a through
>> cleanup.
>
> You have a problem with the pid, right?  Well, that is easily
> cured in itself.  I'll spin a patch for it a bit later, unless
> someone else gets there first.  And if you can confirm that kthread_stop
> can be used in this situation (i.e. thread can spontaneously return
> without kthread_stop) then I'm happy to convert everyone over to checking
> kthread_should_stop.

To be clear I have a problem with using numeric pids of kernel threads,
and with spawning threads from a possibly user space environment.
So on my hitlist are (kill_proc, daemonize, and kernel_thread). 

If it the original process is a user space thread it is possible to
capture pieces of the user space environment unintentionally daemonize
is supposed to fix that but only does for the pieces of user 
space environment that people have anticipated you can capture.

Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Christoph Hellwig <hch@infradead.org> writes:

> Can you add a little bit of context what all this is about, please?
>
> On Thu, Dec 14, 2006 at 02:36:46PM +0100, Cedric Le Goater wrote:
>> 
>> I've added Christoph to Cc: for his expertise in kthread conversions. 
>> 
>> > ...
>> >
>> > You have a problem with the pid, right?  Well, that is easily
>> > cured in itself.  I'll spin a patch for it a bit later, unless
>> > someone else gets there first.  And if you can confirm that kthread_stop
>> > can be used in this situation (i.e. thread can spontaneously return
>> > without kthread_stop) then I'm happy to convert everyone over to checking
>> > kthread_should_stop.

In the long slow process to build container support in the linux kernel
one of the items on our todo list is the kernel_thread to kthread conversion.

While converting the usbatm driver we hit what is at least a partial snag.
I was hoping to remove the sending of signals along with the rest of
the conversion, but I hit a surprising use.

The usb atm drivers have some long running initializers (several seconds
potentially.   So the infrastructure forks off a kernel thread to run them.

The code really does not care if the thread completes or does anything
else until a usb disconnect comes in.  The in wants to wait suggest the
initialization code stop early and abort and then wait until the
initialization is done.

The practical problem is what is the best way to handle that case.

Can we use the kthread_should_stop() test in a thread that can
exit on it's own before kthread_stop is called?

Are signals the best available mechanism to request that a thread
stop that can exit on it's own.

If we don't suggest to the thread to stop having it call
complete_and_exit seems to the simplest race free solution.  The
request to stop though makes things trickier.


Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Christoph Hellwig <hch@infradead.org> writes:

> On Fri, Dec 15, 2006 at 03:17:57AM -0700, Eric W. Biederman wrote:
>
> I think the right fix is to encehance the kthread infrastructure to
> gracefully handle the case where the threat has stopped by itself
> and doesn't exist anymore at the time where we call kthread_stop.

Yep that is about where I thought we were at.  Now we need to figure out
how to do that cleanly, and sanely.

Eric

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Duncan Sands <baldrick@free.fr> writes:

> Hi Eric,
>
> presumably the problem is that if the thread has spontaneously exited, and
> afterwards disconnect calls kthread_stop, then things go boom.  The same
> problem exists (though with lesser consequences) when sending a signal.
> There is already code in usbatm to avoid this problem with signals.  Why
> not just recycle it in the kthread_stop case?  I guess there is no
> problem if you can guarantee that the following occurs:
> if kthread_stop is ever called for the kthread, then the kthread only
> exits after seeing kthread_should_stop return true.

I suspect we can recycle the locking on the signal sending code.  At
least as a first pass.   I have almost digested the problem sufficiently
to write some code. Maybe this weekend.

>> To be clear I have a problem with using numeric pids of kernel threads,
>
> Yes, this is a problem with usbatm at the moment.
>
>> and with spawning threads from a possibly user space environment.
>
> Not the case with usbatm.  It is always spawned from khubd.

That is where I thought we were at, doing the conversion so it
is obvious and we can remove the use of kernel_thread and daemonize
would certainly be good.  The more shared infrastructure we can
reasonably have the more likely the code will function correctly.

Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Can you add a little bit of context what all this is about, please?

On Thu, Dec 14, 2006 at 02:36:46PM +0100, Cedric Le Goater wrote:
> 
> I've added Christoph to Cc: for his expertise in kthread conversions. 
> 
> > ...
> >
> > You have a problem with the pid, right?  Well, that is easily
> > cured in itself.  I'll spin a patch for it a bit later, unless
> > someone else gets there first.  And if you can confirm that kthread_stop
> > can be used in this situation (i.e. thread can spontaneously return
> > without kthread_stop) then I'm happy to convert everyone over to checking
> > kthread_should_stop.


_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Fri, 15 Dec 2006, Christoph Hellwig wrote:

> > Are signals the best available mechanism to request that a thread
> > stop that can exit on it's own.
> 
> Defintly not.  signals should be avoided in kernel threads at all
> cost.

I have a driver that spawns a kernel thread (using kthread_create) which 
does I/O by calling vfs_write and vfs_read.  It relies on signals to 
interrupt the I/O activity when necessary.  Maybe this isn't a good way of 
doing things, but I couldn't think of anything better.

Do you have any suggestions?

Alan Stern

P.S.: What is the reason for saying "signals should be avoided in kernel
threads at all cost"?

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Fri, Dec 15, 2006 at 03:17:57AM -0700, Eric W. Biederman wrote:
> While converting the usbatm driver we hit what is at least a partial snag.
> I was hoping to remove the sending of signals along with the rest of
> the conversion, but I hit a surprising use.
> 
> The usb atm drivers have some long running initializers (several seconds
> potentially.   So the infrastructure forks off a kernel thread to run them.
> 
> The code really does not care if the thread completes or does anything
> else until a usb disconnect comes in.  The in wants to wait suggest the
> initialization code stop early and abort and then wait until the
> initialization is done.
> 
> The practical problem is what is the best way to handle that case.
> 
> Can we use the kthread_should_stop() test in a thread that can
> exit on it's own before kthread_stop is called?

Right now it can't.

> Are signals the best available mechanism to request that a thread
> stop that can exit on it's own.

Defintly not.  signals should be avoided in kernel threads at all
cost.

> If we don't suggest to the thread to stop having it call
> complete_and_exit seems to the simplest race free solution.  The
> request to stop though makes things trickier.

I think the right fix is to encehance the kthread infrastructure to
gracefully handle the case where the threat has stopped by itself
and doesn't exist anymore at the time where we call kthread_stop.

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Hi Eric,

> >> The paradigm in a kthread world for waking up kernel threads is by
> >> calling kthread_stop, and then for testing if a kernel thread should
> >> stop is by calling kthread_should_stop.
> >
> > I considered this, but rejected it because of this comment:
> >
> >  * kthread_stop - stop a thread created by kthread_create().
> >  * ... Your threadfn() must not call do_exit()
> >  * itself if you use this function! ...
> >
> > and this one:
> >
> >  * ... @threadfn can either call do_exit() directly if it is a
> >  * standalone thread for which noone will call kthread_stop(), or
> >  * return when 'kthread_should_stop()' is true (which means
> >  * kthread_stop() has been called).
> >
> > Most of the time the kernel thread starts, performs heavy_init,
> > and exits.  The above comments seem to imply that it is wrong
> > to call do_exit if kthread_stop might be called, and wrong to
> > return if kthread_stop has not been called.  This seems to exclude
> > the case where kthread_stop is sometimes, but not always, called,
> > and the thread sometimes exits without kthread_stop having been
> > called.  But perhaps I misunderstood, since it seems there is kthread
> > code to handle the case of a threadfn that returns without kthread_stop
> > having been called, witness this comment:
> >         /* It might have exited on its own, w/o kthread_stop.  Check. */
> > It's still not clear to me, so if you can enlighten me, please do!
> 
> This is a good point.  I was going to say we could work around
> this with checks in usbatm_do_heavy_init but that appears racy.

presumably the problem is that if the thread has spontaneously exited, and
afterwards disconnect calls kthread_stop, then things go boom.  The same
problem exists (though with lesser consequences) when sending a signal.
There is already code in usbatm to avoid this problem with signals.  Why
not just recycle it in the kthread_stop case?  I guess there is no
problem if you can guarantee that the following occurs:
if kthread_stop is ever called for the kthread, then the kthread only
exits after seeing kthread_should_stop return true.

> I guess I need to think a little more.  I remember seeing this
> and not worry about it because SIGTERM didn't seem to
> be caught, so it didn't appear needed.
> 
> >> I think the reduction in complexity and the increase in uniformity
> >> is most likely worth it.
> >> 
> >> If all else fails I'm happy with something simpler like Cedric's
> >> patch which takes care of the things that I currently have a problem
> >> with, but I'm willing to work through this to make it a through
> >> cleanup.
> >
> > You have a problem with the pid, right?  Well, that is easily
> > cured in itself.  I'll spin a patch for it a bit later, unless
> > someone else gets there first.  And if you can confirm that kthread_stop
> > can be used in this situation (i.e. thread can spontaneously return
> > without kthread_stop) then I'm happy to convert everyone over to checking
> > kthread_should_stop.
> 
> To be clear I have a problem with using numeric pids of kernel threads,

Yes, this is a problem with usbatm at the moment.

> and with spawning threads from a possibly user space environment.

Not the case with usbatm.  It is always spawned from khubd.

> So on my hitlist are (kill_proc, daemonize, and kernel_thread). 
> 
> If it the original process is a user space thread it is possible to
> capture pieces of the user space environment unintentionally daemonize
> is supposed to fix that but only does for the pieces of user 
> space environment that people have anticipated you can capture.

Best wishes,

Duncan.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Tue, 2 Jan 2007, Christoph Hellwig wrote:

> > I have a driver that spawns a kernel thread (using kthread_create) which 
> > does I/O by calling vfs_write and vfs_read.  It relies on signals to 
> > interrupt the I/O activity when necessary.  Maybe this isn't a good way of 
> > doing things, but I couldn't think of anything better.
> 
> Given that we have no other way to interrupt I/O then signals at those
> lower level I don't see a way around the singals if you stick to that
> higher level design.

Okay.

> > P.S.: What is the reason for saying "signals should be avoided in kernel
> > threads at all cost"?
> 
> The probem with signals is that they can come from various sources, most
> notably from random kill commands issues from userland.  This defeats
> the notion of a fixed thread lifetime under control of the owning module.
> Of course this issue doesn't exist for you above useage where you'd
> hopefully avoid allowing signals that could terminate the thread.

In my case the situation is exactly the reverse: I _want_ to allow signals
to terminate the thread (as well as allowing signals to interrupt I/O).

The reason is simple enough.  At system shutdown, if the thread isn't
terminated then it would continue to own an open file, preventing that
file's filesystem from being unmounted cleanly.  Since people should be
able to unmount their disks during shutdown without having to unload
drivers first, the simplest solution is to allow the thread to respond to
the TERM signal normally sent by the shutdown scripts.

Since the thread is owned by the kernel, random kill commands won't have 
any bad effect.  Only kill commands sent by the superuser can terminate 
the thread.

Alan Stern

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Fri, Dec 15, 2006 at 10:14:06AM -0500, Alan Stern wrote:
> On Fri, 15 Dec 2006, Christoph Hellwig wrote:
> 
> > > Are signals the best available mechanism to request that a thread
> > > stop that can exit on it's own.
> > 
> > Defintly not.  signals should be avoided in kernel threads at all
> > cost.
> 
> I have a driver that spawns a kernel thread (using kthread_create) which 
> does I/O by calling vfs_write and vfs_read.  It relies on signals to 
> interrupt the I/O activity when necessary.  Maybe this isn't a good way of 
> doing things, but I couldn't think of anything better.

Given that we have no other way to interrupt I/O then signals at those
lower level I don't see a way around the singals if you stick to that
higher level design.

> P.S.: What is the reason for saying "signals should be avoided in kernel
> threads at all cost"?

The probem with signals is that they can come from various sources, most
notably from random kill commands issues from userland.  This defeats
the notion of a fixed thread lifetime under control of the owning module.
Of course this issue doesn't exist for you above useage where you'd
hopefully avoid allowing signals that could terminate the thread.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 2 Jan 2007, Christoph Hellwig wrote:
>
>> > I have a driver that spawns a kernel thread (using kthread_create) which 
>> > does I/O by calling vfs_write and vfs_read.  It relies on signals to 
>> > interrupt the I/O activity when necessary.  Maybe this isn't a good way of 
>> > doing things, but I couldn't think of anything better.
>> 
>> Given that we have no other way to interrupt I/O then signals at those
>> lower level I don't see a way around the singals if you stick to that
>> higher level design.
>
> Okay.
>
>> > P.S.: What is the reason for saying "signals should be avoided in kernel
>> > threads at all cost"?
>> 
>> The probem with signals is that they can come from various sources, most
>> notably from random kill commands issues from userland.  This defeats
>> the notion of a fixed thread lifetime under control of the owning module.
>> Of course this issue doesn't exist for you above useage where you'd
>> hopefully avoid allowing signals that could terminate the thread.
>
> In my case the situation is exactly the reverse: I _want_ to allow signals
> to terminate the thread (as well as allowing signals to interrupt I/O).
>
> The reason is simple enough.  At system shutdown, if the thread isn't
> terminated then it would continue to own an open file, preventing that
> file's filesystem from being unmounted cleanly.  Since people should be
> able to unmount their disks during shutdown without having to unload
> drivers first, the simplest solution is to allow the thread to respond to
> the TERM signal normally sent by the shutdown scripts.
>
> Since the thread is owned by the kernel, random kill commands won't have 
> any bad effect.  Only kill commands sent by the superuser can terminate 
> the thread.
>

Why in the world would a thread hold a file open for an indeterminate duration?
That seems very wrong.

I can just about understand reading a firmware file or something like that
and close the file afterwards.  But unless you are worrying about a very small
window I think we have a problem here.

Eric
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Christoph Hellwig <hch@infradead.org> writes:

> Given that we have no other way to interrupt I/O then signals at those
> lower level I don't see a way around the singals if you stick to that
> higher level design.

It isn't hard to either modify signal_pending or the place where the
signal pending checks are to terminate things.

>> P.S.: What is the reason for saying "signals should be avoided in kernel
>> threads at all cost"?
>
> The probem with signals is that they can come from various sources, most
> notably from random kill commands issues from userland.  This defeats
> the notion of a fixed thread lifetime under control of the owning module.
> Of course this issue doesn't exist for you above useage where you'd
> hopefully avoid allowing signals that could terminate the thread.

Right unless you can get a state where user space is not allowed to send
signals but the kernel is.  But still reusing the concept if it doesn't quite
fit sounds like a definition mess.

Eric



_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Wed, 3 Jan 2007, Eric W. Biederman wrote:

> >> The probem with signals is that they can come from various sources, most
> >> notably from random kill commands issues from userland.  This defeats
> >> the notion of a fixed thread lifetime under control of the owning module.
> >> Of course this issue doesn't exist for you above useage where you'd
> >> hopefully avoid allowing signals that could terminate the thread.
> >
> > In my case the situation is exactly the reverse: I _want_ to allow signals
> > to terminate the thread (as well as allowing signals to interrupt I/O).

I should have been clearer here.  Signals don't terminate the thread; they
merely notify it to clean up and terminate itself.

> > The reason is simple enough.  At system shutdown, if the thread isn't
> > terminated then it would continue to own an open file, preventing that
> > file's filesystem from being unmounted cleanly.  Since people should be
> > able to unmount their disks during shutdown without having to unload
> > drivers first, the simplest solution is to allow the thread to respond to
> > the TERM signal normally sent by the shutdown scripts.
> >
> > Since the thread is owned by the kernel, random kill commands won't have 
> > any bad effect.  Only kill commands sent by the superuser can terminate 
> > the thread.
> >
> 
> Why in the world would a thread hold a file open for an indeterminate duration?
> That seems very wrong.

The thread uses the file to provide backing storage.  Kind of like the
loop driver, except that my driver uses a higher-level interface into the
VFS than the loop driver does, for greater simplicity.  You wouldn't say
that what the loop driver does is wrong, would you?

> I can just about understand reading a firmware file or something like that
> and close the file afterwards.  But unless you are worrying about a very small
> window I think we have a problem here.

I don't follow.  Why do you think there's a problem?

Alan Stern

_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

On Friday 15 December 2006 11:45, Eric W. Biederman wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Fri, Dec 15, 2006 at 03:17:57AM -0700, Eric W. Biederman wrote:
> >
> > I think the right fix is to encehance the kthread infrastructure to
> > gracefully handle the case where the threat has stopped by itself
> > and doesn't exist anymore at the time where we call kthread_stop.
> 
> Yep that is about where I thought we were at.  Now we need to figure out
> how to do that cleanly, and sanely.

There's a completely different solution, which is to use a workqueue
instead of a kthread, with users providing a cancellation method.
Recall that the functionality is provided by usbatm to drivers:
they use it to perform slow initialization that is too slow to be
done in the probe method.  They register with usbatm, providing a
"heavy_init" method.  They could also provide a "heavy_cancel" method.
(Any special data that heavy_cancel needs can be stored in the
existing driver private data structure; this structure is already
passed to heavy_init).  In the case of the speedtch driver, it
could place a completion in its private data structure; heavy_cancel
would just complete the completion.  Rather than doing interruptible
sleeps, it can use wait_for_completion_timeout.

The only thing that worries me about this solution is... that you
can't shoot down firmware loading from userspace anymore.  For
example, if heavy_init is blocked loading firmware when the system
is halted, it presumably won't react to the kill signal.  Perhaps
it is unimportant; and if not, I guess I can just re-enable signals
in heavy_init.

Ciao,

Duncan.
_______________________________________________
Containers mailing list
Containers@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 2 Jan 2007, Christoph Hellwig wrote:
>
>> > I have a driver that spawns a kernel thread (using kthread_create) which 
>> > does I/O by calling vfs_write and vfs_read.  It relies on signals to 
>> > interrupt the I/O activity when necessary.  Maybe this isn't a good way of 
>> > doing things, but I couldn't think of anything better.
>> 
>> Given that we have no other way to interrupt I/O then signals at those
>> lower level I don't see a way around the singals if you stick to that
>> higher level design.
>
> Okay.
>
>> > P.S.: What is the reason for saying "signals should be avoided in kernel
>> > threads at all cost"?
>> 
>> The probem with signals is that they can come from various sources, most
>> notably from random kill commands issues from userland.  This defeats
>> the notion of a fixed thread lifetime under control of the owning module.
>> Of course this issue doesn't exist for you above useage where you'd
>> hopefully avoid allowing signals that could terminate the thread.
>
> In my case the situation is exactly the reverse: I _want_ to allow signals
> to terminate the thread (as well as allowing signals to interrupt I/O).
>
> The reason is simple enough.  At system shutdown, if the thread isn't
> terminated then it would continue to own an open file, preventing that
> file's filesystem from being unmounted cleanly.  Since people should be
> able to unmount their disks during shutdown without having to unload
> drivers first, the simplest solution is to allow the thread to respond to
> the TERM signal normally sent by the shutdown scripts.

You need a real user space interface.  Historically user space is
required to call unmount and the like, you should have a proper
shutdown interface and script as well.

> Since the thread is owned by the kernel, random kill commands won't have 
> any bad effect.  Only kill commands sent by the superuser can terminate 
> the thread.

I'm putting the final touches on a patchset to finish converting
all kernel threads to the kthread API.    So I have had a chance
to digest the arguments.

Upgrading kthreads to allow them to handle the when a thread exits
on it's own, and to set signal_pending so they terminate interruptible 
sleeps was easy.

Handling signals from user space in kernel threads is a maintenance
disaster.  It makes the kernel thread part of the ABI and makes it
so you can never change the implementation if user space comes to
rely on using them.  If we don't handle signals kernel threads
remain an implementation detail leaving us free to change the
implementation later.

A pid namespace trivially changes the implementation making all kernel
threads invisible.  Which means you can't send a signal to them.

So since sending signals to kernel threads is weird in terms of
semantics, it prevents from changing implementation details, and
because no kernel thread in the kernel appears to actually require it
at this time I refuse to support the concept.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, 18 Apr 2007, Eric W. Biederman wrote:

> > In my case the situation is exactly the reverse: I _want_ to allow signals
> > to terminate the thread (as well as allowing signals to interrupt I/O).
> >
> > The reason is simple enough.  At system shutdown, if the thread isn't
> > terminated then it would continue to own an open file, preventing that
> > file's filesystem from being unmounted cleanly.  Since people should be
> > able to unmount their disks during shutdown without having to unload
> > drivers first, the simplest solution is to allow the thread to respond to
> > the TERM signal normally sent by the shutdown scripts.
> 
> You need a real user space interface.  Historically user space is
> required to call unmount and the like, you should have a proper
> shutdown interface and script as well.

Well, "kill <pid>" _is_ a real userspace interface.

Suppose you have a normal user application that runs a background process.  
It's not a system service or anything like that, so it isn't controlled by
the runtime scripts in /etc/rc.d/init.d or wherever.  How do you manage to
unmount the filesystems it uses at shutdown time?  Simple -- you kill the
background process.  Or rather, you rely on the shutdown script to kill
it for you.

My driver should work in the same way.  Transparently.  The user shouldn't
need to do anything special to shut down when the driver is loaded.

> I'm putting the final touches on a patchset to finish converting
> all kernel threads to the kthread API.    So I have had a chance
> to digest the arguments.
> 
> Upgrading kthreads to allow them to handle the when a thread exits
> on it's own, and to set signal_pending so they terminate interruptible 
> sleeps was easy.
> 
> Handling signals from user space in kernel threads is a maintenance
> disaster.  It makes the kernel thread part of the ABI and makes it
> so you can never change the implementation if user space comes to
> rely on using them.  If we don't handle signals kernel threads
> remain an implementation detail leaving us free to change the
> implementation later.
> 
> A pid namespace trivially changes the implementation making all kernel
> threads invisible.  Which means you can't send a signal to them.

What happens if there are user processes running in a PID namespace 
different from the one used by the shutdown script?  They won't get killed 
at shutdown time, so the script won't be able to unmount the filesystems 
they use.

> So since sending signals to kernel threads is weird in terms of
> semantics, it prevents from changing implementation details, and
> because no kernel thread in the kernel appears to actually require it
> at this time I refuse to support the concept.

_My_ thread is in the kernel and it actually requires it.  Unless you can 
suggest a suitable alternative mechanism.  For example, if you provided a 
way for the thread to allow its PID always to show up in the PID namespace 
used by the shutdown scripts, that would resolve the problem.  Or if you 
suggested a way for the thread to hold an open file reference that would 
automatically be closed when the filesystem was unmounted, that would work 
too.

The alternative I don't like at all is to tell people using the driver
that they need to add

	grep -q g_file_storage /proc/modules && rmmod g_file_storage 

somewhere in their /etc/rc.d/init.d/halt script.

Alan Stern

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers