[PATCH v3 0/3] module loading: add module_long_probe_init()

[PATCH v3 0/3] module loading: add module_long_probe_init()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

systemd-udevd will cause a driver to fail to load if it doesn't load
within the set default timeout, right now 30 seconds. Although we now
have a way to increase the timeout at run time we need a way to both
annotate drivers that need fixing and a way to enable drivers's whose
probe takes long time while they are fixed. Using deferred probe works
but it was recently agreed that we'd instead use kthreads for drivers
that have long probes.

Luis R. Rodriguez (3):
  init / kthread: add module_long_probe_init() and
    module_long_probe_exit()
  cxgb4: use module_long_probe_init()
  mptsas: use module_long_probe_init()

 drivers/message/fusion/mptsas.c                 |  5 ++--
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c |  5 ++--
 include/linux/kthread.h                         | 35 +++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 4 deletions(-)

-- 
2.0.3

[PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <joseph.salisbury@canonical.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
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 <mcgrof@suse.com>
---

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(+)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 13d5520..2b5555a 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -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,
 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();					\
+		if (__init_thread)				\
+			kthread_stop(__init_thread);		\
+	}							\
+	module_exit(__long_probe_##exitfn);
+#endif /* MODULE */
+
 #endif /* _LINUX_KTHREAD_H */
-- 
2.0.3

[PATCH v3 2/3] cxgb4: use module_long_probe_init()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

cxgb4 probe can take up to over 1 minute when the firmware is
is written and installed on the device, even after this the device
driver still does some device probing and can take quite a bit.
This driver needs fixing but right now it simply wont' work on
some systems. Use the new module_long_probe_init() to annotate
this driver's probe is broken and require some love, but makes
the driver operational until that is fixed.

Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
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 <mcgrof@suse.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 36ebbda..5d8231d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -34,6 +34,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/kthread.h>
 #include <linux/bitmap.h>
 #include <linux/crc32.h>
 #include <linux/ctype.h>
@@ -6815,5 +6816,5 @@ static void __exit cxgb4_cleanup_module(void)
 	destroy_workqueue(workq);
 }
 
-module_init(cxgb4_init_module);
-module_exit(cxgb4_cleanup_module);
+module_long_probe_init(cxgb4_init_module);
+module_long_probe_exit(cxgb4_cleanup_module);
-- 
2.0.3

[PATCH v3 3/3] mptsas: use module_long_probe_init()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Its reported that mptsas can at times take over 30 seconds
to recognize SCSI storage devices [0], this is done on the
driver's probe path. Use the the new module_long_probe_init()
to annotate this driver's probe is broken and require some love,
but makes the driver operational until that is fixed.

[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705

Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
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 <mcgrof@suse.com>
---
 drivers/message/fusion/mptsas.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 0707fa2..c2bb72b 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -43,6 +43,7 @@
 */
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -5439,5 +5440,5 @@ mptsas_exit(void)
 	mpt_deregister(mptsasDeviceResetCtx);
 }
 
-module_init(mptsas_init);
-module_exit(mptsas_exit);
+module_long_probe_init(mptsas_init);
+module_long_probe_exit(mptsas_exit);
-- 
2.0.3

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Tetsuo Handa]

Luis R. Rodriguez wrote:
> 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.

> @@ -128,4 +129,38 @@ bool queue_kthread_work(struct kthread_worker *worker,
> 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.

> +     }                                   \\
> +     module_exit(__long_probe_##exitfn);
> +#endif /* MODULE */
> +
> #endif /* _LINUX_KTHREAD_H */

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Greg KH]

On Wed, Aug 13, 2014 at 07:59:06AM +0900, Tetsuo Handa wrote:
> Luis R. Rodriguez wrote:
> > 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.
> 
> > @@ -128,4 +129,38 @@ bool queue_kthread_work(struct kthread_worker *worker,
> > 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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

On 08/12, Luis R. Rodriguez wrote:
>
> +/* To be used by modules which can take over 30 seconds at probe */

Probably the comment should explain that this hack should only be
used if the driver is buggy and is wating for "real fix".

> +#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();					\
> +		if (__init_thread)				\
> +			kthread_stop(__init_thread);		\
> +	}							\

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.

RE: [PATCH v3 2/3] cxgb4: use module_long_probe_init()

[Anish Bhatt]

Adding Casey who's actually incharge of this code and missing from the CC list
-Anish

Re: [PATCH v3 2/3] cxgb4: use module_long_probe_init()

[Casey Leedom]

On 08/13/2014 04:33 PM, Anish Bhatt wrote:
> Adding Casey who's actually incharge of this code and missing from the CC list

   Thanks Anish!

   As I mentioned to Anish, there are fundamentally two problems here in 
the time being consumed by the cxgb4 PCI probe() function:

  1. When various firmware files aren't present, request_firmware()
     can take a very long time.  This is easily solved by using
     request_firmware_direct() and I certainly have no objection to that.

  2. When there are multiple adapters present in a system which
     need firmware downloaded, each one individually may not take
     a ton of time but together they can exceed simple Module Load
     Timeouts.  There's not a simple answer here.

   Part of the problem here is that it's a Module Load Timeout instead 
