Re: [PATCH 01/13] PM: Add wake lock api.

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday 09 February 2009, Arve Hjønnevåg wrote:
> On Sun, Feb 8, 2009 at 4:15 PM, Rafael J. Wysocki <rjw@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.

Well, I don't.  In fact I think it's too complicated.

> >> >> 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.

Wakelocks with timeout are not a solution IMO.

> >> > 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 really think we need it in the mainline kernel.

> I don't see what a per-task flag would be helpful with. Not all wakelocks are
> associated with tasks.

What are they associated with, then?

> >
> >> >> >> +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.

It would hide the real invasiveness of the changes you'd like to make, which
need not be a good thing.

> >> > 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.

Well, you put quite a lot of effort into making this nicely debuggable and so
on, but I think you should have submitted the minimal core functionality first
to see if people were comfortable with it.

Thanks,
Rafael