[PATCH] driver-core: remove lock for platform devices during probe

[PATCH] driver-core: remove lock for platform devices during probe

[Wei Li]

During driver probe procedure, lock on the parent of
platform devices could be removed to make probe in
parallel.

Signed-off-by: Wei Li <weili@codeaurora.org>
---
 drivers/base/dd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a1fbf55..e238fbc 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/platform_device.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -749,13 +750,14 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
-	if (dev->parent)	/* Needed for USB */
+	if (dev->parent &&
+		(dev->bus != &platform_bus_type))	/* Needed for USB */
 		device_lock(dev->parent);
 	device_lock(dev);
 	if (!dev->driver)
 		driver_probe_device(drv, dev);
 	device_unlock(dev);
-	if (dev->parent)
+	if (dev->parent && (dev->bus != &platform_bus_type))
 		device_unlock(dev->parent);
 
 	return 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH] driver-core: remove lock for platform devices during probe

[Greg Kroah-Hartman]

On Mon, Apr 24, 2017 at 01:42:16PM +0800, Wei Li wrote:
> During driver probe procedure, lock on the parent of
> platform devices could be removed to make probe in
> parallel.
> 
> Signed-off-by: Wei Li <weili@codeaurora.org>

Why?  Why does this matter?

> ---
>  drivers/base/dd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index a1fbf55..e238fbc 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -25,6 +25,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/platform_device.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -749,13 +750,14 @@ static int __driver_attach(struct device *dev, void *data)
>  		return ret;
>  	} /* ret > 0 means positive match */
>  
> -	if (dev->parent)	/* Needed for USB */
> +	if (dev->parent &&
> +		(dev->bus != &platform_bus_type))	/* Needed for USB */

The platform_bus_type check is not needed by USB, right?

thanks,

greg k-h

Re: [PATCH] driver-core: remove lock for platform devices during probe

[weili]

Hi Greg,

    We are optimizing boot time for Linux kernel and try to make some 
platform drivers use asynchronous probe(by changing probe type of driver 
to PROBE_PREFER_ASYNCHRONOUS) to reduce boot time. However we found the 
platform drivers did not probe in parallel because they will lock the 
same parent device(platform bus for platform drivers) during probe.  So 
we add this patch to remove lock of parent for platform device. This 
will help to make platform driver probe in parallel and reduce boot 
time.


Best Regards
Wei


On 2017-04-24 15:32, Greg Kroah-Hartman wrote:
> On Mon, Apr 24, 2017 at 01:42:16PM +0800, Wei Li wrote:
>> During driver probe procedure, lock on the parent of
>> platform devices could be removed to make probe in
>> parallel.
>> 
>> Signed-off-by: Wei Li <weili@codeaurora.org>
> 
> Why?  Why does this matter?
> 
>> ---
>>  drivers/base/dd.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index a1fbf55..e238fbc 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/async.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/pinctrl/devinfo.h>
>> +#include <linux/platform_device.h>
>> 
>>  #include "base.h"
>>  #include "power/power.h"
>> @@ -749,13 +750,14 @@ static int __driver_attach(struct device *dev, 
>> void *data)
>>  		return ret;
>>  	} /* ret > 0 means positive match */
>> 
>> -	if (dev->parent)	/* Needed for USB */
>> +	if (dev->parent &&
>> +		(dev->bus != &platform_bus_type))	/* Needed for USB */
> 
> The platform_bus_type check is not needed by USB, right?
> 
> thanks,
> 
> greg k-h

Re: [PATCH] driver-core: remove lock for platform devices during probe

[Greg Kroah-Hartman]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Mon, Apr 24, 2017 at 04:27:44PM +0800, weili@codeaurora.org wrote:
> Hi Greg,
> 
>    We are optimizing boot time for Linux kernel and try to make some
> platform drivers use asynchronous probe(by changing probe type of driver to
> PROBE_PREFER_ASYNCHRONOUS) to reduce boot time. However we found the
> platform drivers did not probe in parallel because they will lock the same
> parent device(platform bus for platform drivers) during probe.  So we add
> this patch to remove lock of parent for platform device. This will help to
> make platform driver probe in parallel and reduce boot time.

And does it really reduce boot time?  What are the numbers?  Why do you
have so many platform devices and not "real bus" devices?  What does the
boot graph look like when you run with and without this patch?  Why is
the platform bus so "special" to warrant this?  Should we perhaps make
this an option for any bus to enable/disable?

In other words, you need to provide a whole lot more information here,
justify why you are doing this type of change, and fix the issue I
pointed out, in order to get such a chance accepted.

Come on now, you know better than this...

greg k-h

Re: [PATCH] driver-core: remove lock for platform devices during probe

[weili]

Hi Greg K-H,

On 2017-04-24 16:46, Greg Kroah-Hartman wrote:

> And does it really reduce boot time? What are the numbers?
   Yes, it really reduce boot time. After making most time-consuming 
platform driver using async probe
   and also applying this patch, we see the driver run in parallel with 
