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

[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 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 2/3] cxgb4: use module_long_probe_init()

[Luis R. Rodriguez]

On Thu, Aug 14, 2014 at 09:42:49AM -0700, Casey Leedom wrote:
>
> 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.

I sent a patch for this a while ago, since there is no objection if
you'd like to apply the patch:

http://patchwork.ozlabs.org/patch/363756/

Apart from that you also want to use asynch firmware loading but
to use that properly (I also had sent some basic initial patches
for asynch firmware loading but without moving out other logic
yet) you want to also let driver initalization complete
asynchronously later.

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

I had originally recommended to write your own platform driver for
this and have each port probe but Greg has provided the last advice
for this on the patch I sent to add deferred probe support from
init, his recommendation was for you to write your own bus code for
the driver:

http://www.spinics.net/lists/linux-scsi/msg76695.html

>   Part of the problem here is that it's a Module Load Timeout instead of a 
> per-device Probe Timeout.

Seems like you can fix this with a bus driver.

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

Some maintainers consider most of the work to get what you need done
simple, I've tried to explain it ain't so, so glad you provided a bit
of details here. To be clear its not just about asynch firmware loading,
you need a bit more work. Can you evaluate using a bus driver?

>   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

You should just use asynch firmware loading there and only once your
driver is done loading firmware start exposing the device(s) as you
see fit with your bus driver.

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

The way that the 30 second timeout was introduced as a new driver
initialization requirement was certainly not ideal specially since
the respective systemd patch that intended to trigger the SIGKILL on
kmod module loading only took effect once kernel commit 786235ee
went in about a year later, and since the original systemd commit
was only addressing asynchronous firmware loading as a possible
issue that drivers may need to fix. The cxgb4 driver is a good
example that needs quite a bit of more work. Regardless systemd
folks are right -- but again, having this be introduced as a new
requirement that otherwise simply kills drivers seems a bit too
aggressive specially if its killing boot on some systems due to
delays on storage drivers. What's done is done -- and we need to
move on. We already reviewed twice now reverting 786235ee and that
won't happen, as a compromise we're looking for an easy agreeable
general driver work around that would both circumvent the issue
and let us easily grep for broken drivers. The deferred probe trick
was the first approach and this series addresses the more agreeable
solution. This 2 line patch then is what we are looking as work
around until your driver gets properly fixed.

Apart from these kernel changes there are systemd changes we've
looked at modifying, Hannes' patch 9719859c07a, now merged upstream on
systemd lets you override the timeout value through the kernel command
line. This will only help for all systems if you use a high enough
large timeout value, or on a case by case basis for each system.
I recently proposed replacing a kill for a warn only for udev
kmod built in commands, that's unacceptable for systemd's architecture
though so the last thing I proposed instead to use *for now* is a
multiplier for each different type of udev built-in command and
for kmod we'd use a high enough value, the timeout therefore would
be really large for module loading for now, but we'd still want to
collect logs of drivers taking long to probe. That's still being
discussed [0] but my hope is that with this series and that other
systemd discussion we'll have covered both areas affected and have
a good strategy to move forward with this new driver requirement.

[0] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/21689

  Luis

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

[Luis R. Rodriguez]

On Thu, Aug 14, 2014 at 09:42:49AM -0700, Casey Leedom wrote:
>
> 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.

I sent a patch for this a while ago, since there is no objection if
you'd like to apply the patch:

http://patchwork.ozlabs.org/patch/363756/

Apart from that you also want to use asynch firmware loading but
to use that properly (I also had sent some basic initial patches
for asynch firmware loading but without moving out other logic
yet) you want to also let driver initalization complete
asynchronously later.

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

I had originally recommended to write your own platform driver for
this and have each port probe but Greg has provided the last advice
for this on the patch I sent to add deferred probe support from
init, his recommendation was for you to write your own bus code for
the driver:

http://www.spinics.net/lists/linux-scsi/msg76695.html

>   Part of the problem here is that it's a Module Load Timeout instead of a 
> per-device Probe Timeout.

Seems like you can fix this with a bus driver.

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

Some maintainers consider most of the work to get what you need done
simple, I've tried to explain it ain't so, so glad you provided a bit
of details here. To be clear its not just about asynch firmware loading,
you need a bit more work. Can you evaluate using a bus driver?

