From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58217) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uz7Ce-0000Ld-2u for qemu-devel@nongnu.org; Tue, 16 Jul 2013 11:30:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uz7CZ-0002tD-1I for qemu-devel@nongnu.org; Tue, 16 Jul 2013 11:30:12 -0400 Received: from mail.avalus.com ([2001:41c8:10:1dd::10]:58058) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uz7CY-0002rJ-N9 for qemu-devel@nongnu.org; Tue, 16 Jul 2013 11:30:06 -0400 Date: Tue, 16 Jul 2013 16:29:53 +0100 From: Alex Bligh Message-ID: <794E19D97CCC267CCBFA8397@Ximines.local> In-Reply-To: <51E4F77C.2090509@redhat.com> References: <1373127897-3445-1-git-send-email-alex@alex.org.uk> <51E4063D.6010308@redhat.com> <75638CBA408455BF52192DA7@Ximines.local> <51E4613D.9000106@redhat.com> <44590808AF4A6E7DC093637A@nimrod.local> <51E4E54A.10908@redhat.com> <51E4F77C.2090509@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline Subject: Re: [Qemu-devel] [PATCH] [RFC] aio/async: Add timed bottom-halves Reply-To: Alex Bligh List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , Anthony Liguori , Stefan Hajnoczi , qemu-devel@nongnu.org, Alex Bligh , rth@twiddle.net Paolo, --On 16 July 2013 09:34:20 +0200 Paolo Bonzini wrote: >>> You did. But aio_wait() ignores the timeout. It is only used by the >>> main loop. >> >> OK well that seems worth fixing in any case, as even without timed bh's >> that means no bh can be executed for an indeterminate time. I'll have >> a look at that. > > No, BHs work because they do aio_notify(). Idle BHs can be skipped for > an indeterminate time, but that's fine because idle BHs are a hack that > we should not need at all. OK, so a bit more code reading later, I think I now understand. 1. non-idle bh's call aio_notify at schedule time, which will cause any poll to exit immediately because at least one FD will be ready. 2. idle bh's do not do aio_notify() because we don't care whether they get stuck in aio_poll and they are a hack [actually I think we could do better here] 3. aio_poll calls aio_bh_poll. If this returns true, this indicates at least one non-idle bh exists, which causes aio_poll not to block. Question 1: it then calls aio_dispatch - if this itself generates non-idle bh's, this would seem not to clear the blocking flag. Does this rely on aio_notify? Question 2: if we're already telling aio_poll not to block by the presence of non-idle bh's as detected in aio_bh_poll, why do we need to use aio_notify too? IE why do we have both the blocking= logic AND the aio_notify logic? 4. aio_poll then calls g_poll (POSIX) or WaitForMultipleObjects (Windows). However, the timeout is either 0 or infinite. Both functions take a milliseconds (yuck) timeout, but that is not used. So, the first thing I don't understand is why aio_poll needs the return value of aio_bh_poll at all. Firstly, after sampling it, it then causes aio_dispatch, and that can presumably set its own bottom half callbacks; if this happens 'int blocking' won't be cleared, and it will still enter g_poll with an infinite timeout. Secondly, there seems to be an entirely separate mechanism (aio_notify) in any case. If a non-idle bh has been scheduled, this will cause g_poll to exit immediately as a read will be ready. I believe this is cleared by the bh being used. The second thing I don't understand is why we aren't using the timeout on g_poll / WaitForMultipleObjects. It would seem to be reasonably easy to make aio_poll call aio_ctx_prepare or something that does the same calculation. This would fix idle bh's to be more reliable (we know it's safe to call them within aio_poll anyway, it's just a question of whether we turn an infinite wait into a 10ms wait). Perhaps these two are related. I /think/ fixing the second (and removing the aio_notify from qemu_bh_schedule_at) is sufficient provided it checks for scheduled bh's immediately prior to the poll. This assumes other threads cannot schedule bh's. This would seem to be less intrusive that a TimedEventNotifier approach which (as far as I can see) requires another thread. -- Alex Bligh