others and saving 140ms.


> Why do you have so many platform devices and not "real bus" devices?
   We are working on an ARM soc. There are many host controllers 
implemented as platform devices.


> What does the boot graph look like when you run with and without this 
> patch?
   Without the patch, the boot graph is like this:
     CPU0: platform driver1 probe -> lock parent -> do probe staff -> 
unlock parent -> probe finish
     CPU1: platform driver2 probe ->                wait for lock on 
parent         -> lock parent -> do probe -> unlock parent -> probe 
finish

   With the patch, the boot graph is like this:
     CPU0: platform driver1 probe -> do probe staff -> probe finish
     CPU1: platform drvier2 probe -> do probe staff -> probe finish


> Why is the platform bus so "special" to warrant this?  Should we 
> perhaps make this
> an option for any bus to enable/disable?
   The lock on parent was first introduced by USB guys in following 
commit
   
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/drivers/base/dd.c?id=bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
   This may be useful for real bus devices such as USB and they think 
overhead of acquiring a lock is not large.
   But since platfrom bus is virtual, the lock is not necessary. Removing 
it for platform devices will make
   driver running in parallel and benefit boot time.

Best Regards
Wei

Re: [PATCH] driver-core: remove lock for platform devices during probe

[Greg Kroah-Hartman]

On Tue, Apr 25, 2017 at 04:43:33PM +0800, weili@codeaurora.org wrote:
> Hi Greg K-H,
> 
> On 2017-04-24 16:46, Greg Kroah-Hartman wrote:
> 
> > And does it really reduce boot time? What are the numbers?
>   Yes, it really reduce boot time. After making most time-consuming platform
> driver using async probe
>   and also applying this patch, we see the driver run in parallel with
> others and saving 140ms.

And why wasn't that information in the initial commit message?

And how much of a % is 140ms?  Why is a single driver taking that long
to initialize itself?

> > Why do you have so many platform devices and not "real bus" devices?
>   We are working on an ARM soc. There are many host controllers implemented
> as platform devices.

Don't you think that is also a problem?

> > What does the boot graph look like when you run with and without this
> > patch?
>   Without the patch, the boot graph is like this:
>     CPU0: platform driver1 probe -> lock parent -> do probe staff -> unlock
> parent -> probe finish
>     CPU1: platform driver2 probe ->                wait for lock on parent
> -> lock parent -> do probe -> unlock parent -> probe finish
> 
>   With the patch, the boot graph is like this:
>     CPU0: platform driver1 probe -> do probe staff -> probe finish
>     CPU1: platform drvier2 probe -> do probe staff -> probe finish

No, I mean the boot graph in pretty .svg format that the kernel can
output, with times and processes and everything.  Look in the tools
directory for more information, it will give you the exact timing for
your change before and after and show you exactly where you are taking
long periods of time.

You did use that, or something else to measure this somehow, right?

> > Why is the platform bus so "special" to warrant this?  Should we perhaps
> > make this
> > an option for any bus to enable/disable?
>   The lock on parent was first introduced by USB guys in following commit
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/drivers/base/dd.c?id=bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
>   This may be useful for real bus devices such as USB and they think
> overhead of acquiring a lock is not large.
>   But since platfrom bus is virtual, the lock is not necessary. Removing it
> for platform devices will make
>   driver running in parallel and benefit boot time.

I know all about USB here :)

