From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33398) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6QX0-0003kq-Jx for qemu-devel@nongnu.org; Mon, 05 Aug 2013 15:33:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6QWs-0001DJ-Bm for qemu-devel@nongnu.org; Mon, 05 Aug 2013 15:33:26 -0400 Received: from mail6.webfaction.com ([74.55.86.74]:58835 helo=smtp.webfaction.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6QWs-0001D7-58 for qemu-devel@nongnu.org; Mon, 05 Aug 2013 15:33:18 -0400 Message-ID: <51FFFDF6.4060602@ctshepherd.com> Date: Mon, 05 Aug 2013 20:33:10 +0100 From: Charlie Shepherd MIME-Version: 1.0 References: <1375728247-1306-1-git-send-email-charlie@ctshepherd.com> <1375728247-1306-4-git-send-email-charlie@ctshepherd.com> <20130805192347.GB4872@kerneis.info> In-Reply-To: <20130805192347.GB4872@kerneis.info> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gabriel Kerneis Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com 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. > Moreover: > >> 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); >> >> #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. Thanks, Charlie