[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: Mon, 05 Aug 2013 20:33:10 +0100
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7

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.


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



reply via email to

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