On 10/17/2018 5:54 PM, Dan Williams wrote:
On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche <bvanassche@xxxxxxx> wrote:
Instead of probing devices sequentially in the PROBE_PREFER_ASYNCHRONOUS
mode, scan devices concurrently. This helps when the wall clock time for
a single probe is significantly above the CPU time needed for a single
probe, e.g. when scanning SCSI LUNs over a storage network.
Alex had a similar patch here [1] that I don't think has been accepted
yet, in any event some collaboration is needed:
[1]: https://lkml.org/lkml/2018/9/27/14
The patch set referenced is a little out of date. The latest set is:
https://lore.kernel.org/lkml/20181015150305.29520.86363.stgit@localhost.localdomain/
I'm also not quite sure what the point of this patch is. I don't think
it is doing what it says it is doing. From what I can tell it is just
allowing the driver init code to ignore if the driver wants to be probed
asynchronously or not. Further comments inline below.
Cc: Lee Duncan <lduncan@xxxxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxxx>
Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
Cc: Johannes Thumshirn <jthumshirn@xxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
drivers/base/bus.c | 3 +--
drivers/base/dd.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..18ca1178821f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -696,8 +696,7 @@ int bus_add_driver(struct device_driver *drv)
out_unregister:
kobject_put(&priv->kobj);
- /* drv->p is freed in driver_release() */
- drv->p = NULL;
+
out_put_bus:
bus_put(bus);
return error;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 033382421351..f8d645aa09be 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include <linux/async.h>
#include <linux/pm_runtime.h>
#include <linux/pinctrl/devinfo.h>
+#include <linux/slab.h>
#include "base.h"
#include "power/power.h"
@@ -691,6 +692,49 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
return ret;
}
+struct driver_and_dev {
+ struct device_driver *drv;
+ struct device *dev;
+};
+
+static void __driver_probe_device_async(void *data, async_cookie_t cookie)
+{
+ struct driver_and_dev *dd = data;
+ struct device_driver *drv = dd->drv;
+ struct device *dev = dd->dev;
+
+ device_lock(dev);
+ driver_probe_device(drv, dev);
+ device_unlock(dev);
+ kobject_put(&drv->p->kobj);
+ module_put(drv->owner);
+ kfree(dd);
+}
+
+static void driver_probe_device_async(struct device_driver *drv,
+ struct device *dev)
+{
+ struct driver_and_dev *dd;
+
+ if (!try_module_get(drv->owner))
+ return;
+ dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+ if (!dd) {
+ /* If out of memory, scan synchronously. */
+ device_lock(dev);
+ driver_probe_device(drv, dev);
+ device_unlock(dev);
+ module_put(drv->owner);
+ return;
+ }
+ *dd = (struct driver_and_dev){
+ .drv = drv,
+ .dev = dev,
+ };
+ kobject_get(&drv->p->kobj);
+ async_schedule(__driver_probe_device_async, dd);
+}
+
So this piece is similar to what I had, but the functionality is being
used in a completely different spot. I'm not entirely convinced this
isn't redundant.
bool driver_allows_async_probing(struct device_driver *drv)
{
switch (drv->probe_type) {
@@ -777,6 +821,11 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
if (data->check_async && async_allowed != data->want_async)
return 0;
+ if (data->check_async) {
+ driver_probe_device_async(drv, dev);
+ return 0;
+ }
+
So this code path assumes the driver is already loaded before the device
is added, and from what I can tell it looks like it is forcing
everything to asynchronously probe isn't it? Wasn't that the purpose of
the async_allowed != data->want_async check?
Also this seems redundant since the return 0 case here is supposed to
have us attach the device asynchronously. So it seems like we are just
adding another layer of aysnc init. It seems like the most direct way of
doing this would be to just force data->want_async to always be true by
passing that instead of false in device_attach?
return driver_probe_device(drv, dev);
}
--
2.19.1.568.g152ad8e336-goog
