OpenVZ Forum - RDF feed
type: channel
items: [PATCH] usbatm: Update to use the kthread api.
items: Re: [PATCH] usbatm: Update to use the kthread api.

[PATCH] usbatm: Update to use the kthread api.
type: item
description: 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, "…
creator: ebiederm
date: 2006-12-12T22:22:50-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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_hea…
creator: Cedric Le Goater
date: 2006-12-13T16:24:17-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: ebiederm
date: 2006-12-13T19:11:01-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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. Thu…
creator: Duncan Sands
date: 2006-12-14T10:44:10-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: > 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
creator: Duncan Sands
date: 2006-12-14T10:45:56-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 = us…
creator: Cedric Le Goater
date: 2006-12-14T11:13:34-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Duncan Sands
date: 2006-12-14T11:30:15-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 mode…
creator: ebiederm
date: 2006-12-14T11:39:50-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 mi…
creator: Duncan Sands
date: 2006-12-14T13:14:38-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Cedric Le Goater
date: 2006-12-14T13:36:46-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Alan Stern
date: 2006-12-14T16:05:16-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 …
creator: ebiederm
date: 2006-12-14T22:51:42-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Christoph Hellwig
date: 2006-12-15T09:16:55-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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…
creator: Duncan Sands
date: 2006-12-15T09:42:37-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: ebiederm
date: 2006-12-15T10:17:57-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Christoph Hellwig
date: 2006-12-15T10:35:01-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: ebiederm
date: 2006-12-15T10:45:40-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: ebiederm
date: 2006-12-15T10:54:00-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Alan Stern
date: 2006-12-15T15:14:06-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Christoph Hellwig
date: 2007-01-02T11:11:19-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Alan Stern
date: 2007-01-02T15:34:16-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 …
creator: ebiederm
date: 2007-01-03T09:05:28-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: ebiederm
date: 2007-01-03T09:08:12-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 _______________________________________________ C…
creator: Alan Stern
date: 2007-01-03T17:52:42-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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
creator: Duncan Sands
date: 2007-01-03T19:12:34-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 fi…
creator: ebiederm
date: 2007-04-19T02:13:56-00:00

Re: [PATCH] usbatm: Update to use the kthread api.
type: item
description: 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 implemen…
creator: Alan Stern
date: 2007-04-19T14:56:51-00:00