On Fri, Feb 27, 2009 at 3:44 PM, Pavel Machek <pavel@xxxxxx> wrote: > Hi! > >> >> > > Wakelocks done right are single atomic_t... and if you set it to 0, >> >> > > you just unblock "sleeper" thread or something. Zero polling and very >> >> > > simple... >> >> > >> >> > Except that you have to check all of the wakelocks periodically in a loop => >> >> > polling. So? >> >> >> >> No. I want to have single atomic_t for all the wakelocks... at least >> >> in non-debug version. Debug version will be slower. I believe you >> >> originally suggested that. >> > >> > I did, but please don't call it "wakelocks". It's confusing. >> >> What you are talking about here is mostly an optimization of the >> wakelock api. You have removed timeout support and made each wakelock >> reference counted. If you ignore wakelocks with timeouts, the current >> wakelock interface can be implemented with a global atomic_t to >> prevent suspend, and a per wakelock atomic_t to prevent a single >> client from changing the global reference count by more than one. > > Actually I'd go to global atomic_t to prevent suspend, and make sure > single client only changes it once by design. If you go with only a global atomic_t then you automatically allow clients to change the count by more than one. Our userland api calls this a reference counted wakelock, and reference counted wakelocks have been a frequent source of bugs. We have code that calls wake_lock every time an item is added to a queue and wake_unlock when it gets a notification that item was removed. If you miss a notification the lock is never released. With non reference counted wakelocks, can unlock every time the queue is empty, and lock either when adding the first item or every time you add an item (useful when combined with timeouts). I could extend the wakelock api to allow reference counted wakelocks, but I have not come across any drivers where it would be beneficial (to the driver, not the wakelock implementation), and causes conflicts with timeouts. >> There are a couple of reasons that I have not done this. It removes >> the ability to easily inspect the system when it is not suspending. I >> do provide an option to turn off the wakelock stats, which makes >> wake_lock/unlock significantly faster, but we never run with wakelock >> stats off. > > It seems wrong to design for the "debugging" case. ... even if you run > with debugging enabled. I'm not sure I would call wakelock stats purely a debugging feature. Either way, designing the api to pass in an object, allows stats and debugging features to be implemented. Designing the api to be a global reference count without an object prevents both. I realize that some of the proposed alternatives allow the same stats and debugging information to be added to other kernel objects (tasks and devices), but I prefer a separate wakelock object. We have drivers that use more then one wakelock per device, and many wakelocks do not have any association with a task either. > >> Also, it pushes timeout handling to the drivers. I know >> may >> of you don't like timeout support, but ignoring the problem is not a >> solution. If each driver that needs timeouts uses its own timer, then >> you will often wakeup from idle just to unlock a wakelock that will >> not trigger suspend. This wakeup is a thousand times as costly on the >> msm platform as a wakelock/unlock pair (with wakelock stats >> enabled). > > Well, you are free to have a library (or something) those broken > drivers can use. But you have to understand that those drivers are > broken... and please don't make it part of core API. I don't agree. Not all the timeout uses are broken. The unknown-wakeup wakelock that is used if suspend fails preserved compatibility with existing drivers without introducing any race conditions. I also use a wakelock with a one second timeout if the alarm driver is suspended less than a second before the alarm is supposed to expire. I could probably rewrite this driver to not use a timeout, but the current implementation is not broken. > It should also make the merging easier; merge non-contraversial parts > first (single atomic_t), then add debugging infrastructure, then maybe > add timeout handling if those drivers can really not be fixed in a > nicer way... Why is a single atomic_t less controversial? It is just as invasive as the wakelock interface and it provides no help in interacting with unmodified subsystems. I'm more interested in getting code merged that can easily be used in a fully functional system, than code that only works for test systems, but if the implementation details of the wakelock interface is the only thing that prevents it from being merged, I can change it. I do find it odd that this is what is objectionable though, as it is already more efficient than the pm_qos interface that people want us to use instead of idle wakelocks. -- Arve Hjønnevåg _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
