Patchwork [2/5] qemu_coroutine_self should not be marked coroutine_fn as it cannot yield

login
register
mail settings
Submitter Charlie Shepherd
Date Aug. 5, 2013, 6:44 p.m.
Message ID <1375728247-1306-3-git-send-email-charlie@ctshepherd.com>
Download mbox | patch
Permalink /patch/264749/
State New
Headers show

Comments

Charlie Shepherd - Aug. 5, 2013, 6:44 p.m.
From: Charlie Shepherd <cs648@cam.ac.uk>

While it only really makes sense to call qemu_coroutine_self() in a coroutine
context, it cannot actually yield execution itself, so remove the coroutine_fn
annotation.
---
 include/block/coroutine.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Stefan Hajnoczi - Aug. 7, 2013, 7:18 p.m.
On Mon, Aug 05, 2013 at 08:44:04PM +0200, Charlie Shepherd wrote:
> From: Charlie Shepherd <cs648@cam.ac.uk>
> 
> While it only really makes sense to call qemu_coroutine_self() in a coroutine
> context, it cannot actually yield execution itself, so remove the coroutine_fn
> annotation.
> ---
>  include/block/coroutine.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

By removing coroutine_fn the rules have changed:

It's now legal to call qemu_coroutine_self() outside a coroutine.
Previously only callers that knew the internals of the coroutine
implementation did that.

coroutine_fn gives coroutine backend implementors more flexibility in
how they choose to implement qemu_coroutine_self().  From an API
perspective I prefer to keep qemu_coroutine_self() marked as a
coroutine_fn.

I guess the practical problem is that CPC will get
upset that it's being called by the coroutine implementation from
non-coroutine contexts.  This can be solved:

1. Keep the public qemu_coroutine_self() marked coroutine_fn.

2. Have it call an internal coroutine_self() function that is not
   coroutine_fn.

This way the API stays strict and the internal implementation doesn't
violate the coroutine_fn calling rule.

Stefan
Gabriel Kerneis - Aug. 7, 2013, 10:13 p.m.
On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
> I guess the practical problem is that CPC will get
> upset that it's being called by the coroutine implementation from
> non-coroutine contexts.

But is it really the case? If Charlie added an annotation, it probably means
that in practice it was only called from coroutine context anyway.

The only downside from CPC's point of view is that there is a cost to making a
coroutine_fn, and it's a pity to pay it when it's useless (ie. when the function
never yields anyway).
Charlie Shepherd - Aug. 8, 2013, 1:25 a.m.
On 07/08/2013 20:18, Stefan Hajnoczi wrote:
> On Mon, Aug 05, 2013 at 08:44:04PM +0200, Charlie Shepherd wrote:
>> From: Charlie Shepherd <cs648@cam.ac.uk>
>>
>> While it only really makes sense to call qemu_coroutine_self() in a coroutine
>> context, it cannot actually yield execution itself, so remove the coroutine_fn
>> annotation.
>> ---
>>   include/block/coroutine.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> By removing coroutine_fn the rules have changed:
>
> It's now legal to call qemu_coroutine_self() outside a coroutine.
> Previously only callers that knew the internals of the coroutine
> implementation did that.

Yes, I agree that it's probably not helpful to loosen the restrictions 
on when its possible to call qemu_coroutine_self().

> coroutine_fn gives coroutine backend implementors more flexibility in
> how they choose to implement qemu_coroutine_self().  From an API
> perspective I prefer to keep qemu_coroutine_self() marked as a
> coroutine_fn.
>
> I guess the practical problem is that CPC will get
> upset that it's being called by the coroutine implementation from
> non-coroutine contexts.  This can be solved:
>
> 1. Keep the public qemu_coroutine_self() marked coroutine_fn.
>
> 2. Have it call an internal coroutine_self() function that is not
>     coroutine_fn.
>
> This way the API stays strict and the internal implementation doesn't
> violate the coroutine_fn calling rule.

This is a good solution, I'll add a patch implementing it to the series.


Charlie
Charlie Shepherd - Aug. 8, 2013, 1:29 a.m.
On 07/08/2013 23:13, Gabriel Kerneis wrote:
> On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
>> I guess the practical problem is that CPC will get
>> upset that it's being called by the coroutine implementation from
>> non-coroutine contexts.
> But is it really the case? If Charlie added an annotation, it probably means
> that in practice it was only called from coroutine context anyway.

It was also called from coroutine implementation in functions that 
weren't annotated coroutine_fn (qemu_coroutine_switch() and friends).

