[linux-pm] [PATCH 01/13] PM: Add wake lock api.

[Arve Hjønnevåg]

On Sun, Feb 8, 2009 at 4:15 PM, Rafael J. Wysocki <rjw at sisk.pl> wrote:
>> >> It means that you can, without knowing the current state of the lock, safely
>> >> call wake_lock when you know that you need the wakelock, and
>> >> wake_unlock when you are done.
>> >
>> > You can do the same with a reference counter IMO.
>>
>> Not without some per lock state.
>
> What prevents me from increasing the reference counter when necessary and
> decreasing it when I'm done, actually?

Nothing prevents a driver from incrementing and decrementing a
reference count correctly, but it has to know if it already has a
reference. If you want to implement my current wake_lock api with a
global reference count, you need some per lock state to indicate if
the lock is locked or not.

>> >> It allows wakelocks with timeouts
>> >
>> > OK
>> >
>> > What for?
>>
>> So we do not have to change everything at once, and so we can allow
>> code that we do not really trust a chance to run.
>
> Well, are you sure this is the best way of achieving this goal?

No, but I have not seen any better suggestions, that solve the problem.

>> > However, it might be better to use a refcount along with some mechanism
>> > allowing user space processes to increase it only once before decreasing.
>> > That would require a per-task flag, but I think you can reuse one of the
>> > freezer flags for that (the freezer is not going to run as long as a wakelock
>> > is held, right?).
>>
>> How would this be better?
>
> Simpler code, less overhead.  But I'm not insisting, just showing you another
> possible approach.

I agree that a global reference count is less overhead, but we need
timeout support the make the system work for now. I don't see what a
per-task flag would be helpful with. Not all wakelocks are associated
with tasks.

>
>> >> >> +This works whether the wakelock is already held or not. It is useful if the
>> >> >> +driver woke up other parts of the system that do not use wakelocks but
>> >> >> +still need to run. Avoid this when possible, since it will waste power
>> >> >> +if the timeout is long or may fail to finish needed work if the timeout is
>> >> >> +short.
>> >> >
>> >> > And what's this call needed for?
>> >
>> > Please don't remove the context.  This makes reading your reply difficult.
>>
>>  +It can also call wake_lock_timeout to release the wakelock after a delay:
>>  +     wake_lock_timeout(&state->wakelock, HZ);
>>  +
>>
>> >
>> >> It is needed to give code that do not use wakelocks a chance to run.
>> >> For instance we have not added wakelocks to the network stack. We also
>> >> use it in some places when passing data to untrusted userspace code.
>> >
>> > Do you mean to take a wakelock with a timeout in one code path so that some
>> > other code path can run knowing that suspend will not happen?
>> >
>> > If this is correct, I don't like it, because it's inherently unreliable (you
>> > really never know how long it will take the other code path to run, so you
>> > can't choose the timeout 100% accurately).
>>
>> I agree, but without wakelock support you have two options. Do not
>> user suspend at all, or use a global timeout that you reset everywhere
>> we lock a wakelock. By using wakelocks with timeouts we can at least
>> make some events 100% reliable.
>
> I'm not convinced.
>
> Sorry to say that, but IMO the "wakelock with timeout" mechanism looks more
> like a (poor) workaround than a solution of the problem.  Surely, I woulnd't
> have tried to introduce anything similar into the kernel.

I do not want to add wakelocks to every kernel subsystem while there
is no wakelock support in the mainline kernel. Using wakelocks with
timeouts allow us to make progress.

>> >
>> > I really would use a refcount and make sure it's increased (and obviously
>> > decreased) by _every_ code path that needs to prevent suspend from happening,
>> > including networking and so on.
>> >
>> >> > Is there a mechanism allowing us to see what wakelocks have been created by
>> >> > the user land?  Something like this would be useful for debugging.
>> >>
>> >> /proc/wakelocks shows all wakelocks. It does not currently indicate if
>> >> a wakelock is from userspace, but if you are looking for a specific
>> >> lock it is easy to find. I can add a prefix to the name of all user
>> >> space wakelocks of you want.
>> >
>> > Well, actually, do the wakelocks have to have the names?
>>
>> If you want to debug the system or provide stats, yes.
>
> Care to elaborate?

We report the name and how long each wakelock is active in
/proc/wakelocks. Without a name, those stats are not very useful. You
can also enable debug options to print a message the kernel log every
time a wakelock is locked, unlocked or used to trigger a wakeup.

-- 
Arve Hjønnevåg