User: Password:
Subscribe / Log in / New account



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

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 :)

(Log in to post comments)


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 :-)

Copyright © 2017, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds