[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Charlie Shepherd
Subject: Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
Date: Thu, 08 Aug 2013 02:31:46 +0100
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7

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);
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

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]