>   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

You should just use asynch firmware loading there and only once your
driver is done loading firmware start exposing the device(s) as you
see fit with your bus driver.

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

The way that the 30 second timeout was introduced as a new driver
initialization requirement was certainly not ideal specially since
the respective systemd patch that intended to trigger the SIGKILL on
kmod module loading only took effect once kernel commit 786235ee
went in about a year later, and since the original systemd commit
was only addressing asynchronous firmware loading as a possible
issue that drivers may need to fix. The cxgb4 driver is a good
example that needs quite a bit of more work. Regardless systemd
folks are right -- but again, having this be introduced as a new
requirement that otherwise simply kills drivers seems a bit too
aggressive specially if its killing boot on some systems due to
delays on storage drivers. What's done is done -- and we need to
move on. We already reviewed twice now reverting 786235ee and that
won't happen, as a compromise we're looking for an easy agreeable
general driver work around that would both circumvent the issue
and let us easily grep for broken drivers. The deferred probe trick
was the first approach and this series addresses the more agreeable
solution. This 2 line patch then is what we are looking as work
around until your driver gets properly fixed.

Apart from these kernel changes there are systemd changes we've
looked at modifying, Hannes' patch 9719859c07a, now merged upstream on
systemd lets you override the timeout value through the kernel command
line. This will only help for all systems if you use a high enough
large timeout value, or on a case by case basis for each system.
I recently proposed replacing a kill for a warn only for udev
kmod built in commands, that's unacceptable for systemd's architecture
though so the last thing I proposed instead to use *for now* is a
multiplier for each different type of udev built-in command and
for kmod we'd use a high enough value, the timeout therefore would
be really large for module loading for now, but we'd still want to
collect logs of drivers taking long to probe. That's still being
discussed [0] but my hope is that with this series and that other
systemd discussion we'll have covered both areas affected and have
a good strategy to move forward with this new driver requirement.

[0] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/21689

  Luis

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 2/3] cxgb4: use module_long_probe_init()

[Luis R. Rodriguez]

On Thu, Aug 14, 2014 at 09:53:21PM +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 14, 2014 at 09:42:49AM -0700, Casey Leedom wrote:
> >   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 ...
> 
> Some maintainers consider most of the work to get what you need done
> simple, I've tried to explain it ain't so, so glad you provided a bit
> of details here. To be clear its not just about asynch firmware loading,
> you need a bit more work. Can you evaluate using a bus driver?

<-- snip -->

> >   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.
> 
> The way that the 30 second timeout was introduced as a new driver
> initialization requirement was certainly not ideal specially since
> the respective systemd patch that intended to trigger the SIGKILL on
> kmod module loading only took effect once kernel commit 786235ee
> went in about a year later, and since the original systemd commit
> was only addressing asynchronous firmware loading as a possible
> issue that drivers may need to fix. The cxgb4 driver is a good
> example that needs quite a bit of more work. Regardless systemd
> folks are right -- but again, having this be introduced as a new
> requirement that otherwise simply kills drivers seems a bit too
> aggressive specially if its killing boot on some systems due to
> delays on storage drivers. What's done is done -- and we need to
> move on. We already reviewed twice now reverting 786235ee and that
> won't happen, as a compromise we're looking for an easy agreeable
> general driver work around that would both circumvent the issue
> and let us easily grep for broken drivers. The deferred probe trick
> was the first approach and this series addresses the more agreeable
> solution. This 2 line patch then is what we are looking as work
> around until your driver gets properly fixed.
> 
> Apart from these kernel changes there are systemd changes we've
> looked at modifying, Hannes' patch 9719859c07a, now merged upstream on
> systemd lets you override the timeout value through the kernel command
> line. This will only help for all systems if you use a high enough
> large timeout value, or on a case by case basis for each system.
> I recently proposed replacing a kill for a warn only for udev
> kmod built in commands, that's unacceptable for systemd's architecture
> though so the last thing I proposed instead to use *for now* is a
> multiplier for each different type of udev built-in command and
> for kmod we'd use a high enough value, the timeout therefore would
> be really large for module loading for now, but we'd still want to
> collect logs of drivers taking long to probe. That's still being
> discussed [0] but my hope is that with this series and that other
> systemd discussion we'll have covered both areas affected and have
> a good strategy to move forward with this new driver requirement.
> 
> [0] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/21689

