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

[Jan Kiszka]

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?).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux