From: Luis R. Rodriguez <hidden> Date: 2014-08-12 22:28:43
From: "Luis R. Rodriguez" <redacted>
Tetsuo bisected and found that commit 786235ee "kthread: make
kthread_create() killable" modified kthread_create() to bail as
soon as SIGKILL is received. This is causing some issues with
some drivers and at times boot. Joseph then found that failures
occur as the systemd-udevd process sends SIGKILL to modprobe if
probe on a driver takes over 30 seconds. When this happens probe
will fail on any driver, its why booting on some system will fail
if the driver happens to be a storage related driver. Some folks
have suggested fixing this by modifying kthread_create() to not
leave upon SIGKILL [3], upon review Oleg rejected this change and
the discussion was punted out to systemd to see if the default
timeout could be increased from 30 seconds to 120. The opinion of
the systemd maintainers is that the driver's behavior should
be fixed [4]. Linus seems to agree [5], however more recently even
networking drivers have been reported to fail on probe since just
writing the firmware to a device and kicking it can take easy over
60 seconds [6]. Benjamim was able to trace the issues recently
reported on cxgb4 down to the same systemd-udevd 30 second timeout [6].
This is an alternative solution which enables drivers that are
known to take long to use kthread_run(), this avoids the 30 second
timeout and lets us annotate drivers with long init sequences that
need some love.
[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
[1] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
[2] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
[3] http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
[4] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
[5] http://article.gmane.org/gmane.linux.kernel/1671333
[6] https://bugzilla.novell.com/show_bug.cgi?id=877622
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <redacted>
Cc: Kay Sievers <redacted>
Cc: One Thousand Gnomes <redacted>
Cc: Tim Gardner <redacted>
Cc: Pierre Fersing <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <redacted>
Cc: Praveen Krishnamoorthy <redacted>
Cc: Sreekanth Reddy <redacted>
Cc: Abhijit Mahajan <redacted>
Cc: Hariprasad S <redacted>
Cc: Santosh Rastapur <redacted>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
A few implementation notes:
1) Two wrappers are used to simply enable the same prototype
as expected on modules for module_init()
2) The new helpers are stuffed under kthread.h since including
kthread.h on init.h caused major issues which are not easy
to resolve, in fact even including kernel.h in init.h cases
some issues. We could have keep this under init.h if we ifef'd
on _LINUX_KTHREAD_H as well but this seems a bit cleaner.
include/linux/kthread.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
@@ -1,6 +1,7 @@#ifndef _LINUX_KTHREAD_H#define _LINUX_KTHREAD_H/* Simple interface for creating and stopping kernel threads without mess. */+#include<linux/init.h>#include<linux/err.h>#include<linux/sched.h>
@@ -128,4 +129,38 @@ bool queue_kthread_work(struct kthread_worker *worker,voidflush_kthread_work(structkthread_work*work);voidflush_kthread_worker(structkthread_worker*worker);+#ifndef MODULE++#define module_long_probe_init(x) __initcall(x);+#define module_long_probe_exit(x) __exitcall(x);++#else+/* To be used by modules which can take over 30 seconds at probe */+#define module_long_probe_init(initfn) \+staticstructtask_struct*__init_thread;\+staticint_long_probe_##initfn(void*arg)\+{\+returninitfn();\+}\+staticinline__initint__long_probe_##initfn(void)\+{\+__init_thread=kthread_run(_long_probe_##initfn,\+NULL,\+#initfn); \+if(IS_ERR(__init_thread))\+returnPTR_ERR(__init_thread);\+return0;\+}\+module_init(__long_probe_##initfn);+/* To be used by modules that require module_long_probe_init() */+#define module_long_probe_exit(exitfn) \+staticinlinevoid__long_probe_##exitfn(void)\+{\+exitfn();\+if(__init_thread)\+kthread_stop(__init_thread);\+}\+module_exit(__long_probe_##exitfn);+#endif /* MODULE */+#endif /* _LINUX_KTHREAD_H */
Tetsuo bisected and found that commit 786235ee \"kthread: make
kthread_create() killable\" modified kthread_create() to bail as
soon as SIGKILL is received.
I just wrote commit 786235ee. It is not Tetsuo who bisected it.
void flush_kthread_work(struct kthread_work *work);
void flush_kthread_worker(struct kthread_worker *worker);
+#ifndef MODULE
+
+#define module_long_probe_init(x) __initcall(x);
+#define module_long_probe_exit(x) __exitcall(x);
+
+#else
+/* To be used by modules which can take over 30 seconds at probe */
+#define module_long_probe_init(initfn) \\
+ static struct task_struct *__init_thread; \\
+ static int _long_probe_##initfn(void *arg) \\
+ { \\
+ return initfn(); \\
+ } \\
+ static inline __init int __long_probe_##initfn(void) \\
+ { \\
+ __init_thread = kthread_run(_long_probe_##initfn,\\
+ NULL, \\
+ #initfn); \\
+ if (IS_ERR(__init_thread)) \\
+ return PTR_ERR(__init_thread); \\
+ return 0; \\
+ } \\
+ module_init(__long_probe_##initfn);
+/* To be used by modules that require module_long_probe_init() */
+#define module_long_probe_exit(exitfn) \\
+ static inline void __long_probe_##exitfn(void) \\
+ { \\
+ exitfn(); \\
exitfn() must not be called if initfn() failed or has not
completed yet. You need a bool variable for indicating that
we are ready to call exitfn().
Also, subsequent userspace operations may fail if
we return to userspace before initfn() completes
(e.g. device nodes are not created yet).
+ if (__init_thread) \\
+ kthread_stop(__init_thread); \\
We can\'t use kthread_stop() here because we have to
wait for initfn() to succeed before exitfn() is called.
On Wed, Aug 13, 2014 at 07:59:06AM +0900, Tetsuo Handa wrote:
Luis R. Rodriguez wrote:
quoted
Tetsuo bisected and found that commit 786235ee \"kthread: make
kthread_create() killable\" modified kthread_create() to bail as
soon as SIGKILL is received.
I just wrote commit 786235ee. It is not Tetsuo who bisected it.
void flush_kthread_work(struct kthread_work *work);
void flush_kthread_worker(struct kthread_worker *worker);
+#ifndef MODULE
+
+#define module_long_probe_init(x) __initcall(x);
+#define module_long_probe_exit(x) __exitcall(x);
+
+#else
+/* To be used by modules which can take over 30 seconds at probe */
+#define module_long_probe_init(initfn) \\
+ static struct task_struct *__init_thread; \\
+ static int _long_probe_##initfn(void *arg) \\
+ { \\
+ return initfn(); \\
+ } \\
+ static inline __init int __long_probe_##initfn(void) \\
+ { \\
+ __init_thread = kthread_run(_long_probe_##initfn,\\
+ NULL, \\
+ #initfn); \\
+ if (IS_ERR(__init_thread)) \\
+ return PTR_ERR(__init_thread); \\
+ return 0; \\
+ } \\
+ module_init(__long_probe_##initfn);
+/* To be used by modules that require module_long_probe_init() */
+#define module_long_probe_exit(exitfn) \\
+ static inline void __long_probe_##exitfn(void) \\
+ { \\
+ exitfn(); \\
exitfn() must not be called if initfn() failed or has not
completed yet. You need a bool variable for indicating that
we are ready to call exitfn().
Also, subsequent userspace operations may fail if
we return to userspace before initfn() completes
(e.g. device nodes are not created yet).
I doubt that this will be a problem, as device nodes are usually created
_after_ module_init() returns.
But the cleanup issues are real on error paths. Given that these
drivers will need "work" anyway, I don't think it's really a big deal.
thanks,
greg k-h
exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do
int err = kthread_stop(__init_thread);
if (!err)
exitfn();
But there is an additional complication, you can't use __init_thread
without get_task_struct(), so __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
Oleg.
exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do
int err = kthread_stop(__init_thread);
if (!err)
exitfn();
Thanks! With the check for __init_thread as well as it can be
ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
reason).
But there is an additional complication, you can't use __init_thread
without get_task_struct(),
Can you elaborate why ? kthread_stop() uses get_task_struct(),
wake_up_process() and finally put_task_struct(), and we're the
only user of this thread. Also kthread_run() ensures wake_up_process()
gets called on startup, so not sure where the race would be provided
all users here and with the respective helpers on buggy drivers.
so __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
I fail to see why we'd need to add get_task_struct() on
module_long_probe_init(), can you clarify?
Luis
exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do
int err = kthread_stop(__init_thread);
if (!err)
exitfn();
Thanks! With the check for __init_thread as well as it can be
ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
reason).
Do you mean __long_probe_##exitfn() should also check ERR_PTR(__init_thread)?
I don't think so. If kthread_run() above fails, module_init() should return
the error (it does), so module_exit() won't be called.
quoted
But there is an additional complication, you can't use __init_thread
without get_task_struct(),
Can you elaborate why ? kthread_stop() uses get_task_struct(),
This is too late. This task_struct can be already freed/reused. See below.
wake_up_process() and finally put_task_struct(), and we're the
only user of this thread. Also kthread_run() ensures wake_up_process()
gets called on startup, so not sure where the race would be provided
all users here and with the respective helpers on buggy drivers.
quoted
so __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
I fail to see why we'd need to add get_task_struct() on
module_long_probe_init(), can you clarify?
kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
on its own, without checking kthread_should_stop(). And btw that is why
kthread_stop() does get_task_struct()).
If callback() can exit (if it calls do_exit() or simply returns), then nothing
protects this task_struct, it will be freed.
Oleg.
exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do
int err = kthread_stop(__init_thread);
if (!err)
exitfn();
Thanks! With the check for __init_thread as well as it can be
ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
reason).
Do you mean __long_probe_##exitfn() should also check ERR_PTR(__init_thread)?
I don't think so. If kthread_run() above fails, module_init() should return
the error (it does), so module_exit() won't be called.
Good point.
quoted
quoted
But there is an additional complication, you can't use __init_thread
without get_task_struct(),
Can you elaborate why ? kthread_stop() uses get_task_struct(),
This is too late. This task_struct can be already freed/reused. See below.
quoted
wake_up_process() and finally put_task_struct(), and we're the
only user of this thread. Also kthread_run() ensures wake_up_process()
gets called on startup, so not sure where the race would be provided
all users here and with the respective helpers on buggy drivers.
quoted
so __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
I fail to see why we'd need to add get_task_struct() on
module_long_probe_init(), can you clarify?
kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
on its own, without checking kthread_should_stop(). And btw that is why
kthread_stop() does get_task_struct()).
If callback() can exit (if it calls do_exit() or simply returns), then nothing
protects this task_struct, it will be freed.
OK thanks, yeah I see the issue now, and I was able to create a null
pointer dereference by simply calling schedule() quite a bit, will
roll in the required fixes, but come to think of it if there are
other uses (I haven't SmPLd grep'd for grammar uses yet) perhaps
generic helpers would be good? kthread_run_alloc() kthread_run_free().
Luis
exitfn() should be called after kthread_stop(), and only if initfn()
returns 0. So it should probably do
int err = kthread_stop(__init_thread);
if (!err)
exitfn();
Thanks! With the check for __init_thread as well as it can be
ERR_PTR(-ENOMEM), ERR_PTR(-EINTR), or NULL (for whatever other
reason).
Do you mean __long_probe_##exitfn() should also check ERR_PTR(__init_thread)?
I don't think so. If kthread_run() above fails, module_init() should return
the error (it does), so module_exit() won't be called.
Good point.
quoted
quoted
quoted
But there is an additional complication, you can't use __init_thread
without get_task_struct(),
Can you elaborate why ? kthread_stop() uses get_task_struct(),
This is too late. This task_struct can be already freed/reused. See below.
quoted
wake_up_process() and finally put_task_struct(), and we're the
only user of this thread. Also kthread_run() ensures wake_up_process()
gets called on startup, so not sure where the race would be provided
all users here and with the respective helpers on buggy drivers.
quoted
so __long_probe_##initfn() can't use
kthread_run(). It needs kthread_create() + get_task_struct() + wakeup.
I fail to see why we'd need to add get_task_struct() on
module_long_probe_init(), can you clarify?
kthread_stop(kthread_run(callback)) is only safe if callback() can not exit
on its own, without checking kthread_should_stop(). And btw that is why
kthread_stop() does get_task_struct()).
If callback() can exit (if it calls do_exit() or simply returns), then nothing
protects this task_struct, it will be freed.
OK thanks, yeah I see the issue now, and I was able to create a null
pointer dereference by simply calling schedule() quite a bit, will
roll in the required fixes, but come to think of it if there are
other uses (I haven't SmPLd grep'd for grammar uses yet) perhaps
generic helpers would be good? kthread_run_alloc() kthread_run_free().
How about just increasing/decreasing the module count for blocking the
exit call? For example:
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
int ret = initfn(); \
module_put(THIS_MODULE); \
return ret; \
} \
static inline __init int __long_probe_##initfn(void) \
{ \
struct task_struct *__init_thread; \
__module_get(THIS_MODULE); \
__init_thread = kthread_run(_long_probe_##initfn,\
NULL, \
#initfn); \
if (IS_ERR(__init_thread)) { \
module_put(THIS_MODULE); \
return PTR_ERR(__init_thread); \
} \
return 0; \
} \
module_init(__long_probe_##initfn);
/* To be used by modules that require module_long_probe_init() */
#define module_long_probe_exit(exitfn) \
module_exit(exitfn);
Takashi
How about just increasing/decreasing the module count for blocking the
exit call? For example:
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
int ret = initfn(); \
module_put(THIS_MODULE); \
I leave this to you and Luis, but personally I think this is very
nice idea, I like it. Because sys_delete_module() won't hang in D
state waiting for initfn().
There is a small problem. This module can be unloaded right after
module_put() above. In this case its memory can be unmapped and
the exiting thread can crash.
This is very unlikely, this thread needs to execute just a few insn
and escape from this module's memory. Given that only the buggy
modules should use this hack, perhaps we can even ignore this race.
But perhaps it makes sense to close this race anyway, and we already
have complete_and_exit() which can be used instead of "return ret"
above. Just we need the additional "static struct completion" and
module_exit() should call wait_for_completion.
Oleg.
How about just increasing/decreasing the module count for blocking the
exit call? For example:
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
int ret = initfn(); \
module_put(THIS_MODULE); \
I leave this to you and Luis, but personally I think this is very
nice idea, I like it. Because sys_delete_module() won't hang in D
state waiting for initfn().
There is a small problem. This module can be unloaded right after
module_put() above. In this case its memory can be unmapped and
the exiting thread can crash.
This is very unlikely, this thread needs to execute just a few insn
and escape from this module's memory. Given that only the buggy
modules should use this hack, perhaps we can even ignore this race.
But perhaps it makes sense to close this race anyway, and we already
have complete_and_exit() which can be used instead of "return ret"
above. Just we need the additional "static struct completion" and
module_exit() should call wait_for_completion.
Forgot to mention... and __long_probe_##initfn() could be simpler
without kthread_run,
__init_thread = kthread_create(...);
if (IS_ERR(__init_thread))
return PTR_ERR();
module_get(THIS_MODULE);
wake_up_process(__init_thread);
return 0;
but this is subjective, up to you.
Oleg.
Damn, sorry for noise ;)
I was going to suggest to introduce module_put_and_exit() to simplify
this and potentially other users, but it already exists. So this code
can use it too without additional complications.
On 08/17, Oleg Nesterov wrote:
On 08/17, Oleg Nesterov wrote:
quoted
On 08/17, Takashi Iwai wrote:
quoted
How about just increasing/decreasing the module count for blocking the
exit call? For example:
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
int ret = initfn(); \
module_put(THIS_MODULE); \
I leave this to you and Luis, but personally I think this is very
nice idea, I like it. Because sys_delete_module() won't hang in D
state waiting for initfn().
There is a small problem. This module can be unloaded right after
module_put() above. In this case its memory can be unmapped and
the exiting thread can crash.
This is very unlikely, this thread needs to execute just a few insn
and escape from this module's memory. Given that only the buggy
modules should use this hack, perhaps we can even ignore this race.
But perhaps it makes sense to close this race anyway, and we already
have complete_and_exit() which can be used instead of "return ret"
above. Just we need the additional "static struct completion" and
module_exit() should call wait_for_completion.
Forgot to mention... and __long_probe_##initfn() could be simpler
without kthread_run,
__init_thread = kthread_create(...);
if (IS_ERR(__init_thread))
return PTR_ERR();
module_get(THIS_MODULE);
wake_up_process(__init_thread);
return 0;
but this is subjective, up to you.
Oleg.
From: Luis R. Rodriguez <hidden> Date: 2014-08-17 17:46:32
On Sun, Aug 17, 2014 at 02:55:05PM +0200, Oleg Nesterov wrote:
Damn, sorry for noise ;)
I was going to suggest to introduce module_put_and_exit() to simplify
this and potentially other users, but it already exists. So this code
can use it too without additional complications.
In the last iteration that I have stress tested for corner cases I just
get_task_struct() on the init and then put_task_struct() at the exit, is that
fine too or are there reasons to prefer the module stuff?
Note that technically the issue is not that init is taking long its probe that
takes long but since the driver core runs it immediately after init on buses
that autoprobe probe is also then collaterally of concern, but see the other
reply for that.
Moving this to device.h worked better as you don't have to add extra
lines on drivers to include kthread.h.
In the last iteration that I have stress tested for corner cases I just
get_task_struct() on the init and then put_task_struct() at the exit, is that
fine too or are there reasons to prefer the module stuff?
I am fine either way.
I like the Takashi's idea because if sys_delete_module() is called before
initfn() completes it will return -EBUSY and not hang in TASK_UNINTERRUPTIBLE
state. But this is not necessarily good, so I leave this to you and Takashi.
+/*
+ * Linux device drivers must strive to handle driver initialization
+ * within less than 30 seconds,
Well, perhaps the comment should name the reason ;)
if device probing takes longer
+ * for whatever reason asynchronous probing of devices / loading
+ * firmware should be used. If a driver takes longer than 30 second
+ * on the initialization path
Or if the initialization code can't handle the errors properly (say,
mptsas can't handle the errors caused by SIGKILL).
+ * Drivers that use this helper should be considered broken and in need
+ * of some serious love.
+ */
Yes.
+#define module_long_probe_init(initfn) \
+ static struct task_struct *__init_thread; \
+ static int _long_probe_##initfn(void *arg) \
+ { \
+ return initfn(); \
+ } \
+ static inline __init int __long_probe_##initfn(void) \
+ { \
+ __init_thread = kthread_create(_long_probe_##initfn,\
+ NULL, \
+ #initfn); \
+ if (IS_ERR(__init_thread)) \
+ return PTR_ERR(__init_thread); \
+ /* \
+ * callback won't check kthread_should_stop() \
+ * before bailing, so we need to protect it \
+ * before running it. \
+ */ \
+ get_task_struct(__init_thread); \
+ wake_up_process(__init_thread); \
+ return 0; \
+ } \
+ module_init(__long_probe_##initfn);
+
+/* To be used by modules that require module_long_probe_init() */
+#define module_long_probe_exit(exitfn) \
+ static inline void __long_probe_##exitfn(void) \
+ { \
+ int err; \
+ /* \
+ * exitfn() will not be run if the driver's \
+ * real probe which is run on the kthread \
+ * failed for whatever reason, this will \
+ * wait for it to end. \
+ */ \
+ err = kthread_stop(__init_thread); \
+ if (!err) \
+ exitfn(); \
+ put_task_struct(__init_thread); \
+ } \
+ module_exit(__long_probe_##exitfn);
Both inline's look misleading, gcc will generate the code out-of-line
anyway. But this is cosmetic. And for cosmetic reasons, since the 1st
macro uses __init, the 2nd one should probably use __exit.
I believe this version is correct.
Oleg.
At Sun, 17 Aug 2014 20:21:38 +0200,
Oleg Nesterov wrote:
On 08/17, Luis R. Rodriguez wrote:
quoted
In the last iteration that I have stress tested for corner cases I just
get_task_struct() on the init and then put_task_struct() at the exit, is that
fine too or are there reasons to prefer the module stuff?
I am fine either way.
I like the Takashi's idea because if sys_delete_module() is called before
initfn() completes it will return -EBUSY and not hang in TASK_UNINTERRUPTIBLE
state. But this is not necessarily good, so I leave this to you and Takashi.
Another merit of fiddling with module count is that the thread object
isn't referred in other than module_init. That is, we'd need only
module_init() implementation like below (thanks to Oleg's advice):
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
module_put_and_exit(initfn()); \
return 0; \
} \
static int __init __long_probe_##initfn(void) \
{ \
struct task_struct *__init_thread = \
kthread_create(_long_probe_##initfn, \
NULL, #initfn); \
if (IS_ERR(__init_thread)) \
return PTR_ERR(__init_thread); \
__module_get(THIS_MODULE); \
wake_up_process(__init_thread); \
return 0; \
} \
module_init(__long_probe_##initfn)
... and module_exit() remains identical as the normal version.
But, it's really a small difference, and I don't mind much which way
to take, too.
quoted
+/*
+ * Linux device drivers must strive to handle driver initialization
+ * within less than 30 seconds,
Well, perhaps the comment should name the reason ;)
quoted
if device probing takes longer
+ * for whatever reason asynchronous probing of devices / loading
+ * firmware should be used. If a driver takes longer than 30 second
+ * on the initialization path
Or if the initialization code can't handle the errors properly (say,
mptsas can't handle the errors caused by SIGKILL).
quoted
+ * Drivers that use this helper should be considered broken and in need
+ * of some serious love.
+ */
Yes.
quoted
+#define module_long_probe_init(initfn) \
+ static struct task_struct *__init_thread; \
+ static int _long_probe_##initfn(void *arg) \
+ { \
+ return initfn(); \
+ } \
+ static inline __init int __long_probe_##initfn(void) \
+ { \
+ __init_thread = kthread_create(_long_probe_##initfn,\
+ NULL, \
+ #initfn); \
+ if (IS_ERR(__init_thread)) \
+ return PTR_ERR(__init_thread); \
+ /* \
+ * callback won't check kthread_should_stop() \
+ * before bailing, so we need to protect it \
+ * before running it. \
+ */ \
+ get_task_struct(__init_thread); \
+ wake_up_process(__init_thread); \
+ return 0; \
+ } \
+ module_init(__long_probe_##initfn);
+
+/* To be used by modules that require module_long_probe_init() */
+#define module_long_probe_exit(exitfn) \
+ static inline void __long_probe_##exitfn(void) \
+ { \
+ int err; \
+ /* \
+ * exitfn() will not be run if the driver's \
+ * real probe which is run on the kthread \
+ * failed for whatever reason, this will \
+ * wait for it to end. \
+ */ \
+ err = kthread_stop(__init_thread); \
+ if (!err) \
+ exitfn(); \
+ put_task_struct(__init_thread); \
+ } \
+ module_exit(__long_probe_##exitfn);
Both inline's look misleading, gcc will generate the code out-of-line
anyway. But this is cosmetic. And for cosmetic reasons, since the 1st
macro uses __init, the 2nd one should probably use __exit.
Yes, and it'd be better to mention not to mark initfn with __init
prefix. (Meanwhile exitfn can be with __exit prefix.)
thanks,
Takashi
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
module_put_and_exit(initfn()); \
return 0; \
} \
static int __init __long_probe_##initfn(void) \
{ \
struct task_struct *__init_thread = \
kthread_create(_long_probe_##initfn, \
NULL, #initfn); \
if (IS_ERR(__init_thread)) \
return PTR_ERR(__init_thread); \
__module_get(THIS_MODULE); \
wake_up_process(__init_thread); \
return 0; \
} \
module_init(__long_probe_##initfn)
... and module_exit() remains identical as the normal version.
Aaaah. This is not true, module_exit() should not call exitfn() if initfn()
fails... So _long_probe_##initfn() needs to save the error code which should
be checked by module_exit().
But, it's really a small difference, and I don't mind much which way
to take, too.
At Mon, 18 Aug 2014 14:22:17 +0200,
Oleg Nesterov wrote:
On 08/18, Takashi Iwai wrote:
quoted
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
module_put_and_exit(initfn()); \
return 0; \
} \
static int __init __long_probe_##initfn(void) \
{ \
struct task_struct *__init_thread = \
kthread_create(_long_probe_##initfn, \
NULL, #initfn); \
if (IS_ERR(__init_thread)) \
return PTR_ERR(__init_thread); \
__module_get(THIS_MODULE); \
wake_up_process(__init_thread); \
return 0; \
} \
module_init(__long_probe_##initfn)
... and module_exit() remains identical as the normal version.
Aaaah. This is not true, module_exit() should not call exitfn() if initfn()
fails... So _long_probe_##initfn() needs to save the error code which should
be checked by module_exit().
Oh, right. So we need a reference in the module exit path in anyway,
and Luis' version might be shorter in the end.
thanks,
Takashi
At Mon, 18 Aug 2014 14:22:17 +0200,
Oleg Nesterov wrote:
quoted
On 08/18, Takashi Iwai wrote:
quoted
#define module_long_probe_init(initfn) \
static int _long_probe_##initfn(void *arg) \
{ \
module_put_and_exit(initfn()); \
return 0; \
} \
static int __init __long_probe_##initfn(void) \
{ \
struct task_struct *__init_thread = \
kthread_create(_long_probe_##initfn, \
NULL, #initfn); \
if (IS_ERR(__init_thread)) \
return PTR_ERR(__init_thread); \
__module_get(THIS_MODULE); \
wake_up_process(__init_thread); \
return 0; \
} \
module_init(__long_probe_##initfn)
... and module_exit() remains identical as the normal version.
Aaaah. This is not true, module_exit() should not call exitfn() if initfn()
fails... So _long_probe_##initfn() needs to save the error code which should
be checked by module_exit().
Oh, right. So we need a reference in the module exit path in anyway,
We only need to save the error code,
static int _long_probe_retval;
static int _long_probe_##initfn(void *arg)
{
_long_probe_retval = initfn();
module_put_and_exit(0); /* noreturn */
}
static void __long_probe_##exitfn(void)
{
if (!_long_probe_retval)
exitfn();
}
and Luis' version might be shorter in the end.
I dont't think that "shorter" does matter in this case. The real difference
is sys_delete_module() behaviour if it is called before initfn() completes.
And, again, I do not really know which version is better.
Oleg.
From: Luis R. Rodriguez <hidden> Date: 2014-08-19 04:11:35
On Mon, Aug 18, 2014 at 10:19 AM, Oleg Nesterov [off-list ref] wrote:
And, again, I do not really know which version is better.
In Chicago right now -- feedback was it seems the that generally
splitting up probe from init might be good in the end, if we do this
we won't need a work around for drivers that wait until our
grandmothers die on probe, but we certainly will then be penalizing
drivers who's init does take over 30 seconds. I'm waiting to see an
alternative version of the solution provided as an example on the
other thread, maybe it will fix my keyboard issue :)
Luis