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

[Charlie Shepherd <charlie@ctshepherd.com>]

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