Re: [Qemu-devel] [RFC] [PATCHv10 00/31] aio / timers: Add AioContext timers and use ppoll

[Paolo Bonzini <pbonzini@redhat.com>]

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