Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations

[Stefan Hajnoczi]

On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> On 05/08/2013 20:23, Gabriel Kerneis wrote:
> >On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> >>diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> >>index f133d65..d0ab27d 100644
> >>--- a/include/block/coroutine_int.h
> >>+++ b/include/block/coroutine_int.h
> >>@@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
> >>  void qemu_coroutine_delete(Coroutine *co);
> >>  CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
> >>                                        CoroutineAction action);
> >>-void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
> >>+void qemu_co_queue_run_restart(Coroutine *co);
> >>  #endif
> >Adding coroutine_fn where it is necessary seems straightforward to me: just
> >follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming 
> >you
> >got it right and did not over-annotate). On the other hand, you
> >should probably explain in the commit message why you *remove* those three
> >coroutine_fn annotations.
> 
> Yes that does merit some explanation.
> 
> Building the tree with cps inference warned that these functions
> were annotated spuriously. I initially thought this was because they
> called a coroutine function that hadn't been annotated, but it seems
> the *_handler functions called qemu_aio_set_fd_handler which, from
> my investigation, it seems does not need annotating. Therefore they
> were indeed spuriously annotated and so I removed the annotation.
> 
> qemu_co_queue_run_restart is a bit different. It is only called from
> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> were waiting but have now moved to the runnable state by the actions
> of the most recent coroutine (I believe). I think we discussed this
> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> yield, so again I removed the annotation. I'll add these
> explanations to the commit message.

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.

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.

The thing is, these leaf functions are typically only called from
coroutine context anyway.  I think they should be left marked
coroutine_fn.

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.

Stefan