On Wed, Aug 07, 2013 at 09:30:25PM +0200, Stefan Hajnoczi wrote: > I have mixed feelings about removing coroutine_fn annotations from a > function when it does not yield or call other coroutine_fn functions. > > These functions were probably written as part of a coroutine code path. > The coroutine_fn annotation tells me I'm in coroutine context.
No, it tells you it is forbidden to call this function from outside coroutine context. Which is false: if the function never yields, it is definitely correct to call from somewhere else (unless there is some other invariant in qemu about coroutine context?). > By removing this information those modifying the code now need to > convert it back to coroutine_fn after auditing callers before they can > use coroutine context. I don't understand this. You mean someone who, later, would decide to make a version of the function that yields? In that case, wouldn't it make sense to introduce an alternative coroutine_fn counter-part? (I see, however, how line of reasoning might have led to the "dynamic functions" maze). > I'd compare this to a comment that says "lock foo is held across this > function" but the function doesn't use anything that lock foo protects. > Removing the comment isn't really helpful, you are throwing away > information that can be useful when modifying the function. Except that the function might also makes sense outside of coroutine context, and you are forcing people to allocate a spurious coroutine if they want to use it. Note that I'm arguing for the sake of defining precisely what Qemu developpers expect when they read or write "coroutine_fn". As long as we can agree on that point, and get it documented in coroutine.h, I'm fine. From a performance point-of-view, it matters only for the CPC backend and there aren't that many spuriously-annotated functions anyway. We could even benchmark the overhead at the end of the GSoC, but I don't expect it to be significant (if there were dozens of them, it would be a different story). Best, -- Gabriel