> The only downside from CPC's point of view is that there is a cost to making a
> coroutine_fn, and it's a pity to pay it when it's useless (ie. when the function
> never yields anyway).

Yes although in this case it won't be too high as the function will 
simply be

Coroutine *internal_qemu_coroutine_self(void);

coroutine_fn Coroutine *qemu_coroutine_self(void)
{
     return internal_qemu_coroutine_self();
}

and the original qemu_coroutine_self couldn't be inlined, so I don't 
think it is too prohibitive. Besides, I doubt qemu_coroutine_self() is 
called in many fast-paths.


Charlie
Gabriel Kerneis - Aug. 8, 2013, 6:16 a.m.
On Thu, Aug 08, 2013 at 02:29:39AM +0100, Charlie Shepherd wrote:
> On 07/08/2013 23:13, Gabriel Kerneis wrote:
> >On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
> >>I guess the practical problem is that CPC will get
> >>upset that it's being called by the coroutine implementation from
> >>non-coroutine contexts.
> >But is it really the case? If Charlie added an annotation, it probably means
> >that in practice it was only called from coroutine context anyway.
> 
> It was also called from coroutine implementation in functions that
> weren't annotated coroutine_fn (qemu_coroutine_switch() and
> friends).

In that case, the interface/implementation split suggested by Stefan makes
sense.

Note that qemu_coroutine_self() in principle really needs to be annotated
coroutine_fn since it accesses (and returns) its execution context. The fact
that it is implemented with thread-local variables in Qemu is an implementation
detail, almost a hack (however smart); the "natural" CPC way would be to just
return the coroutine associated with the current continuation. So keeping the
annotation definitely makes sense.
Charlie Shepherd - Aug. 8, 2013, 9:10 a.m.
On 08/08/2013 07:16, Gabriel Kerneis wrote:
> On Thu, Aug 08, 2013 at 02:29:39AM +0100, Charlie Shepherd wrote:
>> On 07/08/2013 23:13, Gabriel Kerneis wrote:
>>> On Wed, Aug 07, 2013 at 09:18:05PM +0200, Stefan Hajnoczi wrote:
>>>> I guess the practical problem is that CPC will get
>>>> upset that it's being called by the coroutine implementation from
>>>> non-coroutine contexts.
>>> But is it really the case? If Charlie added an annotation, it probably means
>>> that in practice it was only called from coroutine context anyway.
>> It was also called from coroutine implementation in functions that
>> weren't annotated coroutine_fn (qemu_coroutine_switch() and
>> friends).
> In that case, the interface/implementation split suggested by Stefan makes
> sense.
>
> Note that qemu_coroutine_self() in principle really needs to be annotated
> coroutine_fn since it accesses (and returns) its execution context. The fact
> that it is implemented with thread-local variables in Qemu is an implementation
> detail, almost a hack (however smart); the "natural" CPC way would be to just
> return the coroutine associated with the current continuation. So keeping the
> annotation definitely makes sense.

While that would be the natural way, we are going to need the thread 
local variables regardless due to the use of the "leader" coroutine I 
believe (Stefan is this correct?). Also implementing the split in the 
way Stefan suggests would mean it's not possible to return the CPC 
continuation without a hack like the last hack I did for 
qemu_coroutine_yield(), as internal_qemu_coroutine_self() (I guess this 
function needs a better name too) would be marked as non coroutine 
across all coroutine implementions (ie in coroutine_int.h).


Charlie
Gabriel Kerneis - Aug. 8, 2013, 9:12 a.m.
On Thu, Aug 08, 2013 at 10:10:18AM +0100, Charlie Shepherd wrote:
> >Note that qemu_coroutine_self() in principle really needs to be annotated
> >coroutine_fn since it accesses (and returns) its execution context. The fact
> >that it is implemented with thread-local variables in Qemu is an implementation
> >detail, almost a hack (however smart); the "natural" CPC way would be to just
> >return the coroutine associated with the current continuation. So keeping the
> >annotation definitely makes sense.
> 
> While that would be the natural way, we are going to need the thread
> local variables regardless due to the use of the "leader" coroutine
> I believe (Stefan is this correct?).

Sure. I was simply pointing out that from a "philosophical" point-of-view,
it makes more sense to keep it coroutine_fn.

Patch

diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 3b94b6d..563dcde 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -87,7 +87,7 @@  void coroutine_fn qemu_coroutine_yield(void);
 /**
  * Get the currently executing coroutine
  */
-Coroutine *coroutine_fn qemu_coroutine_self(void);
+Coroutine *qemu_coroutine_self(void);
 
 /**
  * Return whether or not currently inside a coroutine