Here's another affected driver:

https://bugzilla.kernel.org/show_bug.cgi?id=59581

pata_marvell, and using the work around in this series should work,
just as the deferred probe work around. Alexander however notes that
the pata_marvell driver is just a simple wrapper and other devices
can act the same way. This can surely be fixed perhaps in libata
but its an example of an old driver and folks not being around to
care much over drivers which are affected.

This driver also uses module_pci_driver() so a module_long_probe_driver()
and respective module_long_probe_pci_driver() would need to be considered
if but easily implemented (sent to Alex to test).

  Luis

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

[Luis R. Rodriguez]

On Thu, Aug 14, 2014 at 09:53:21PM +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 14, 2014 at 09:42:49AM -0700, Casey Leedom wrote:
> >   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 ...
> 
> Some maintainers consider most of the work to get what you need done
> simple, I've tried to explain it ain't so, so glad you provided a bit
> of details here. To be clear its not just about asynch firmware loading,
> you need a bit more work. Can you evaluate using a bus driver?

<-- snip -->

> >   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.
> 
> The way that the 30 second timeout was introduced as a new driver
> initialization requirement was certainly not ideal specially since
> the respective systemd patch that intended to trigger the SIGKILL on
> kmod module loading only took effect once kernel commit 786235ee
> went in about a year later, and since the original systemd commit
> was only addressing asynchronous firmware loading as a possible
> issue that drivers may need to fix. The cxgb4 driver is a good
> example that needs quite a bit of more work. Regardless systemd
> folks are right -- but again, having this be introduced as a new
> requirement that otherwise simply kills drivers seems a bit too
> aggressive specially if its killing boot on some systems due to
> delays on storage drivers. What's done is done -- and we need to
> move on. We already reviewed twice now reverting 786235ee and that
> won't happen, as a compromise we're looking for an easy agreeable
> general driver work around that would both circumvent the issue
> and let us easily grep for broken drivers. The deferred probe trick
> was the first approach and this series addresses the more agreeable
> solution. This 2 line patch then is what we are looking as work
> around until your driver gets properly fixed.
> 
> Apart from these kernel changes there are systemd changes we've
> looked at modifying, Hannes' patch 9719859c07a, now merged upstream on
> systemd lets you override the timeout value through the kernel command
> line. This will only help for all systems if you use a high enough
> large timeout value, or on a case by case basis for each system.
> I recently proposed replacing a kill for a warn only for udev
> kmod built in commands, that's unacceptable for systemd's architecture
> though so the last thing I proposed instead to use *for now* is a
> multiplier for each different type of udev built-in command and
> for kmod we'd use a high enough value, the timeout therefore would
> be really large for module loading for now, but we'd still want to
> collect logs of drivers taking long to probe. That's still being
> discussed [0] but my hope is that with this series and that other
> systemd discussion we'll have covered both areas affected and have
> a good strategy to move forward with this new driver requirement.
> 
> [0] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/21689

Here's another affected driver:

https://bugzilla.kernel.org/show_bug.cgi?id=59581

pata_marvell, and using the work around in this series should work,
just as the deferred probe work around. Alexander however notes that
the pata_marvell driver is just a simple wrapper and other devices
can act the same way. This can surely be fixed perhaps in libata
but its an example of an old driver and folks not being around to
care much over drivers which are affected.

This driver also uses module_pci_driver() so a module_long_probe_driver()
and respective module_long_probe_pci_driver() would need to be considered
if but easily implemented (sent to Alex to test).

  Luis

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

[gregkh]

On Fri, Aug 15, 2014 at 02:14:58AM +0200, Luis R. Rodriguez wrote:
> This driver also uses module_pci_driver() so a module_long_probe_driver()
> and respective module_long_probe_pci_driver() would need to be considered
> if but easily implemented (sent to Alex to test).

No, don't create bus-only versions of the "long probe" function, just
unwrap the module_pci_driver() logic and use the module_long_probe()
call, we want it to be "obvious" that something is odd here and needs to
be fixed someday.

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/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 2/3] cxgb4: use module_long_probe_init()

[Luis R. Rodriguez]

