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

[Charlie Shepherd <charlie@ctshepherd.com>]

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