Wakelocks

Posted Dec 23, 2011 1:52 UTC (Fri) by neilbrown (subscriber, #359)
In reply to: Wakelocks by mjg59
Parent article: Bringing Android closer to the mainline

You missed a part of the sequence.

When user-space hits a key the keyboard driver activates a wakeup-source. This has the effect of causing the suspend request to abort (yes, this can be done race-free). The CPU says awake, the keystroke is responded to, userspace notices that the user is doing something and arranges not to go to sleep just yet (user may still throw phone out the window, but it is probably because they hit the wrong key by mistake).

And as for "do the necessary work": Please see https://lwn.net/Articles/464144/. Just as soon as I can get my new phone suspending reliably I'll be testing this is a real-life situation.

Wakelocks

Posted Dec 23, 2011 12:36 UTC (Fri) by mjg59 (subscriber, #23239) [Link] (12 responses)

Right, but other than precise implementation details this is still equivalent to wakelocks (the main thing being that userspace has to repeatedly ask the kernel to go to sleep rather than it just happening automatically). It's no more elegant, and if you want it to carry on working in all the cases that wakelocks do then you still need to have timeout-based wakeup sources sprinkled throughout random bits of your code.

Wakelocks

Posted Dec 23, 2011 19:42 UTC (Fri) by neilbrown (subscriber, #359) [Link] (11 responses)

I won't try to argue elegance - that is in the eye of the beholder. The point really is existence. This exists in mainline Linux today so the response to "we need wakelocks" is actually "we have them".

They haven't been tested heavily and there is no demonstration that they are sufficient for Androids needs so it my mind, that is the next step. I would like to see android running on a mainline kernel (which is nearly possible with what it in staging in -next) and then replace little bits so that it uses mainline-preferred functionality in place of Android-specific functionality and see how it works. Then we could move on from "you should do it like this" arguments - which never work - to "I have done it like this" arguments which tend to hold a lot more water.

Timeout-based wakeup sources are only needed when interacting with an external device (such as a human or a network switch) and in that case they are always needed. e.g. if you receive a wake-on-lan magic-packet you need to assume there will be real data soon but you cannot wait forever so you need a timeout. Similarly a key stroke is usually followed by another so stay awake for it. These sorts of things always need timeouts. You should need no more or fewer timeout-based wakeup sources than you would need equivalent timeouts with Android wakelocks.

Wakelocks

Posted Dec 23, 2011 19:57 UTC (Fri) by mjg59 (subscriber, #23239) [Link] (10 responses)

That's not quite it. The timeouts aren't there because there may be more data coming - the timeouts are there because for most things more complicated than the input case, you don't know whether or not userspace has consumed the event. That means that on Android you have little things like 5 second timeouts in the MMC code purely to handle the case that there's no easy way to detect whether userspace is done dealing with the SD card that you just inserted or not, or (as in your case) how long it'll take some magic packet you received via wifi to bubble all the way up to userspace. It's difficult to see these kernel-level timeouts being acceptable.

You can defer the timeouts to userspace instead, but then your userspace has to be amazingly aware of how everything ties together - rather than having generic userspace, you now need userspace that knows every single wakeup event that the kernel may generate, and whether or not it needs to take a timeout on them. That's more complicated than the existing Android case.

I'm not arguing that the wakeup events code is bad or evil or does nothing to get us closer to a model where Android works, just that right now it only solves the simple cases and doesn't have a clear strategy for solving the difficult ones.

Wakelocks

Posted Dec 23, 2011 21:08 UTC (Fri) by neilbrown (subscriber, #359) [Link] (3 responses)

This is how I would handle your SD card example.

The card-detect interrupt activates a wakeup_source which is then deactivated once the insertion is visible to user-space. I'm guessing that is just after the kobject_uevent() call which reports a 'change' or 'add'.

The user-space suspend daemon has a 'prepare' and a 'confirm' stage to requesting suspend. A wakeup_source activation after 'prepare' will cause 'confirm' to fail. Between these two stages it would do something like "udevadm settle". This would ensure that any kobject_uevent had actually been handled.

The udev handler for "sd card insertion" would fsck and mount, scan for viruses and new media files and sends a dbus signal to the UI. Then exit.

The UI would notice the signal, tell the suspend-daemon that it is busy, acknowlege the signal (thus allowing udev handler to exit), and then pop up a widget saying "new music and viruses available - which should I play?" and wait for the user to respond (or timeout).

So: no timeout needed in the kernel or at all until you get to user interaction. Also the user-space side seems entirely generic.

The kernel doesn't need to know when user-space consumes the event, only when the event is visible to user-space.

The suspend-daemon *can* know if user-space has consumed the event because it can ask every user-space subsystem "Have you consumed all your events", and the prepare/commit makes this race-free. It sounds expensive to ask everyone that question but I believe it can be done quiet efficiently. e.g. at start up the subsystem says "here is an fd - you just check it yourself, don't bother me". Then "ask everyone" becomes a single "select" system call.

If you have other examples that you think are more complicated I would love to hear them - I have little practical experience in this area and would love to benefit from the experience of others.

Thanks.

Wakelocks

Posted Dec 23, 2011 21:24 UTC (Fri) by mjg59 (subscriber, #23239) [Link] (1 responses)

Ok, so you take a wakeup source on card insertion, and then the uevent code has support for releasing the source when an event has been sent. You'd presumably need some sort of token to ensure that the event sent corresponds to the wakeup source. Userspace then attempts another suspend, and calls the prepare() method for the mounting agent. Either that or the confirm() method (or both) verifies that the event queue is empty (presumably by making sure that there's no active fds in a set it's poll()ing on, or something) and then the suspend is attempted.

If an event occurs between confirm() being run but before the suspend is requested, you'd miss that. So the suspend needs to then fail, but that's ok if you've got a token from before you called prepare() and that token's been invalidated by another wakeup event in the intervening time. So it does sound like this could be made race-free, but it does involve adding some extra complexity to the uevent code. That's not an obviously bad idea in any case - it'd be a useful thing to have for avoiding races in the existing suspend path.

Now, how about the network case? It ought to be possible in much the same way (you'd deactivate the source when the packet has been read), but there's a pretty huge distance between an irq from a network device (where you have to activate the source just in case the packet is magic, which you don't yet know) and userspace, and it could get lost at various points in between - so you need to map every path that it could take.

This is all obviously possible, and I think your approach could certainly be made to work. But making it work well is going to involve a lot of code. Not a criticism, just an observation!

This is one of the reasons why a scheduler-based approach is more attractive in many ways - you avoid the kernel timeouts problem without having to add a huge amount of kernel complexity. It obviously does result in a bunch of additional problems on the userspace side, many of which could probably be mitigated with timer slack - but I'm obviously speaking in a purely hypothetical sense, since I haven't found the time to actually do the work necessary to make that workable. You're actually doing the work, so you get to ignore me here :)

Wakelocks

Posted Dec 23, 2011 22:10 UTC (Fri) by neilbrown (subscriber, #359) [Link]

The "token" is a "struct wakeup_source". You can embed these where ever you like. Alternatively you can use a string name and "wakeup_source_register" will create on for you.

I don't think the uevent code would be changed.

I think for the sd card case there would be two wakeup sources - one for the MMC interface and one for the block device. So on card-detect the MMC interface activates its wakeup_source. It then inspects the card, figures out that it is a storage device, calls mmc_blk_probe, then releases its wakeup_source. mmc_blk_probe would activate its wakeup_source, do whatever it does, ultimately call add_disk (which will make the uevent visible to userspace) and then release the mmc_blk wakeup_source. So any driver in a wakeup chain needs a wakeup_source and needs to activate/deactivate as appropriate.

I don't know the network code very well so I'm guessing more than usual here, but I think there is an 'skbuf' that travels from device to socket queue. I imagine a single system-wide wakeup_source and a flag in the skbuf to say if it is active (a wakeup_source has a counter and so can be shared). Device sets it when creating an skbuf if wake-up is enabled and socket code clears it when it is added to a queue (or when the skbuf is discarded).

A network device might want a separate wakeup source if there is a gap between the interrupt happening and the packet being read from the network device.

But of course the devil is in the detail, and adding anything to fast-paths in the network code is a not an option so we would need to be very careful.

One step at a time :-)

Wakelocks

Posted Dec 24, 2011 15:42 UTC (Sat) by ndye (guest, #9947) [Link]

. . . then pop up a widget saying "new music and viruses available - which should I play?" and wait . . .

First, thanks for the well-toned humor.

And in the midst of a non-childish conversation . . . made my day!

So long, Merry Christmas, and thanks for all the fish!

Wakelocks

Posted Dec 23, 2011 23:14 UTC (Fri) by dlang (guest, #313) [Link] (5 responses)

the problem is that userspace wakelocks don't solve the problem you are presenting either.

at least not unless every app can take them, but that's not what happens in android, only 'trusted' apps are allowed to take the wakelock.

in android they still use timeouts for most of userspace consuming events, and sidestep a huge amount of it by having the power controller have a wakelock for the entire time the screen backlight is on so that you don't have to scatter wakelocks through every app.

there are many of us who think that there are better ways of dealing with this userspace problem, but the problems of doing our own kernels makes it hard to experiment.

Wakelocks

Posted Dec 23, 2011 23:25 UTC (Fri) by neilbrown (subscriber, #359) [Link] (4 responses)

I don't quite follow - what exactly is the barrier that you see to experimentation?

Wakelocks

Posted Dec 24, 2011 6:20 UTC (Sat) by dlang (guest, #313) [Link] (3 responses)

the fact that I can't easily use the features that are in a kernel.org kernel with android userspace is one big barrier.

even if I am trying to compile my own android kernel, there doesn't seem to be any way of installing it on a device in a way that will let me fall back to the older kernel if I run into problems with the new one (the equivalent to the old lilo -R option)

the integration of things into the mainline will greatly help the first problem, but I still don't know a good solution for the second one.

Wakelocks

Posted Dec 24, 2011 11:50 UTC (Sat) by mjg59 (subscriber, #23239) [Link]

If you hit problems you just drop back to recovery and flash the old kernel back. Alternatively, do your testing using fastboot to push the kernel and ramdisk over USB and don't touch your flash at all.

Wakelocks

Posted Dec 24, 2011 12:30 UTC (Sat) by Cyberax (✭ supporter ✭, #52523) [Link]

You can use kexec, but I don't bother. On Galaxy S phones it's always possible to drop into "download mode" and just restore the old kernel image.

It's _always_ possible even if your firmware is totally corrupted and even battery controller is dead. In some cases it requires a magic jig, though (a 101kOhm resistor shorting two USB lines).

Wakelocks

Posted Mar 19, 2012 14:14 UTC (Mon) by jimmyflip (guest, #83547) [Link]

It's hard to imagine anyone doing even a small amount of Android kernel development wouldn't use 'fastboot boot' to test their kernel (assuming they have a capable bootloader). Any problems, just reboot.