On Fri, Aug 15, 2014 at 11:15:00AM -0700, Casey Leedom wrote:
>
> On 08/14/2014 12:53 PM, Luis R. Rodriguez wrote:
>> On Thu, Aug 14, 2014 at 09:42:49AM -0700, Casey Leedom wrote:
>>> 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.
>> I sent a patch for this a while ago, since there is no objection if
>> you'd like to apply the patch:
>>
>> http://patchwork.ozlabs.org/patch/363756/
>
>   I think I'll leave that to Hariprasad since he's been doing all the 
> direct kernel.org work recently.  Also, I honestly don't understand the 
> differences between request_firmware() and request_firmware_direct().  It 
> sounds like request_firmware() will invoke some user-level process if it 
> can't find the specified firmware.  What that user-level process will do is 
> unknown and of unknown value (and undocumented as far as I can tell).

The userspace helper ended up being a bad idea so its being phased out
and will eventually be removed. udev used to be the default helper
and it would write firmware using the sysfs interface, and it would
have a timeout of 60 seconds. Other drivers could provide their own
userspace helper and last I checked we only had one Dell device driver
left which still required this kernel feature. The goal is to phase
that out and simply nuke it, in favor for the direct kernel firmware
loading, the kernel essentially opens the file and reads its contents
directly form the filesystem, this removes the silly timeout. udev
is part of systemd and a patch in May tried to remove the udev firmware
loader [0], it however was decided it won't be until the dell rbu
driver gets fixed to not require kernel side support for the userspace
firmware helper [1]. I would not be surprised if this happens soon and
you should expect Linux distributions avoiding enabling the kernel
CONFIG_FW_LOADER_USER_HELPER, as it essentially has an incurred
built-in timeout of 60 seconds every single time request_firmware()
is used. request_firmware_direct() enables device drivers that know
that the firmwares being requested are optional to skip the usermode
helper and therefore skips the 60 second timeout all together.
Once the usermode helper is removed request_firmware() will behave
the same as request_firmware_direct() with the only difference
being a print of failure if the firmware was expected to be present.
Right now using request_firmware_direct() will help speed things up
for drivers that have optional firmware as the usermode helper is
still present, that's all.

[0] http://lists.freedesktop.org/archives/systemd-devel/2014-May/019587.html
[1] http://lists.freedesktop.org/archives/systemd-devel/2014-May/019631.html

>> Apart from that you also want to use asynch firmware loading but
>> to use that properly (I also had sent some basic initial patches
>> for asynch firmware loading but without moving out other logic
>> yet) you want to also let driver initalization complete
>> asynchronously later.
>
>   So it looked like the module_init() function is what the various patches 
> have moved into an asynchronous thread, right? 

Its a temporary hack to fix drivers which are taking over 30 seconds on
init, but it also helps us annotate drivers that are not adhering to the
new 30 second requirement imposed on us by systemd and now supported
as reasonable kernel policy. I would in no way consider it asynchronous
functionality, you should call it a hack and annotation of broken
drivers with long probes that need work.