You did not answer my questions :(

greg k-h

Re: [PATCH] driver-core: remove lock for platform devices during probe

[weili]

[-- Attachment #1: Type: text/plain, Size: 2947 bytes --]

Hi Greg K-H,

On 2017-04-25 19:36, Greg Kroah-Hartman wrote:
> On Tue, Apr 25, 2017 at 04:43:33PM +0800, weili@codeaurora.org wrote:
>> Hi Greg K-H,
>> 
>> On 2017-04-24 16:46, Greg Kroah-Hartman wrote:
>> 
>> > And does it really reduce boot time? What are the numbers?
>>   Yes, it really reduce boot time. After making most time-consuming 
>> platform
>> driver using async probe
>>   and also applying this patch, we see the driver run in parallel with
>> others and saving 140ms.
> 
> And why wasn't that information in the initial commit message?
> 
> And how much of a % is 140ms?  Why is a single driver taking that long
> to initialize itself?
The kernel took 1.72 seconds to boot to run the first init program. 
140ms is 8% improvement.
140ms is long for a single driver initialize. We are in discussion with 
the driver owner
about optimization.

>> > What does the boot graph look like when you run with and without this
>> > patch?
>>   Without the patch, the boot graph is like this:
>>     CPU0: platform driver1 probe -> lock parent -> do probe staff -> 
>> unlock
>> parent -> probe finish
>>     CPU1: platform driver2 probe ->                wait for lock on 
>> parent
>> -> lock parent -> do probe -> unlock parent -> probe finish
>> 
>>   With the patch, the boot graph is like this:
>>     CPU0: platform driver1 probe -> do probe staff -> probe finish
>>     CPU1: platform drvier2 probe -> do probe staff -> probe finish
> 
> No, I mean the boot graph in pretty .svg format that the kernel can
> output, with times and processes and everything.  Look in the tools
> directory for more information, it will give you the exact timing for
> your change before and after and show you exactly where you are taking
> long periods of time.
> 
> You did use that, or something else to measure this somehow, right?
> 
The boot graph is in the attachment. The function msm_sharedmem_init 
took
long time because it is blocked by another async probe driver. After
applying the patch, msm_sharedmem_init is no longer blocked.

>> > Why is the platform bus so "special" to warrant this?  Should we perhaps
>> > make this
>> > an option for any bus to enable/disable?
>>   The lock on parent was first introduced by USB guys in following 
>> commit
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/drivers/base/dd.c?id=bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
>>   This may be useful for real bus devices such as USB and they think
>> overhead of acquiring a lock is not large.
>>   But since platfrom bus is virtual, the lock is not necessary. 
>> Removing it
>> for platform devices will make
>>   driver running in parallel and benefit boot time.
> 
> I know all about USB here :)
> 
> You did not answer my questions :(
> 
Do you suggest that we add some varible like "async_probe" in struct 
bus_type and
then check the varible during probe to decide whether to lock the 
parent?

Best Regards
Wei

[-- Attachment #2: boot_before_patch.svg --]
[-- Type: image/svg+xml, Size: 35036 bytes --]

[-- Attachment #3: boot_after_patch.svg --]
[-- Type: image/svg+xml, Size: 36882 bytes --]

Re: [PATCH] driver-core: remove lock for platform devices during probe

[Greg Kroah-Hartman]

On Tue, May 02, 2017 at 10:18:25AM +0800, weili@codeaurora.org wrote:
> Hi Greg K-H,
> 
> On 2017-04-25 19:36, Greg Kroah-Hartman wrote:
> > On Tue, Apr 25, 2017 at 04:43:33PM +0800, weili@codeaurora.org wrote:
> > > Hi Greg K-H,
> > > 
> > > On 2017-04-24 16:46, Greg Kroah-Hartman wrote:
> > > 
> > > > And does it really reduce boot time? What are the numbers?
> > >   Yes, it really reduce boot time. After making most time-consuming
> > > platform
> > > driver using async probe
> > >   and also applying this patch, we see the driver run in parallel with
> > > others and saving 140ms.
> > 
> > And why wasn't that information in the initial commit message?
> > 
> > And how much of a % is 140ms?  Why is a single driver taking that long
> > to initialize itself?
> The kernel took 1.72 seconds to boot to run the first init program. 140ms is
> 8% improvement.
> 140ms is long for a single driver initialize. We are in discussion with the
> driver owner
> about optimization.

Yes, please fix that.

> > > > What does the boot graph look like when you run with and without this
> > > > patch?
> > >   Without the patch, the boot graph is like this:
> > >     CPU0: platform driver1 probe -> lock parent -> do probe staff ->
> > > unlock
> > > parent -> probe finish
> > >     CPU1: platform driver2 probe ->                wait for lock on
> > > parent
> > > -> lock parent -> do probe -> unlock parent -> probe finish
> > > 
> > >   With the patch, the boot graph is like this:
> > >     CPU0: platform driver1 probe -> do probe staff -> probe finish
> > >     CPU1: platform drvier2 probe -> do probe staff -> probe finish
> > 
> > No, I mean the boot graph in pretty .svg format that the kernel can
> > output, with times and processes and everything.  Look in the tools
> > directory for more information, it will give you the exact timing for
> > your change before and after and show you exactly where you are taking
> > long periods of time.
> > 
> > You did use that, or something else to measure this somehow, right?
> > 
> The boot graph is in the attachment. The function msm_sharedmem_init took
> long time because it is blocked by another async probe driver. After
> applying the patch, msm_sharedmem_init is no longer blocked.

Why isn't the boot graph showing any parallel tasks?  I thought it
would.

> > > > Why is the platform bus so "special" to warrant this?  Should we perhaps
> > > > make this
> > > > an option for any bus to enable/disable?
> > >   The lock on parent was first introduced by USB guys in following
> > > commit
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/drivers/base/dd.c?id=bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
> > >   This may be useful for real bus devices such as USB and they think
> > > overhead of acquiring a lock is not large.
> > >   But since platfrom bus is virtual, the lock is not necessary.
> > > Removing it
> > > for platform devices will make
> > >   driver running in parallel and benefit boot time.
> > 
> > I know all about USB here :)
> > 
> > You did not answer my questions :(
> > 
> Do you suggest that we add some varible like "async_probe" in struct
> bus_type and then check the varible during probe to decide whether to
> lock the parent?

You don't want to do this for all platform devices, things will break,
we found this out a long time ago when we tried to make everything init
in parallel.  So you are going to have to do a lot of testing on lots of
platforms...

thanks,

greg k-h