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

[Charlie Shepherd]

On 05/08/2013 20:23, Gabriel Kerneis wrote:
> On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> > This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of 
> > struct BlockDriver
> > to be explicitly annotated as coroutine_fn, rather than yielding dynamically 
> > depending on whether
> > they are executed in a coroutine context or not.
> The commit message is incomplete. You also convert brdv_read and brdv_write.

Thanks, I do need to mention this.

> Moreover:
>
> > diff --git a/block/ssh.c b/block/ssh.c
> > index d7e7bf8..66ac4c1 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
> > @@ -748,7 +748,7 @@ static int return_true(void *opaque)
> >       return 1;
> >   }
> >   
> > -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> > +static void set_fd_handler(BDRVSSHState *s)
> >   {
> >       int r;
> >       IOHandler *rd_handler = NULL, *wr_handler = NULL;
> > @@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> >       qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
> >   }
> >   
> > -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
> > +static void clear_fd_handler(BDRVSSHState *s)
> >   {
> >       DPRINTF("s->sock=%d", s->sock);
> >       qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
> > 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.


Thanks,

Charlie