of a per-device Probe Timeout.   Part of the problem is that the current 
architecture has Device Probe happening out of the Module Initialization 
when we call pci_register_driver() with our PCI Device ID Table.

   Running the Device Probes asynchronously has been discussed but that 
has the problem that it's then impossible to return the Device Probe 
Status.  This is a problem for Driver Fallback and, if the probe fails, 
we're not supposed to call the Device Remove function. To make this 
work, the synchronous/asynchronous boundary would really need to be up 
in the PCI Infrastructure layer so the Device Probe status could be 
captured in the normal logic.  This would be a moderately large change 
there ...

   Deferring the Device Initialization till the first "ifup" has also 
been discussed and is certainly possible, though a moderately large 
architectural change to every driver which needs it.  It also has the 
unfortunate effect of introducing random large delays directly on user 
commands.  From a User Experience perspective I would tend to want such 
large delays in the Device Probe.  But that's something that really 
deserves a real User Interaction study rather than throwing a dart.

   On the whole, I think that introducing these Module Load Timeouts 
hasn't been well thought out with respect to the repercussions and I'd 
be more inclined to back that out till a well thought out design is 
developed.  But I'm here for the discussion.

Casey

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Luis R. Rodriguez]

On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> On 08/12, Luis R. Rodriguez wrote:
> >
> > +/* To be used by modules which can take over 30 seconds at probe */
> 
> Probably the comment should explain that this hack should only be
> used if the driver is buggy and is wating for "real fix".
> 
> > +#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();					\
> > +		if (__init_thread)				\
> > +			kthread_stop(__init_thread);		\
> > +	}							\
> 
> 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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

On 08/15, Luis R. Rodriguez wrote:
>
> On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> > On 08/12, Luis R. Rodriguez wrote:
> > >
> > > +/* To be used by modules which can take over 30 seconds at probe */
> >
> > Probably the comment should explain that this hack should only be
> > used if the driver is buggy and is wating for "real fix".
> >
> > > +#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();					\
> > > +		if (__init_thread)				\
> > > +			kthread_stop(__init_thread);		\
> > > +	}							\
> >
> > 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.

> > 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.
>
> > 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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Luis R. Rodriguez]

On Fri, Aug 15, 2014 at 04:39:02PM +0200, Oleg Nesterov wrote:
> On 08/15, Luis R. Rodriguez wrote:
> >
> > On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> > > On 08/12, Luis R. Rodriguez wrote:
> > > >
> > > > +/* To be used by modules which can take over 30 seconds at probe */
> > >
> > > Probably the comment should explain that this hack should only be
> > > used if the driver is buggy and is wating for "real fix".
> > >
> > > > +#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();					\
> > > > +		if (__init_thread)				\
> > > > +			kthread_stop(__init_thread);		\
> > > > +	}							\
> > >
> > > 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.

> > > 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.
> >
> > > 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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Takashi Iwai]

At Sat, 16 Aug 2014 04:50:07 +0200,
Luis R. Rodriguez wrote:
> 
> On Fri, Aug 15, 2014 at 04:39:02PM +0200, Oleg Nesterov wrote:
> > On 08/15, Luis R. Rodriguez wrote:
> > >
> > > On Wed, Aug 13, 2014 at 07:51:01PM +0200, Oleg Nesterov wrote:
> > > > On 08/12, Luis R. Rodriguez wrote:
> > > > >
> > > > > +/* To be used by modules which can take over 30 seconds at probe */
> > > >
> > > > Probably the comment should explain that this hack should only be
> > > > used if the driver is buggy and is wating for "real fix".
> > > >
> > > > > +#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();					\
> > > > > +		if (__init_thread)				\
> > > > > +			kthread_stop(__init_thread);		\
> > > > > +	}							\
> > > >
> > > > 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.
> 
> > > > 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.
> > >
> > > > 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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

On 08/17, Takashi Iwai wrote:
>
> 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);			\

WINDOW, please see below.

> 		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;					\
> 	}							\

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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

On 08/17, Oleg Nesterov wrote:
>
> On 08/17, Takashi Iwai wrote:
> >
> > 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);			\
>
> WINDOW, please see below.
>
> > 		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;					\
> > 	}							\
>
> 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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

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:
> >
> > On 08/17, Takashi Iwai wrote:
> > >
> > > 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);			\
> >
> > WINDOW, please see below.
> >
> > > 		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;					\
> > > 	}							\
> >
> > 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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Luis R. Rodriguez]

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.

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_ */

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Takashi Iwai]

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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Takashi Iwai]

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

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Oleg Nesterov]

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.

Re: [PATCH v3 1/3] init / kthread: add module_long_probe_init() and module_long_probe_exit()

[Luis R. Rodriguez]

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