> That should address the 
> worry that a failing PCI probe() wouldn't be able to return the failure 
> status to the PCI infrastructure (and ensure that the PCI remove() function 
> wouldn't be erroneously called for a device which failed probe()).

Right, that should not happen with this hack, the only difference
here is that loading the driver will return immediately and return
a success, while behidn the scenes it goes on running the driver's
own old probe. For this reason the module loading will return
sucessfully immediately unless there is no memory for the driver,
for example. This is acceptable given that driver's init sequence
should initialize the device driver, probing for devices should be
handled asynchronously behind the scenes. Proper asynchronous probing
will vary depending on the subsystem and architecture of the driver,
using asynchronous firmware loading is just one of the strategies
drivers should be using. Your driver obviously needs much more work.

>>>   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.
>> I had originally recommended to write your own platform driver for
>> this and have each port probe but Greg has provided the last advice
>> for this on the patch I sent to add deferred probe support from
>> init, his recommendation was for you to write your own bus code for
>> the driver:
>>
>> http://www.spinics.net/lists/linux-scsi/msg76695.html
>
>   I'm unfamiliar with the term "platform driver"

drivers/base/platform.c
include/linux/platform_device.h

> but the "probe()" is of 
> the adapter, not ports.  Our driver attaches to a single PCI Physical 
> Function and manages all adapter resources (including instantiating 
> multiple Network Devices) from that single PF.  This allows the driver to 
> manage adapter resources as a allocation pools, etc.

The idea is to have each network device be initialized through an internal
bus logic.

>   The probe() function verifies that the PCI Device is what we want to 
> manage and performs all adapter initialization.  This has been what "Device 
> Probe" routine have been doing for more than forty years (predating UNIX).  
> Some OSes have separately abstracted the concept of Device Initialization 
> but UNIX/Linux have mostly stayed with the single Device Probe function ... 
>  So I'm not sure where people are asking us to move the Device 
> Initialization code to ...

There's a difference between driver initialization and device driver probing.
On Linux when device drivers have autoprobe enabled (most) the driver's own
probe will be run if a device is found to have a match right after the driver
is initialization is completed, this in turn means that *both* init and probe
will be called upon driver initialization right now.

You therefore are right to yell bloody murder as cxgb4_init_module() doesn't
do anything other than register the PCI driver with pci_driver_register().
Its your driver's PCI probe which takes eons.

pci_driver_register() --> bus_add_driver() --> driver_attach()

Now, one could argue and IMHO it would be reasonable that the driver's
probing should be dealt with separately, it at least seems that's what
we want, given that its not driver's init which is long, its the driver's
own probe which we wish to start behaving asynchronously. We could however
generalize offloading probe on a kthread as well, but its not clear to me
if this is desirable, Greg? I tried this on my system and it did seem to
help with boot time, systemd-analyze yield this before and after:

Startup finished in 4.271s (kernel) + 2.547s (initrd) + 30.868s (userspace) = 37.686s
Startup finished in 3.483s (kernel) + 2.455s (initrd) + 31.580s (userspace) = 37.519s

A patch for this is supplied at the bottom in case folks would like to try.
I did run into an issue with my built in keyboard not working though but
didn't find why, a USB keyboard however did the trick.

>   For us, this Device Probe adapter initialization includes downloading the 
> main chip firmware (if needed when the adapter contains old unsupported 
> firmware) and possibly external PHY firmware as well for 10Gb/s "BT" cards 
> ...  Even without the request_firmware() user-level process issues, simply 
> FLASH'ing both firmware images on the BT cards can take quite a while.

Yup, yeah this is what Greg recommended could be dealt with through a device
specific bus.

>>>    Part of the problem here is that it's a Module Load Timeout instead of a
>>> per-device Probe Timeout.
>> Seems like you can fix this with a bus driver.
>
>   I apologize, again I'm unfamiliar with this term with respect to Linux.  
> (Not surprising since I'm still a neophyte with regard to Linux.)

drivers/ssb/ is an example but perhaps Greg can provide a better more
suitable example ?

>>
>>>    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 ...
>> Some maintainers consider most of the work to get what you need done
>> simple, I've tried to explain it ain't so, so glad you provided a bit
>> of details here. To be clear its not just about asynch firmware loading,
>> you need a bit more work. Can you evaluate using a bus driver?
>
>   Hhmmm, a second reference to this term.  Obviously I need a pointer here 
> and an understanding of how a Bus Driver differs from "Normal Drivers" ...

Sure, start looking at ssb and I can see if there are other examples.

>>>    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
>> You should just use asynch firmware loading there and only once your
>> driver is done loading firmware start exposing the device(s) as you
>> see fit with your bus driver.
>
>   The problem here is that our Device Initialization depends intimately on 
> the main firmware.  Our T3 chip also had firmware but with T4/T5 (and 
> beyond) we've moved a lot more functionality into the main firmware — 
> including Chip initialization — which has allowed us to simplify the Host 
> Drivers and also hid most of the chip version differences from the Host 
> Driver.

OK. Regardless you should use asynch firmware loading.

>>> .  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.
>> The way that the 30 second timeout was introduced as a new driver
>> initialization requirement was certainly not ideal specially since
>> the respective systemd patch that intended to trigger the SIGKILL on
>> kmod module loading only took effect once kernel commit 786235ee
>> went in about a year later, and since the original systemd commit
>> was only addressing asynchronous firmware loading as a possible
>> issue that drivers may need to fix. The cxgb4 driver is a good
>> example that needs quite a bit of more work. Regardless systemd
>> folks are right -- but again, having this be introduced as a new
>> requirement that otherwise simply kills drivers seems a bit too
>> aggressive specially if its killing boot on some systems due to
>> delays on storage drivers. What's done is done -- and we need to
>> move on. We already reviewed twice now reverting 786235ee and that
>> won't happen, as a compromise we're looking for an easy agreeable
>> general driver work around that would both circumvent the issue
>> and let us easily grep for broken drivers. The deferred probe trick
>> was the first approach and this series addresses the more agreeable
>> solution. This 2 line patch then is what we are looking as work
>> around until your driver gets properly fixed.
>>
>> Apart from these kernel changes there are systemd changes we've
>> looked at modifying, Hannes' patch 9719859c07a, now merged upstream on
>> systemd lets you override the timeout value through the kernel command
>> line. This will only help for all systems if you use a high enough
>> large timeout value, or on a case by case basis for each system.
>> I recently proposed replacing a kill for a warn only for udev
>> kmod built in commands, that's unacceptable for systemd's architecture
>> though so the last thing I proposed instead to use *for now* is a
>> multiplier for each different type of udev built-in command and
>> for kmod we'd use a high enough value, the timeout therefore would
>> be really large for module loading for now, but we'd still want to
>> collect logs of drivers taking long to probe. That's still being
>> discussed [0] but my hope is that with this series and that other
>> systemd discussion we'll have covered both areas affected and have
>> a good strategy to move forward with this new driver requirement.
>>
>> [0] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/21689
>
>   Okay, obviously a lot of history here.  I think I'm stuck with the basic 
> desire to return a Device Probe failure if the adapter fails initialization 
> but also being told that we can't take "too long" performing the Device 
> Initialization portion of the Device Probe ...  Where should Device 
> Initialization be performed if not in the Device Probe routine?

Yeah its a good point, its important we highlight that all along its
not driver's init sequence that is taking long, its the probe that
is taking long and its because we trigger that right after we
initialize the driver, immediately if autoprobe is enabled on the
bus. Regardless though things like asynch firmware loading is
something folks should strive for -- that no one is going to argue
with, its the other aspects of probing that you also need to work
on such as a bus driver but -- now that we've reached this
clarification I do wonder if it makes sense to *not* have drivers
probe calls spawned immediately and instead have them run through
kthreads. Example patch on how to do that below.

  Luis

---
 drivers/base/dd.c      | 20 +++++++++++++++++++-
 include/linux/device.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..b0f54db 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -482,6 +482,12 @@ static int __driver_attach(struct device *dev, void *data)
 	return 0;
 }
 
+int driver_attach_all(void *arg)
+{
+	struct device_driver *drv = (struct device_driver *) arg;
+	return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);
+}
+
 /**
  * driver_attach - try to bind driver to devices.
  * @drv: driver.
@@ -493,7 +499,13 @@ static int __driver_attach(struct device *dev, void *data)
  */
 int driver_attach(struct device_driver *drv)
 {
-	return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);
+	drv->attach_thread = kthread_create(driver_attach_all, drv, "%s_%s",
+					    "driver_attach", drv->name);
+	if (IS_ERR(drv->attach_thread))
+		return PTR_ERR(drv->attach_thread);
+	get_task_struct(drv->attach_thread);
+	wake_up_process(drv->attach_thread);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(driver_attach);
 
@@ -562,6 +574,11 @@ void driver_detach(struct device_driver *drv)
 {
 	struct device_private *dev_prv;
 	struct device *dev;
+	int err;
+
+	err = kthread_stop(drv->attach_thread);
+	if (err)
+		return;
 
 	for (;;) {
 		spin_lock(&drv->p->klist_devices.k_lock);
@@ -586,4 +603,5 @@ void driver_detach(struct device_driver *drv)
 			device_unlock(dev->parent);
 		put_device(dev);
 	}
+	put_task_struct(drv->attach_thread);
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index dc7c0ba7..fa2bced 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -235,6 +235,7 @@ struct device_driver {
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
 
+	struct task_struct *attach_thread;
 	const struct of_device_id	*of_match_table;
 	const struct acpi_device_id	*acpi_match_table;
 
-- 
2.0.3

_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

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