From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50802) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7F53-0001V2-IJ for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:32:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7F4u-0006MY-7G for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:31:57 -0400 Received: from mail6.webfaction.com ([74.55.86.74]:37640 helo=smtp.webfaction.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7F4u-0006MT-0L for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:31:48 -0400 Message-ID: <5202F502.5020808@ctshepherd.com> Date: Thu, 08 Aug 2013 02:31:46 +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> <51FFFDF6.4060602@ctshepherd.com> <20130807193025.GB16226@stefanha-thinkpad.hitronhub.home> In-Reply-To: <20130807193025.GB16226@stefanha-thinkpad.hitronhub.home> 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: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, Gabriel Kerneis , qemu-devel@nongnu.org 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); >>>> #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. > 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 > coroutine_fn. > > 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. Charlie