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 linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
