Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On 10/18/2018 10:15 AM, Greg KH wrote: On Thu, Oct 18, 2018 at 09:51:03AM -0700, Alexander Duyck wrote: On 10/18/2018 9:46 AM, Bart Van Assche wrote: On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote: On 10/17/2018 5:54 PM, Dan Williams wrote: On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche 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. Hi Alexander, Thanks for the pointer to the latest version of your patch series. I was not yet aware of your work when I posted this patch series. Now that I have had a look at your patch series I like your approach better than what I did in this patch. Since it could take a while before agreement is reached about the async domain patches in the same patch series, how about you submitting patches 3/6 and 4/6 from your patch series to Greg for kernel version v4.20? If I drop the driver core patches from my patch series and replace these with your driver core patches I achieve the same results. If you Cc me when you resubmit these patches I will review them. Thanks, Bart. Actually the async and workqueue patches have already been reviewed and last I knew they were okay with the workqueue guys. These patches are already submitted to Greg for 4.20. It's too late for 4.20 now, sorry. They will have to wait. Given that 4.19-final could have come out last weekend, this shouldn't be a supprise. They are in my review queue and I'll get to them after 4.20-rc1 is out. thanks, greg k-h Well I was hoping for 4.20 anyway. :-/ Thanks for letting me know. - Alex
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On Thu, Oct 18, 2018 at 09:51:03AM -0700, Alexander Duyck wrote: > On 10/18/2018 9:46 AM, Bart Van Assche wrote: > > On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote: > > > On 10/17/2018 5:54 PM, Dan Williams wrote: > > > > On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche > > > > 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. > > > > Hi Alexander, > > > > Thanks for the pointer to the latest version of your patch series. I was not > > yet aware of your work when I posted this patch series. Now that I have had > > a > > look at your patch series I like your approach better than what I did in > > this > > patch. Since it could take a while before agreement is reached about the > > async > > domain patches in the same patch series, how about you submitting patches > > 3/6 > > and 4/6 from your patch series to Greg for kernel version v4.20? If I drop > > the driver core patches from my patch series and replace these with your > > driver core patches I achieve the same results. If you Cc me when you > > resubmit > > these patches I will review them. > > > > Thanks, > > > > Bart. > > Actually the async and workqueue patches have already been reviewed and last > I knew they were okay with the workqueue guys. These patches are already > submitted to Greg for 4.20. It's too late for 4.20 now, sorry. They will have to wait. Given that 4.19-final could have come out last weekend, this shouldn't be a supprise. They are in my review queue and I'll get to them after 4.20-rc1 is out. thanks, greg k-h
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On 10/18/2018 9:46 AM, Bart Van Assche wrote: On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote: On 10/17/2018 5:54 PM, Dan Williams wrote: On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche 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. Hi Alexander, Thanks for the pointer to the latest version of your patch series. I was not yet aware of your work when I posted this patch series. Now that I have had a look at your patch series I like your approach better than what I did in this patch. Since it could take a while before agreement is reached about the async domain patches in the same patch series, how about you submitting patches 3/6 and 4/6 from your patch series to Greg for kernel version v4.20? If I drop the driver core patches from my patch series and replace these with your driver core patches I achieve the same results. If you Cc me when you resubmit these patches I will review them. Thanks, Bart. Actually the async and workqueue patches have already been reviewed and last I knew they were okay with the workqueue guys. These patches are already submitted to Greg for 4.20. The only bits that I had left were the patches for driver-core and I am already working those through with Rafael Wysocki and Greg KH in terms of addressing their review comments. Thanks. - Alex
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote: > On 10/17/2018 5:54 PM, Dan Williams wrote: > > On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche 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. Hi Alexander, Thanks for the pointer to the latest version of your patch series. I was not yet aware of your work when I posted this patch series. Now that I have had a look at your patch series I like your approach better than what I did in this patch. Since it could take a while before agreement is reached about the async domain patches in the same patch series, how about you submitting patches 3/6 and 4/6 from your patch series to Greg for kernel version v4.20? If I drop the driver core patches from my patch series and replace these with your driver core patches I achieve the same results. If you Cc me when you resubmit these patches I will review them. Thanks, Bart.
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On 10/17/2018 5:54 PM, Dan Williams wrote:
On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche 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
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
Signed-off-by: Bart Van Assche
---
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(>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
#include
#include
+#include
#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(>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(>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
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On Wed, Oct 17, 2018 at 05:54:56PM -0700, Dan Williams wrote: > On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche 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 Yes, they both need to work together here... thanks for pointing this out. greg k-h
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche 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
>
> Cc: Lee Duncan
> Cc: Hannes Reinecke
> Cc: Luis Chamberlain
> Cc: Johannes Thumshirn
> Cc: Christoph Hellwig
> Cc: Greg Kroah-Hartman
> Cc: Dan Williams
> Signed-off-by: Bart Van Assche
> ---
> 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(>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
> #include
> #include
> +#include
>
> #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(>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(>p->kobj);
> + async_schedule(__driver_probe_device_async, dd);
> +}
> +
> 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;
> + }
> +
> return driver_probe_device(drv, dev);
> }
>
> --
> 2.19.1.568.g152ad8e336-goog
>
[PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
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.
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
Signed-off-by: Bart Van Assche
---
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(>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
#include
#include
+#include
#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(>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(>p->kobj);
+ async_schedule(__driver_probe_device_async, dd);
+}
+
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;
+ }
+
return driver_probe_device(drv, dev);
}
--
2.19.1.568.g152ad8e336-goog
