| Message ID | 1375728247-1306-3-git-send-email-charlie@ctshepherd.com |
|---|---|
| State | New |
| Headers | show |
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
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).
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
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
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.
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
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.
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
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(-)