Re: [PATCH 3/3] aio-posix: keep aio_notify_me disabled during polling

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2921 bytes --]

On 04/08/20 12:29, Stefan Hajnoczi wrote:
> On Tue, Aug 04, 2020 at 06:28:04AM +0100, Stefan Hajnoczi wrote:
>> @@ -597,15 +574,38 @@ bool aio_poll(AioContext *ctx, bool blocking)
>>       * system call---a single round of run_poll_handlers_once suffices.
>>       */
>>      if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
>> +        /*
>> +         * aio_notify can avoid the expensive event_notifier_set if
>> +         * everything (file descriptors, bottom halves, timers) will
>> +         * be re-evaluated before the next blocking poll().  This is
>> +         * already true when aio_poll is called with blocking == false;
>> +         * if blocking == true, it is only true after poll() returns,
>> +         * so disable the optimization now.
>> +         */
>> +        if (timeout) {
>> +            atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
>> +            /*
>> +             * Write ctx->notify_me before computing the timeout
>> +             * (reading bottom half flags, etc.).  Pairs with
>> +             * smp_mb in aio_notify().
>> +             */
>> +            smp_mb();
>> +
>> +            /* Check again in case a shorter timer was added */
>> +            timeout = qemu_soonest_timeout(timeout, aio_compute_timeout(ctx));
>> +        }
>> +
>>          ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
>> -    }
>>  
>> -    if (blocking) {
>> -        /* Finish the poll before clearing the flag.  */
>> -        atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 2);
>> -        aio_notify_accept(ctx);
>> +        if (timeout) {
>> +            /* Finish the poll before clearing the flag.  */
>> +            atomic_store_release(&ctx->notify_me,
>> +                                 atomic_read(&ctx->notify_me) - 2);
>> +        }
>>      }
> 
> Hi Paolo,
> We can avoid calling aio_compute_timeout() like this, what do you think?

I don't understand :) except I guess you mean we can avoid the second
call.  Can you post either a complete patch with this squashed, or a 4th
patch (whatever you think is best)?

Paolo

>   bool use_notify_me = timeout != 0;
> 
>   if (use_notify_me) {
>       atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
>       /*
>        * Write ctx->notify_me before computing the timeout
>        * (reading bottom half flags, etc.).  Pairs with
>        * smp_mb in aio_notify().
>        */
>       smp_mb();
> 
>       /* Don't block if aio_notify() was called */
>       if (atomic_read(ctx->notified)) {
>           timeout = 0;
>       }
>   }
> 
>   ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
> 
>   if (use_notify_me) {
>       /* Finish the poll before clearing the flag.  */
>       atomic_store_release(&ctx->notify_me,
>                            atomic_read(&ctx->notify_me) - 2);
>   }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]