From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48095) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLyhl-000765-It for qemu-devel@nongnu.org; Tue, 17 Sep 2013 13:04:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VLyhf-0007C6-Hj for qemu-devel@nongnu.org; Tue, 17 Sep 2013 13:04:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36072) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLyhf-0007Bu-8t for qemu-devel@nongnu.org; Tue, 17 Sep 2013 13:04:43 -0400 Message-ID: <52388BAA.1060207@redhat.com> Date: Tue, 17 Sep 2013 19:04:42 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1376239405-4084-1-git-send-email-alex@alex.org.uk> <520A2511.4000709@siemens.com> <307AE3B5-FAFE-4E9C-A336-092245809528@alex.org.uk> <520A33B4.9030207@siemens.com> <14A27B81-C9FD-4EE5-BC4A-7106CD70527A@alex.org.uk> <520A3888.9020307@siemens.com> <20130813142204.GA3008@stefanha-thinkpad.redhat.com> <52387EC2.6000502@siemens.com> <5238810E.1050304@redhat.com> <5238883A.90607@siemens.com> In-Reply-To: <5238883A.90607@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] [PATCHv10 00/31] aio / timers: Add AioContext timers and use ppoll List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Kevin Wolf , Anthony Liguori , Alex Bligh , Stefan Hajnoczi , qemu-devel@nongnu.org, liu ping fan , Stefan Hajnoczi , MORITA Kazutaka , rth@twiddle.net Il 17/09/2013 18:50, Jan Kiszka ha scritto: > On 2013-09-17 18:38, Alex Bligh wrote: >> >> On 17 Sep 2013, at 17:19, Paolo Bonzini wrote: >> >>> Il 17/09/2013 18:09, Jan Kiszka ha scritto: >>>> On 2013-08-13 16:22, Stefan Hajnoczi wrote: >>>>> On Tue, Aug 13, 2013 at 03:45:44PM +0200, Jan Kiszka wrote: >>>>>> Yeah: >>>>>> >>>>>> - /* No AIO operations? Get us out of here */ >>>>>> - if (!busy) { >>>>>> + /* early return if we only have the aio_notify() fd */ >>>>>> + if (ctx->pollfds->len == 1) { >>>>>> return progress; >>>>>> } >>>>>> >>>>>> So this is even worse for my use case. >>>>> >>>>> We can change the semantics of aio_poll() so long as we don't break >>>>> existing callers and tests. It would make sense to do that after >>>>> merging the io_flush and AioContext timers series. >>>> >>>> Need to pick up this topic again because above change is now mainline >>>> and breaks aio_poll-based timer threads: >>>> >>>> How can we make progress with overcoming that check, at least for the >>>> timer thread use case? Additional argument "truly_block" for aio_poll? >>> >>> I wonder if we still need that "if" at all. Guys, do you remember what >>> it is good for? O:-) >> >> I can't remember whether the code above was in my patch or Stefan's >> subsequent ones, but I had suggested >> if (blocking && (ctx->pollfds->len <= 1)) >> >> On reflection, I don't think the 'if' line should be there at all. >> >> I think the argument was that it was meant to be for stupidity >> protection, i.e. where someone calls with blocking set to 1 and no >> fds, it would block indefinitely (if there were no timers) >> as it would select with no timeout and no fds. >> >> Personally I think if you call things this way you deserve all you >> get. I'm not sure attempting to rescue the user by returning >> immediately is a great plan. If there are truly no fds at all >> (not even the notify fd) and an infinite timeout perhaps we should >> set the timeout to a second and log an error? >> >> I presume the reason it's breaking aio_poll based timer threads is >> because there is only one fd (the aio_notify_fd), there are >> no other fd's, but there is a timeout (from the timers)? > > Right. > >> If >> that's true, I think we /shouldn't/ return. Equally if there >> are no timers but something is genuinely attempting to wait >> on an aio_notify, I don't think we should return. >> > > In any case, test-aio seems to stress that if clause. If we remove it, > the test case hangs infinitely. But I'm more worried about understanding > if there are actual users depending on the current behavior > (bdrv_drain_all?). Yes, bdrv_drain_all comes to mind. But now we should be able to fix this old remark: /* FIXME: We do not have timer support here, so this is effectively * a busy wait. */ QTAILQ_FOREACH(bs, &bdrv_states, list) { if (bdrv_start_throttled_reqs(bs)) { busy = true; } } busy = bdrv_requests_pending_all(); busy |= aio_poll(qemu_get_aio_context(), busy); Alex, what's missing before block.c and QED can use aio_timer_new on the main AioContext, instead of timer_new? Paolo