| Message ID | 20140817174624.GE3347@wotan.suse.de |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/17, Luis R. Rodriguez wrote: > > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Sun, 17 Aug 2014 20:21:38 +0200, Oleg Nesterov wrote: > > On 08/17, Luis R. Rodriguez wrote: > > > > 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. > > +/* > > + * 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. Yes, and it'd be better to mention not to mark initfn with __init prefix. (Meanwhile exitfn can be with __exit prefix.) thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/18, Takashi Iwai wrote: > > #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. Agreed. Oleg. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Mon, 18 Aug 2014 14:22:17 +0200, Oleg Nesterov wrote: > > On 08/18, Takashi Iwai wrote: > > > > #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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/18, Takashi Iwai wrote: > > At Mon, 18 Aug 2014 14:22:17 +0200, > Oleg Nesterov wrote: > > > > On 08/18, Takashi Iwai wrote: > > > > > > #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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 18, 2014 at 10:19 AM, Oleg Nesterov <oleg@redhat.com> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/device.h b/include/linux/device.h index 43d183a..dc7c0ba7 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -27,6 +27,7 @@ #include <linux/ratelimit.h> #include <linux/uidgid.h> #include <linux/gfp.h> +#include <linux/kthread.h> #include <asm/device.h> struct device; @@ -1227,4 +1228,72 @@ static void __exit __driver##_exit(void) \ } \ module_exit(__driver##_exit); +#ifndef MODULE + +#define module_long_probe_init(x) __initcall(x); +#define module_long_probe_exit(x) __exitcall(x); + +#else +/* + * Linux device drivers must strive to handle driver initialization + * within less than 30 seconds, 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 this macro can be used to help annotate + * the driver as needing work and prevent userspace init processes + * from killing drivers not loading within a specified timeout. + * + * module probe will return immediately and since we are not waiting + * for the kthread to end on init we won't be able to inform userspace + * of the result of the full init sequence. Probing should initialize + * the device driver, probing for devices should be handled asynchronously + * behind the scenes. + * + * Drivers that use this helper should be considered broken and in need + * of some serious love. + */ +#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); + +#endif /* MODULE */ + #endif /* _DEVICE_H_ */