Re: Async suspend-resume patch w/ completions (was: Re: Asyncsuspend-resume patch w/ rwsems)

[Linus Torvalds]

On Fri, 11 Dec 2009, Rafael J. Wysocki wrote:
> 
> But fine, say we use the approach based on rwsems and consider suspend and
> the inner lock.  We acquire it using down_write(), because we want to wait for
> multiple other dirvers.  Now, in fact we could do literally
> 
> down_write(dev->power.rwsem);
> up_write(dev->power.rwsem);
> 
> because the lock doesn't really protect anything from anyone.  What it does is
> to prevent _us_ from doing something too early.  To me, personally, it's not a
> usual use of locks.

I agree that it's fairly unusual, but on the other hand, it's unusual only 
because you contrieved it to be.

If you instead do

	down_write(dev->power.rwsem);
	.. do the actual suspend ..
	up_write(dev->power.rwsem);

it doesn't look odd any more, does it? And while you don't _need_ to hold 
the power lock over the suspend call, it actually does make sense, and 
gives you some nicer guarantees.

For an example of the kinds of guarantees it would give you - I think that 
you might actually be able to do a partial suspend and then a resume 
without any other locks, and you'd know that just the per-device locking 
would already guarantee that no device is ever tried to resume before it 
has finished its asynchronous suspend.

Think about it.

In the completion model, the "async_synchronize_full()" will synchronize 
all async work, and as a result you think that you don't need that level 
of robustness from the locking itself.

But think about it this way: if you could abort a failed suspend, and 
start resuming devices immediately, without doing that 
"async_synchronize_full()" in between - simply because you know that the 
node locking itself will just "do the right thing".

To me, that's a sign of a _good_ design. Using a rwsem is simply just more 
robust and natural for the problem in question. Exactly because it's a 
real lock.

> > Don't try to make up problems. The _only_ subsystem we know wants this is 
> > USB, and we know USB is purely a tree.
> 
> Not really.
> 
> I've already said it once, but let me repeat.  Some device objects have those
> ACPI "shadow" device objects that represent the ACPI view of given "physical"
> device and have their own suspend and resume routines.  It turns out that
> these ACPI "shadow" devices have to be suspended after their "physical"
> counterparts and resumed before them, or else things beak really badly.
> I don't know the reason for that, I only verified it experimentally (I also
> don't like that design, but I didn't invent it and I have to live with it at
> least for now).  So if we don't enforce these constraints doing async
> suspend and resume, we won't be able to handle _any_ devices with those
> ACPI "shadow" things asynchronously.  Ever.  [That includes the majority
> PCI devices, at least the "planar" ones (which is unfortunate, but that's how
> it goes).]

So?

First off, you're wrong. It's not "ever". I'm happy to add complexity 
later, I just don't want to start out with a complex model. Adding 
complexity too early "just because we migth need it" is the wrong thing to 
do.

Secondly, I repeat: we don't want to do those PCI devices asynchronously 
anyway. You're again digging yourself deeper by just continually bringing 
up this total non-issue. I realize you did it for testing, but I'm serious 
when I say that we should limit these things as much as possible, rather 
than see it as an opportunity to do crazy things.

Solve the problem at hand _first_. Solve it as simply as you can. And hope 
that you never ever will need anything more complex.

				Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/