Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630309 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] driver-core: remove lock for platform devices during  probe weili@codeaurora.org - 2017-04-25 10:50 +0200
    Re: [PATCH] driver-core: remove lock for platform devices during  probe Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-04-25 13:50 +0200
      Re: [PATCH] driver-core: remove lock for platform devices during  probe Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-02 20:40 +0200

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

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

[toc] | [next] | [standalone]

#1630428

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

[toc] | [prev] | [next] | [standalone]

#1634568

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web