Re: [Qemu-devel] [PATCH] thread-pool: fix deadlock when callbacks depends on each other

Re: [Qemu-devel] [PATCH] thread-pool: fix deadlock when callbacks depends on each other

[Paolo Bonzini]

Il 31/05/2014 20:29, Marcin Gibuła ha scritto:
>
> @@ -185,6 +191,14 @@ restart:
>          }
>          if (elem->state == THREAD_DONE && elem->common.cb) {
>              QLIST_REMOVE(elem, all);
> +            /* If more completed requests are waiting, notifier needs
> +             * to be rearmed so callback can progress with aio_pool().
> +             */
> +            pool->pending_completions--;
> +            if (pool->pending_completions) {
> +                event_notifier_set(notifier);
> +            }
> +
>              /* Read state before ret.  */
>              smp_rmb();
>              elem->common.cb(elem->common.opaque, elem->ret);

Good catch!  The main problem with the patch is that you need to use 
atomic_inc/atomic_dec to increment and decrement pool->pending_completions.

Secondarily, event_notifier_set is pretty heavy-weight, does it work if 
you wrap the loop like this?

restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
         ...
     }
     if (pool->pending_completions) {
         goto restart;
     }
     event_notifier_test_and_clear(notifier);
     if (pool->pending_completions) {
         event_notifier_set(notifier);
         goto restart;
     }

Finally, the same bug is also in block/linux-aio.c and block/win32-aio.c.

Paolo

Re: [Qemu-devel] [PATCH] thread-pool: fix deadlock when callbacks depends on each other

[Marcin Gibuła]

> Good catch!  The main problem with the patch is that you need to use
> atomic_inc/atomic_dec to increment and decrement pool->pending_completions.

Ok.

> Secondarily, event_notifier_set is pretty heavy-weight, does it work if
> you wrap the loop like this?
>
> restart:
>      QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
>          ...
>      }
>      if (pool->pending_completions) {
>          goto restart;
>      }
>      event_notifier_test_and_clear(notifier);
>      if (pool->pending_completions) {
>          event_notifier_set(notifier);
>          goto restart;
>      }

I'll test it tomorrow. I assume you want to avoid calling 
event_notifier_set() until function is reentered via aio_pool?

 > Finally, the same bug is also in block/linux-aio.c and
 > block/win32-aio.c.

I can try with linux-aio, but my knowledge of windows api is zero...

-- 
mg

Re: [Qemu-devel] [PATCH] thread-pool: fix deadlock when callbacks depends on each other

[Paolo Bonzini]

Il 01/06/2014 21:02, Marcin Gibuła ha scritto:
>> Good catch!  The main problem with the patch is that you need to use
>> atomic_inc/atomic_dec to increment and decrement
>> pool->pending_completions.
>
> Ok.
>
>> Secondarily, event_notifier_set is pretty heavy-weight, does it work if
>> you wrap the loop like this?
>>
>> restart:
>>      QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
>>          ...
>>      }
>>      if (pool->pending_completions) {
>>          goto restart;
>>      }
>>      event_notifier_test_and_clear(notifier);
>>      if (pool->pending_completions) {
>>          event_notifier_set(notifier);
>>          goto restart;
>>      }
>
> I'll test it tomorrow. I assume you want to avoid calling
> event_notifier_set() until function is reentered via aio_pool?

Yes.  But actually, I need to check if it's possible to fix 
bdrv_drain_all.  If you're in coroutine context, you can defer the 
draining to a safe point using a bottom half.  If you're not in 
coroutine context, perhaps bdrv_drain_all has to be made illegal.  Which 
means a bunch of code auditing...

Paolo

>> Finally, the same bug is also in block/linux-aio.c and
>> block/win32-aio.c.
>
> I can try with linux-aio, but my knowledge of windows api is zero...
>

Re: [Qemu-devel] [PATCH] thread-pool: fix deadlock when callbacks depends on each other

[Marcin Gibuła]

>> I'll test it tomorrow. I assume you want to avoid calling
>> event_notifier_set() until function is reentered via aio_pool?
>
> Yes.  But actually, I need to check if it's possible to fix
> bdrv_drain_all.  If you're in coroutine context, you can defer the
> draining to a safe point using a bottom half.  If you're not in
> coroutine context, perhaps bdrv_drain_all has to be made illegal.  Which
> means a bunch of code auditing...

For what it's worth, your solution also works fine, I couldn't recreate 
hang with it. Updated patch proposal posted earlier today.

-- 
mg