Re: [PATCH] kthread: Spontaneous exit support
On Tue, Apr 24, 2007 at 03:08:57PM +0200, Jan Engelhardt wrote:
> >I don't think having to parallel APIs is a good idea, people will
> >get utterly confused which one to use. Better always grab a reference
> >in kthread_create and drop it in kthread_stop. For normal thread
> >no change in behaviour and only slightly more code in the slowpath.
>
> I *am* already confused... a driver of mine does:
>
> static __init int thkd_init(void)
> {
> touch_task = kthread_run(touch_thread, Device, "thkd");
> ...
> }
>
> and
>
> static __exit void thkd_exit(void)
> {
> kthread_stop(touch_task);
> /* I bet something is missing */
> }
>
> now what good would kthread_run do me?
Please read the kerneldoc documentation for kthread_create
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Apr 23 2007 12:25, Christoph Hellwig wrote:
>On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
>>
>> This patch implements the kthread helper functions kthread_start
>> and kthread_end which make it simple to support a kernel thread
>> that may decided to exit on it's own before we request it to.
>> It is still assumed that eventually we will get around to requesting
>> that the kernel thread stop.
>
>I don't think having to parallel APIs is a good idea, people will
>get utterly confused which one to use. Better always grab a reference
>in kthread_create and drop it in kthread_stop. For normal thread
>no change in behaviour and only slightly more code in the slowpath.
I *am* already confused... a driver of mine does:
static __init int thkd_init(void)
{
touch_task = kthread_run(touch_thread, Device, "thkd");
...
}
and
static __exit void thkd_exit(void)
{
kthread_stop(touch_task);
/* I bet something is missing */
}
now what good would kthread_run do me?
Jan
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Apr 23 2007 12:25, Christoph Hellwig wrote:
On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
This patch implements the kthread helper functions kthread_start
and kthread_end which make it simple to support a kernel thread
that may decided to exit on it's own before we request it to.
It is still assumed that eventually we will get around to requesting
that the kernel thread stop.
I don't think having to parallel APIs is a good idea, people will
get utterly confused which one to use. Better always grab a reference
in kthread_create and drop it in kthread_stop. For normal thread
no change in behaviour and only slightly more code in the slowpath.
I *am* already confused... a driver of mine does:
static __init int thkd_init(void)
{
touch_task = kthread_run(touch_thread, Device, thkd);
...
}
and
static __exit void thkd_exit(void)
{
kthread_stop(touch_task);
/* I bet something is missing */
}
now what good would kthread_run do me?
Jan
--
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Tue, Apr 24, 2007 at 03:08:57PM +0200, Jan Engelhardt wrote:
I don't think having to parallel APIs is a good idea, people will
get utterly confused which one to use. Better always grab a reference
in kthread_create and drop it in kthread_stop. For normal thread
no change in behaviour and only slightly more code in the slowpath.
I *am* already confused... a driver of mine does:
static __init int thkd_init(void)
{
touch_task = kthread_run(touch_thread, Device, thkd);
...
}
and
static __exit void thkd_exit(void)
{
kthread_stop(touch_task);
/* I bet something is missing */
}
now what good would kthread_run do me?
Please read the kerneldoc documentation for kthread_create
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On 04/23, Eric W. Biederman wrote: > > So I propose we add a kthread_orphan as a basic primitive to decrement the > count on the task_struct if we want a kthread to simply exit after it > has done some work. > > And as a helper function we can have a kthread_run_orphan. Speaking about helpers, could we also add kthread_start(), which should be used instead of direct wake_up_process() ? Not that it is terribly important, but still. Note that "kthread_create() pins the task_struct" allows us to cleanup the code. Look at this ugly "wait_to_die:" label in migration_thread(). Is is because migration_thread() can't exit until CPU_DEAD reaps it. Other reasons were already solved by kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Mon, Apr 23, 2007 at 11:45:51AM -0600, Eric W. Biederman wrote: > Ok. Thinking about it I agree with Christoph that parallel API's can > be a problem. > > However we do still need to support kernel threads where kthread_stop will > never be called. There appear to be a few legitimate cases where > someone wants to fire off a thread and have it do some work but don't > care at all for stopping it before it is done. There's two cases where it's valid that kthread_stop is not called: a) the user is always builtin and the thread runs until the kernel halts. examples: voyager, arm ecard b) the thread is normally started/stopped, e.g. at module_init/module_exit but there is some reason why it could terminate earlier. examples: the various bluetooth threads, nfs-related threads that can be killed using signals c) we have some kind of asynchronous helper thread. examples: various s390 drivers, usbatm, therm_pm72 d) a driver has threadpools were we need to start/stop threads on demand. examples: nfsd, xpc case a) is trivial, we can just ignore the refcounting issue. case b) is what refcounting the task struct and proper handling in kthread_stop will deal with. case c) should get a new kthread_create_async api which starts a thread without blocking, so we can get rid of the workqueues in the s390 drivers. it should probably also be safe to be called from irq context. What makes this a bit complicated is the need to make sure no more thread is running in case the caller terminates (shutdown of the structure it's associated with or module removal) case d) should be deal with with a kthread_pool api - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
Oleg Nesterov <[EMAIL PROTECTED]> writes: > On 04/23, Christoph Hellwig wrote: >> >> On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote: >> > >> > This patch implements the kthread helper functions kthread_start >> > and kthread_end which make it simple to support a kernel thread >> > that may decided to exit on it's own before we request it to. >> > It is still assumed that eventually we will get around to requesting >> > that the kernel thread stop. >> >> I don't think having to parallel APIs is a good idea, people will >> get utterly confused which one to use. Better always grab a reference >> in kthread_create and drop it in kthread_stop. For normal thread >> no change in behaviour and only slightly more code in the slowpath. >> >> Of course it will need an audit for half-assed kthread conversion >> first to avoid task_struct reference count leaks. > > In that case it is better to grab a reference in kthread(). This also > close the race when a new thread is woken (freezer) and exits before > kthread_create() does get_task_struct(). > >> In addition to that kthrad_end implementation look wrong. When >> the kthread has exited prematurely no one will call complete >> on kthread_stop_info.done before it's been setup. > > This is not true anymore, see another patch from Eric > > kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Ok. Thinking about it I agree with Christoph that parallel API's can be a problem. However we do still need to support kernel threads where kthread_stop will never be called. There appear to be a few legitimate cases where someone wants to fire off a thread and have it do some work but don't care at all for stopping it before it is done. So I propose we add a kthread_orphan as a basic primitive to decrement the count on the task_struct if we want a kthread to simply exit after it has done some work. And as a helper function we can have a kthread_run_orphan. I think having a kthread_orphan will document what we are doing better and make it easy to find kernel threads that don't use kthread_stop. The pain is that this requires an audit of all kernel kthread creators so that we call kthread_orphan on the right ones, or else we will have a task_struct leak. At least that isn't a fatal condition. Eric - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On 04/23, Christoph Hellwig wrote: > > On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote: > > > > This patch implements the kthread helper functions kthread_start > > and kthread_end which make it simple to support a kernel thread > > that may decided to exit on it's own before we request it to. > > It is still assumed that eventually we will get around to requesting > > that the kernel thread stop. > > I don't think having to parallel APIs is a good idea, people will > get utterly confused which one to use. Better always grab a reference > in kthread_create and drop it in kthread_stop. For normal thread > no change in behaviour and only slightly more code in the slowpath. > > Of course it will need an audit for half-assed kthread conversion > first to avoid task_struct reference count leaks. In that case it is better to grab a reference in kthread(). This also close the race when a new thread is woken (freezer) and exits before kthread_create() does get_task_struct(). > In addition to that kthrad_end implementation look wrong. When > the kthread has exited prematurely no one will call complete > on kthread_stop_info.done before it's been setup. This is not true anymore, see another patch from Eric kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
>
> This patch implements the kthread helper functions kthread_start
> and kthread_end which make it simple to support a kernel thread
> that may decided to exit on it's own before we request it to.
> It is still assumed that eventually we will get around to requesting
> that the kernel thread stop.
I don't think having to parallel APIs is a good idea, people will
get utterly confused which one to use. Better always grab a reference
in kthread_create and drop it in kthread_stop. For normal thread
no change in behaviour and only slightly more code in the slowpath.
Of course it will need an audit for half-assed kthread conversion
first to avoid task_struct reference count leaks.
In addition to that kthrad_end implementation look wrong. When
the kthread has exited prematurely no one will call complete
on kthread_stop_info.done before it's been setup. Interestingly
the comment there indicates someone thought about threads exiting
early, but it became defunkt during all the rewrites of the
kthread code.
> +/**
> + * kthread_start - create and wake a thread.
> + * @threadfn: the function to run until kthread_should_stop().
> + * @data: data ptr for @threadfn.
> + * @namefmt: printf-style name for the thread.
> + *
> + * Description: Convenient wrapper for kthread_create() followed by
> + * get_task_struct() and wake_up_process. kthread_start should be paired
> + * with kthread_end() so we don't leak task structs.
> + *
> + * Returns the kthread or ERR_PTR(-ENOMEM).
> + */
> +#define kthread_start(threadfn, data, namefmt, ...) \
> +({ \
> + struct task_struct *__k\
> + = kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
> + if (!IS_ERR(__k)) {\
> + get_task_struct(__k); \
> + wake_up_process(__k); \
> + } \
> + __k; \
> +})
> +int kthread_end(struct task_struct *k);
>
> static inline int __kthread_should_stop(struct task_struct *tsk)
> {
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 9b3c19f..d6d63c6 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -179,6 +179,24 @@ int kthread_stop(struct task_struct *tsk)
> }
> EXPORT_SYMBOL(kthread_stop);
>
> +/**
> + * kthread_end - signal a kthread and wait for it to exit.
> + * @task: The kthread to end.
> + *
> + * Description: Convenient wrapper for kthread_stop() followed by
> + * put_task_struct(). Returns the kthread exit code.
> + *
> + * kthread_start()/kthread_end() can handle kthread that spontaneously exit
> + * before the kthread is requested to terminate.
> + */
> +int kthread_end(struct task_struct *task)
> +{
> + int ret;
> + ret = kthread_stop(task);
> + put_task_struct(task);
> + return ret;
> +}
> +EXPORT_SYMBOL(kthread_end);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote:
This patch implements the kthread helper functions kthread_start
and kthread_end which make it simple to support a kernel thread
that may decided to exit on it's own before we request it to.
It is still assumed that eventually we will get around to requesting
that the kernel thread stop.
I don't think having to parallel APIs is a good idea, people will
get utterly confused which one to use. Better always grab a reference
in kthread_create and drop it in kthread_stop. For normal thread
no change in behaviour and only slightly more code in the slowpath.
Of course it will need an audit for half-assed kthread conversion
first to avoid task_struct reference count leaks.
In addition to that kthrad_end implementation look wrong. When
the kthread has exited prematurely no one will call complete
on kthread_stop_info.done before it's been setup. Interestingly
the comment there indicates someone thought about threads exiting
early, but it became defunkt during all the rewrites of the
kthread code.
+/**
+ * kthread_start - create and wake a thread.
+ * @threadfn: the function to run until kthread_should_stop().
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * get_task_struct() and wake_up_process. kthread_start should be paired
+ * with kthread_end() so we don't leak task structs.
+ *
+ * Returns the kthread or ERR_PTR(-ENOMEM).
+ */
+#define kthread_start(threadfn, data, namefmt, ...) \
+({ \
+ struct task_struct *__k\
+ = kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+ if (!IS_ERR(__k)) {\
+ get_task_struct(__k); \
+ wake_up_process(__k); \
+ } \
+ __k; \
+})
+int kthread_end(struct task_struct *k);
static inline int __kthread_should_stop(struct task_struct *tsk)
{
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9b3c19f..d6d63c6 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -179,6 +179,24 @@ int kthread_stop(struct task_struct *tsk)
}
EXPORT_SYMBOL(kthread_stop);
+/**
+ * kthread_end - signal a kthread and wait for it to exit.
+ * @task: The kthread to end.
+ *
+ * Description: Convenient wrapper for kthread_stop() followed by
+ * put_task_struct(). Returns the kthread exit code.
+ *
+ * kthread_start()/kthread_end() can handle kthread that spontaneously exit
+ * before the kthread is requested to terminate.
+ */
+int kthread_end(struct task_struct *task)
+{
+ int ret;
+ ret = kthread_stop(task);
+ put_task_struct(task);
+ return ret;
+}
+EXPORT_SYMBOL(kthread_end);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On 04/23, Christoph Hellwig wrote: On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote: This patch implements the kthread helper functions kthread_start and kthread_end which make it simple to support a kernel thread that may decided to exit on it's own before we request it to. It is still assumed that eventually we will get around to requesting that the kernel thread stop. I don't think having to parallel APIs is a good idea, people will get utterly confused which one to use. Better always grab a reference in kthread_create and drop it in kthread_stop. For normal thread no change in behaviour and only slightly more code in the slowpath. Of course it will need an audit for half-assed kthread conversion first to avoid task_struct reference count leaks. In that case it is better to grab a reference in kthread(). This also close the race when a new thread is woken (freezer) and exits before kthread_create() does get_task_struct(). In addition to that kthrad_end implementation look wrong. When the kthread has exited prematurely no one will call complete on kthread_stop_info.done before it's been setup. This is not true anymore, see another patch from Eric kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
Oleg Nesterov [EMAIL PROTECTED] writes: On 04/23, Christoph Hellwig wrote: On Sun, Apr 22, 2007 at 09:12:55PM -0600, Eric W. Biederman wrote: This patch implements the kthread helper functions kthread_start and kthread_end which make it simple to support a kernel thread that may decided to exit on it's own before we request it to. It is still assumed that eventually we will get around to requesting that the kernel thread stop. I don't think having to parallel APIs is a good idea, people will get utterly confused which one to use. Better always grab a reference in kthread_create and drop it in kthread_stop. For normal thread no change in behaviour and only slightly more code in the slowpath. Of course it will need an audit for half-assed kthread conversion first to avoid task_struct reference count leaks. In that case it is better to grab a reference in kthread(). This also close the race when a new thread is woken (freezer) and exits before kthread_create() does get_task_struct(). In addition to that kthrad_end implementation look wrong. When the kthread has exited prematurely no one will call complete on kthread_stop_info.done before it's been setup. This is not true anymore, see another patch from Eric kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Ok. Thinking about it I agree with Christoph that parallel API's can be a problem. However we do still need to support kernel threads where kthread_stop will never be called. There appear to be a few legitimate cases where someone wants to fire off a thread and have it do some work but don't care at all for stopping it before it is done. So I propose we add a kthread_orphan as a basic primitive to decrement the count on the task_struct if we want a kthread to simply exit after it has done some work. And as a helper function we can have a kthread_run_orphan. I think having a kthread_orphan will document what we are doing better and make it easy to find kernel threads that don't use kthread_stop. The pain is that this requires an audit of all kernel kthread creators so that we call kthread_orphan on the right ones, or else we will have a task_struct leak. At least that isn't a fatal condition. Eric - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On Mon, Apr 23, 2007 at 11:45:51AM -0600, Eric W. Biederman wrote: Ok. Thinking about it I agree with Christoph that parallel API's can be a problem. However we do still need to support kernel threads where kthread_stop will never be called. There appear to be a few legitimate cases where someone wants to fire off a thread and have it do some work but don't care at all for stopping it before it is done. There's two cases where it's valid that kthread_stop is not called: a) the user is always builtin and the thread runs until the kernel halts. examples: voyager, arm ecard b) the thread is normally started/stopped, e.g. at module_init/module_exit but there is some reason why it could terminate earlier. examples: the various bluetooth threads, nfs-related threads that can be killed using signals c) we have some kind of asynchronous helper thread. examples: various s390 drivers, usbatm, therm_pm72 d) a driver has threadpools were we need to start/stop threads on demand. examples: nfsd, xpc case a) is trivial, we can just ignore the refcounting issue. case b) is what refcounting the task struct and proper handling in kthread_stop will deal with. case c) should get a new kthread_create_async api which starts a thread without blocking, so we can get rid of the workqueues in the s390 drivers. it should probably also be safe to be called from irq context. What makes this a bit complicated is the need to make sure no more thread is running in case the caller terminates (shutdown of the structure it's associated with or module removal) case d) should be deal with with a kthread_pool api - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Spontaneous exit support
On 04/23, Eric W. Biederman wrote: So I propose we add a kthread_orphan as a basic primitive to decrement the count on the task_struct if we want a kthread to simply exit after it has done some work. And as a helper function we can have a kthread_run_orphan. Speaking about helpers, could we also add kthread_start(), which should be used instead of direct wake_up_process() ? Not that it is terribly important, but still. Note that kthread_create() pins the task_struct allows us to cleanup the code. Look at this ugly wait_to_die: label in migration_thread(). Is is because migration_thread() can't exit until CPU_DEAD reaps it. Other reasons were already solved by kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
