[PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

[PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

This patch adds a quick check for the driver<->device match before taking
the locks and doin gthe expensive checks. Taking the lock hurts in asynchronous
boot context where the device lock gets hit; one of the init functions takes
the lock and goes to do an expensive hardware init; the other init functions
walk the same PCI list and get stuck on the lock as a result.

For the common case, we can know there's no chance whatsoever of a match
if the device isn't in the drivers ID table... so this patch does that check
as a best-effort-avoid-the-lock approach.

Bootcharts for before and after can be seen at
http://www.fenrus.org/before.svg
http://www.fenrus.org/after.svg

Note the long time "agp_ali_init" takes in the first graph; my laptop doesn't
even have an ALI chip in it!
(the bootgraphs look a bit dissimilar, but that's the point, the first one has a bunch
of arbitrary delays in it that cause it to look very different)

This reduces my kernel boot time by about 20%

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/base/dd.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 3ac443b..20febc0 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -257,6 +257,9 @@ static int __driver_attach(struct device *dev, void *data)
 	 * is an error.
 	 */
 
+	if (drv->bus->match && !drv->bus->match(dev, drv))
+		return 0;
+
 	if (dev->parent)	/* Needed for USB */
 		down(&dev->parent->sem);
 	down(&dev->sem);
-- 
1.5.5.1



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

[Cornelia Huck]

On Sun, 14 Sep 2008 08:32:06 -0700,
Arjan van de Ven <arjan@infradead.org> wrote:

> This patch adds a quick check for the driver<->device match before taking
> the locks and doin gthe expensive checks. Taking the lock hurts in asynchronous
> boot context where the device lock gets hit; one of the init functions takes
> the lock and goes to do an expensive hardware init; the other init functions
> walk the same PCI list and get stuck on the lock as a result.

Hm, you call bus->match twice now; once without dev->sem held and once
with it. For the busses I'm familiar with that shouldn't be a problem,
but are you sure there aren't busses which want dev->sem held?
(Although I think not relying on dev->sem would be the sane thing...)
> 
> For the common case, we can know there's no chance whatsoever of a match
> if the device isn't in the drivers ID table... so this patch does that check
> as a best-effort-avoid-the-lock approach.

I've always thought of ->match being a quick check which just looks at
the IDs with ->probe doing the heavier stuff, so this should be
reasonable (if all busses play nicely). But driver_probe_device() still
calls ->match a second time, and device_attach() will thus always call
->match under the lock. Should it be moved out of the lock there as
well?

Re: [PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

[Arjan van de Ven]

On Mon, 15 Sep 2008 13:32:26 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Sun, 14 Sep 2008 08:32:06 -0700,
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > This patch adds a quick check for the driver<->device match before
> > taking the locks and doin gthe expensive checks. Taking the lock
> > hurts in asynchronous boot context where the device lock gets hit;
> > one of the init functions takes the lock and goes to do an
> > expensive hardware init; the other init functions walk the same PCI
> > list and get stuck on the lock as a result.
> 
> Hm, you call bus->match twice now; once without dev->sem held and once
> with it. For the busses I'm familiar with that shouldn't be a problem,
> but are you sure there aren't busses which want dev->sem held?
> (Although I think not relying on dev->sem would be the sane thing...)

As far as I can see it's ok, but if not I obviously like to hear about
it SOON :)


> > 
> > For the common case, we can know there's no chance whatsoever of a
> > match if the device isn't in the drivers ID table... so this patch
> > does that check as a best-effort-avoid-the-lock approach.
> 
> I've always thought of ->match being a quick check which just looks at
> the IDs with ->probe doing the heavier stuff, so this should be
> reasonable (if all busses play nicely). But driver_probe_device()
> still calls ->match a second time, and device_attach() will thus
> always call ->match under the lock. Should it be moved out of the
> lock there as well?

having a second check is actually not a bad thing per se; in terms of
programming pattern, doing the quick checks before the lock, but doing
the final check inside the lock makes sense to me. If there's real
objections to doing the match the second time (it's cheap!) I'll remove
it, but this way, you can call the "heavy" function always and from
anywhere, and it'll just do the right thing no matter what. I kinda like
that as concept ;)



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] device model: Do a quickcheck for driver binding before doing an expensive check

[Cornelia Huck]

On Mon, 15 Sep 2008 07:06:44 -0700,
Arjan van de Ven <arjan@infradead.org> wrote:

> On Mon, 15 Sep 2008 13:32:26 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Sun, 14 Sep 2008 08:32:06 -0700,
> > Arjan van de Ven <arjan@infradead.org> wrote:
> > 
> > > This patch adds a quick check for the driver<->device match before
> > > taking the locks and doin gthe expensive checks. Taking the lock
> > > hurts in asynchronous boot context where the device lock gets hit;
> > > one of the init functions takes the lock and goes to do an
> > > expensive hardware init; the other init functions walk the same PCI
> > > list and get stuck on the lock as a result.
> > 
> > Hm, you call bus->match twice now; once without dev->sem held and once
> > with it. For the busses I'm familiar with that shouldn't be a problem,
> > but are you sure there aren't busses which want dev->sem held?
> > (Although I think not relying on dev->sem would be the sane thing...)
> 
> As far as I can see it's ok, but if not I obviously like to hear about
> it SOON :)

I don't see any problem on the s390 busses, and pci and usb look OK as
well at a glance.

> 
> 
> > > 
> > > For the common case, we can know there's no chance whatsoever of a
> > > match if the device isn't in the drivers ID table... so this patch
> > > does that check as a best-effort-avoid-the-lock approach.
> > 
> > I've always thought of ->match being a quick check which just looks at
> > the IDs with ->probe doing the heavier stuff, so this should be
> > reasonable (if all busses play nicely). But driver_probe_device()
> > still calls ->match a second time, and device_attach() will thus
> > always call ->match under the lock. Should it be moved out of the
> > lock there as well?
> 
> having a second check is actually not a bad thing per se; in terms of
> programming pattern, doing the quick checks before the lock, but doing
> the final check inside the lock makes sense to me. If there's real
> objections to doing the match the second time (it's cheap!) I'll remove
> it, but this way, you can call the "heavy" function always and from
> anywhere, and it'll just do the right thing no matter what. I kinda like
> that as concept ;)

OK, you have a point. I just find it a bit ugly; especially as the
->probe function will check if the device matches as well (by poking at
the device).

But I'd be fine with either way :)