Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
On Tue, Oct 7, 2014 at 11:55 AM, Luis R. Rodriguez wrote: > OK I'll just kill bus.enable_kern_async=1 to enable built-in async > probe support *and* also have prefer_async_probe *always* be > respected, whether modular or not. Well and I just realized you *do* want to flush, so will throw that in too without an option to skip it. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
On Tue, Oct 7, 2014 at 10:55 AM, Tejun Heo wrote: > Hello, > > On Tue, Oct 07, 2014 at 07:50:10PM +0200, Luis R. Rodriguez wrote: >> On Tue, Oct 07, 2014 at 01:34:04PM -0400, Tejun Heo wrote: >> > But you can create a new workqueue and queue all the async probing >> > work items there and flush the workqueue right after >> > async_synchronize_full(). >> >> On second thought I would prefer to avoid this, I see this being good >> to help with old userspace but other than that I don't see a requirement >> for new userspace. Do you? > > Hmmm... we batch up and do everything parallel, so I'm not sure how > much gain we'd be looking at by not waiting for at the end before > jumping into the userland. Also, it's a bit of an orthogonal issue. > If we wanna skip such synchornization point before passing control to > userland, why are we applying that to this but not > async_synchronize_full() which has a far larger impact? It's weird to > synchronize one while not the other, so yeah, if there are actual > benefits we can consider it but let's do it separately. OK I'll just kill bus.enable_kern_async=1 to enable built-in async probe support *and* also have prefer_async_probe *always* be respected, whether modular or not. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
Hello, On Tue, Oct 07, 2014 at 07:50:10PM +0200, Luis R. Rodriguez wrote: > On Tue, Oct 07, 2014 at 01:34:04PM -0400, Tejun Heo wrote: > > But you can create a new workqueue and queue all the async probing > > work items there and flush the workqueue right after > > async_synchronize_full(). > > On second thought I would prefer to avoid this, I see this being good > to help with old userspace but other than that I don't see a requirement > for new userspace. Do you? Hmmm... we batch up and do everything parallel, so I'm not sure how much gain we'd be looking at by not waiting for at the end before jumping into the userland. Also, it's a bit of an orthogonal issue. If we wanna skip such synchornization point before passing control to userland, why are we applying that to this but not async_synchronize_full() which has a far larger impact? It's weird to synchronize one while not the other, so yeah, if there are actual benefits we can consider it but let's do it separately. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
On Tue, Oct 07, 2014 at 01:34:04PM -0400, Tejun Heo wrote: > Hello, > > On Tue, Oct 07, 2014 at 01:10:46AM +0200, Luis R. Rodriguez wrote: > > On Mon, Oct 06, 2014 at 05:01:18PM -0400, Tejun Heo wrote: > > > For in-kernel stuff, we already have a clear > > > synchronization point where we already synchronize all async calls. > > > Shouldn't we be flushing these async probes there too? > > > > This seems to be addressing if what I meant by prepared, "ready", so let > > me address this as I do think its important. > > > > By async calls do you mean users of async_schedule()? I see it > > Yes. > > > also uses system_unbound_wq as well but I do not see anyone calling > > flush_workqueue(system_unbound_wq) on the kernel. We do use > > async_synchronize_full() on kernel_init() but that just waits. > > But you can create a new workqueue and queue all the async probing > work items there and flush the workqueue right after > async_synchronize_full(). On second thought I would prefer to avoid this, I see this being good to help with old userspace but other than that I don't see a requirement for new userspace. Do you? > ... > > bus.enable_kern_async=1 would still also serve as a helper for the driver > > core > > to figure out if it should use async probe then on modules if > > prefer_async_probe > > was enabled. Let me know if you figure out a way to avoid it. > > Why do we need the choice at all? It always should, no? I'm OK to live with that, in that case I see no point to bus.enable_kern_async=1 at all. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
Hello, On Tue, Oct 07, 2014 at 01:10:46AM +0200, Luis R. Rodriguez wrote: > On Mon, Oct 06, 2014 at 05:01:18PM -0400, Tejun Heo wrote: > > For in-kernel stuff, we already have a clear > > synchronization point where we already synchronize all async calls. > > Shouldn't we be flushing these async probes there too? > > This seems to be addressing if what I meant by prepared, "ready", so let > me address this as I do think its important. > > By async calls do you mean users of async_schedule()? I see it Yes. > also uses system_unbound_wq as well but I do not see anyone calling > flush_workqueue(system_unbound_wq) on the kernel. We do use > async_synchronize_full() on kernel_init() but that just waits. But you can create a new workqueue and queue all the async probing work items there and flush the workqueue right after async_synchronize_full(). ... > bus.enable_kern_async=1 would still also serve as a helper for the driver core > to figure out if it should use async probe then on modules if > prefer_async_probe > was enabled. Let me know if you figure out a way to avoid it. Why do we need the choice at all? It always should, no? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
On Mon, Oct 06, 2014 at 05:01:18PM -0400, Tejun Heo wrote: > Hello, > > On Mon, Oct 06, 2014 at 10:36:27PM +0200, Luis R. Rodriguez wrote: > > > Do we intend to keep this param permanently? Isn't this more of a > > > temp tool to be used during development? If so, maybe we should make > > > that clear with __DEVEL__ too? > > > > As its designed right now no, its not a temp tool, its there to > > require compatibility with old userspace. For modules we can require > > the module parameter but for built-in we need something else and this > > is what came to mind. It is also what would allow the prefer_async_probe > > flag too as otherwise we won't know if userspace is prepared. > > I don't get it. By prepared I meant that userspace can handle async probe, but you're right that we don't need to know that. I don't see how we'd be breaking old userspace by doing async probe of a driver is built-in right now... unless of course built-in always assumes all possible devices would be present after right before userspace init. > For in-kernel stuff, we already have a clear > synchronization point where we already synchronize all async calls. > Shouldn't we be flushing these async probes there too? This seems to be addressing if what I meant by prepared, "ready", so let me address this as I do think its important. By async calls do you mean users of async_schedule()? I see it also uses system_unbound_wq as well but I do not see anyone calling flush_workqueue(system_unbound_wq) on the kernel. We do use async_synchronize_full() on kernel_init() but that just waits. As it is we don't wait on init then, should we? Must we? Could / should we use bus.enable_kern_async=1 to enable avoiding having to wait ? At this point I'd prefer to address what we must do only. > insmod'ing is > userland visible but there's no reason this has to be for the built-in > drivers. Good point. bus.enable_kern_async=1 would still also serve as a helper for the driver core to figure out if it should use async probe then on modules if prefer_async_probe was enabled. Let me know if you figure out a way to avoid it. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
Hello, On Mon, Oct 06, 2014 at 10:36:27PM +0200, Luis R. Rodriguez wrote: > > Do we intend to keep this param permanently? Isn't this more of a > > temp tool to be used during development? If so, maybe we should make > > that clear with __DEVEL__ too? > > As its designed right now no, its not a temp tool, its there to > require compatibility with old userspace. For modules we can require > the module parameter but for built-in we need something else and this > is what came to mind. It is also what would allow the prefer_async_probe > flag too as otherwise we won't know if userspace is prepared. I don't get it. For in-kernel stuff, we already have a clear synchronization point where we already synchronize all async calls. Shouldn't we be flushing these async probes there too? insmod'ing is userland visible but there's no reason this has to be for the built-in drivers. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
Hi Luis, On Fri, Oct 03, 2014 at 02:44:43PM -0700, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > At times we may wish to express the desire to prefer to have > a device driver probe asynchronously by default. We cannot > simply enable all device drivers to do this without vetting > that userspace is prepared for this though given that some > old userspace is expected to exist which is not equipped to > deal with broad async probe support. This defines a new kernel > parameter, bus.enable_kern_async=1, to help address this both to > help enable async probe support for built-in drivers and to > enable drivers to specify a preference to opt in for async > probe support. > > If you have a device driver that should use async probe > support when possible enable the prefer_async_probe bool. > > Folks wishing to test enabling async probe for all built-in > drivers can enable bus.__DEBUG__kernel_force_mod_async_probe=1, > if you use that though you are on your own. Thank you for working on this. However there are still couple of issues with the async probe. 1. As far as I can see you only handle the case when device is already present and you load a driver. In this case we will do either async or sync probing, depending on the driver/module settings. However if driver has already been loaded/registered and we are adding a new device (another module load, for example you load i2c controller module and it enumerates its children, or driver signalled deferral during binding) the probe will be synchronous regardless of the settings. 2. I thin kin the current implementation deferred binding process is still single-threaded and basically synchronous. Both of these issues stem form the fact that you only plugging into bus_add_driver(), but you also need to plug into bus_probe_device(). I believe I handled these 2 cases properly in the version of patch I sent a couple of weeks ago so if you could incorporate that in your work that would be great. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
On Mon, Oct 06, 2014 at 04:19:26PM -0400, Tejun Heo wrote: > Hello, Luis. > > The patchset generally looks good to me. Please feel free to add > > Reviewed-by: Tejun Heo Will do. > A question below. > > On Fri, Oct 03, 2014 at 02:44:43PM -0700, Luis R. Rodriguez wrote: > > + bus.enable_kern_async=1 [KNL] > > + This tells the kernel userspace has been vetted for > > + asynchronous probe support and can listen to the device > > + driver prefer_async_probe flag for both built-in device > > + drivers and modules. > > Do we intend to keep this param permanently? Isn't this more of a > temp tool to be used during development? If so, maybe we should make > that clear with __DEVEL__ too? As its designed right now no, its not a temp tool, its there to require compatibility with old userspace. For modules we can require the module parameter but for built-in we need something else and this is what came to mind. It is also what would allow the prefer_async_probe flag too as otherwise we won't know if userspace is prepared. If we want to get rid of it, it would mean that we're letting go of the idea that some userspace might exist which depends on *not* doing async probe. As such I would not consider it a __DEVEL__ param and it'd be a judgement call to eventually *not require* it. I can see that happening but perhaps a large series of kernels down the road? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
Hello, Luis. The patchset generally looks good to me. Please feel free to add Reviewed-by: Tejun Heo A question below. On Fri, Oct 03, 2014 at 02:44:43PM -0700, Luis R. Rodriguez wrote: > + bus.enable_kern_async=1 [KNL] > + This tells the kernel userspace has been vetted for > + asynchronous probe support and can listen to the device > + driver prefer_async_probe flag for both built-in device > + drivers and modules. Do we intend to keep this param permanently? Isn't this more of a temp tool to be used during development? If so, maybe we should make that clear with __DEVEL__ too? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
From: "Luis R. Rodriguez"
At times we may wish to express the desire to prefer to have
a device driver probe asynchronously by default. We cannot
simply enable all device drivers to do this without vetting
that userspace is prepared for this though given that some
old userspace is expected to exist which is not equipped to
deal with broad async probe support. This defines a new kernel
parameter, bus.enable_kern_async=1, to help address this both to
help enable async probe support for built-in drivers and to
enable drivers to specify a preference to opt in for async
probe support.
If you have a device driver that should use async probe
support when possible enable the prefer_async_probe bool.
Folks wishing to test enabling async probe for all built-in
drivers can enable bus.__DEBUG__kernel_force_mod_async_probe=1,
if you use that though you are on your own.
Cc: Tejun Heo
Cc: Arjan van de Ven
Cc: Greg Kroah-Hartman
Cc: Tetsuo Handa
Cc: Joseph Salisbury
Cc: Kay Sievers
Cc: One Thousand Gnomes
Cc: Tim Gardner
Cc: Pierre Fersing
Cc: Andrew Morton
Cc: Oleg Nesterov
Cc: Benjamin Poirier
Cc: Nagalakshmi Nandigama
Cc: Praveen Krishnamoorthy
Cc: Sreekanth Reddy
Cc: Abhijit Mahajan
Cc: Casey Leedom
Cc: Hariprasad S
Cc: Santosh Rastapur
Cc: mpt-fusionlinux@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez
---
Documentation/kernel-parameters.txt | 9 +++
drivers/base/bus.c | 52 ++---
include/linux/device.h | 9 +++
3 files changed, 60 insertions(+), 10 deletions(-)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index e4be3b8..56f4d4e 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -914,12 +914,21 @@ bytes respectively. Such letter suffixes can also be
entirely omitted.
Enable debug messages at boot time. See
Documentation/dynamic-debug-howto.txt for details.
+ bus.enable_kern_async=1 [KNL]
+ This tells the kernel userspace has been vetted for
+ asynchronous probe support and can listen to the device
+ driver prefer_async_probe flag for both built-in device
+ drivers and modules.
module.async_probe [KNL]
Enable asynchronous probe on this module.
bus.__DEBUG__module_force_mod_async_probe=1 [KNL]
Enable asynchronous probe on all modules. This is
testing parameter and using it will taint your
kernel.
+ bus.__DEBUG__kernel_force_mod_async_probe=1 [KNL]
+ Enable asynchronous probe on all built-in drivers. This
+ is testing parameter and using it will taint your
+ kernel.
early_ioremap_debug [KNL]
Enable debug messages in early_ioremap support. This
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index ec203d6..25d0412 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -695,8 +695,10 @@ int bus_driver_async_probe(struct device_driver *drv)
INIT_WORK(&priv->attach_work->work, driver_attach_workfn);
/* Keep this as pr_info() until this is prevalent */
- pr_info("bus: '%s': probe for driver %s is run asynchronously\n",
-drv->bus->name, drv->name);
+ pr_info("bus: '%s': probe for %s driver %s is run asynchronously\n",
+drv->bus->name,
+drv->owner ? "module" : "built-in",
+drv->name);
queue_work(system_unbound_wq, &priv->attach_work->work);
@@ -708,6 +710,16 @@ module_param_named(__DEBUG__module_force_mod_async_probe,
force_mod_async, bool,
MODULE_PARM_DESC(__DEBUG__module_force_mod_async_probe,
"Force async probe on all modules");
+static bool force_kern_async = false;
+module_param_named(__DEBUG__kernel_force_mod_async_probe, force_kern_async,
bool, 0400);
+MODULE_PARM_DESC(__DEBUG__kernel_force_mod_async_probe,
+"Force async probe on all modules");
+
+static bool enable_kern_async = false;
+module_param_named(enable_kern_async, enable_kern_async, bool, 0400);
+MODULE_PARM_DESC(enable_kern_async,
+"Userspace is vetted to handle driver async probe");
+
/**
* drv_enable_async_probe - evaluates if async probe should be used
* @drv: device driver to evaluate
@@ -722,25 +734,41 @@ MODULE_PARM_DESC(__DEBUG__module_force_mod_async_probe,
* be used on it.
*
* Drivers can be built-in or modules. Userspace can inform the kernel that
- * it is prepared for async probe by passing the module parameter
- * async_probe on each module it wishes to load. The async_probe parameter is
+ * it is prepared for async probe by either passin
