On Monday 09 February 2009, Arve Hjønnevåg wrote: > On Sun, Feb 8, 2009 at 4:15 PM, Rafael J. Wysocki <rjw@xxxxxxx> 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 _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
