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

[Charlie Shepherd]

On 07/08/2013 20:30, Stefan Hajnoczi wrote:
> 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.

I see, so in that case coroutine_fn is more a contract saying "this 
function can only be, and is only, called from a coroutine context"? 
That feels fair enough, but I'll amend the comment in coroutine.h to 
that effect.


Charlie