diff mbox series

[v3,06/33] util/async: aio_co_schedule(): support reschedule in same ctx

Message ID 20210416080911.83197-7-vsementsov@virtuozzo.com (mailing list archive)
State New
Headers show
Series block/nbd: rework client connection | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 16, 2021, 8:08 a.m. UTC
With the following patch we want to call wake coroutine from thread.
And it doesn't work with aio_co_wake:
Assume we have no iothreads.
Assume we have a coroutine A, which waits in the yield point for
external aio_co_wake(), and no progress can be done until it happen.
Main thread is in blocking aio_poll() (for example, in blk_read()).

Now, in a separate thread we do aio_co_wake(). It calls  aio_co_enter(),
which goes through last "else" branch and do aio_context_acquire(ctx).

Now we have a deadlock, as aio_poll() will not release the context lock
until some progress is done, and progress can't be done until
aio_co_wake() wake the coroutine A. And it can't because it wait for
aio_context_acquire().

Still, aio_co_schedule() works well in parallel with blocking
aio_poll(). So we want use it. Let's add a possibility of rescheduling
coroutine in same ctx where it was yield'ed.

Fetch co->ctx in same way as it is done in aio_co_wake().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/aio.h | 2 +-
 util/async.c        | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Roman Kagan April 23, 2021, 10:09 a.m. UTC | #1
On Fri, Apr 16, 2021 at 11:08:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> With the following patch we want to call wake coroutine from thread.
> And it doesn't work with aio_co_wake:
> Assume we have no iothreads.
> Assume we have a coroutine A, which waits in the yield point for
> external aio_co_wake(), and no progress can be done until it happen.
> Main thread is in blocking aio_poll() (for example, in blk_read()).
> 
> Now, in a separate thread we do aio_co_wake(). It calls  aio_co_enter(),
> which goes through last "else" branch and do aio_context_acquire(ctx).
> 
> Now we have a deadlock, as aio_poll() will not release the context lock
> until some progress is done, and progress can't be done until
> aio_co_wake() wake the coroutine A. And it can't because it wait for
> aio_context_acquire().
> 
> Still, aio_co_schedule() works well in parallel with blocking
> aio_poll(). So we want use it. Let's add a possibility of rescheduling
> coroutine in same ctx where it was yield'ed.
> 
> Fetch co->ctx in same way as it is done in aio_co_wake().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/aio.h | 2 +-
>  util/async.c        | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 5f342267d5..744b695525 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -643,7 +643,7 @@ static inline bool aio_node_check(AioContext *ctx, bool is_external)
>  
>  /**
>   * aio_co_schedule:
> - * @ctx: the aio context
> + * @ctx: the aio context, if NULL, the current ctx of @co will be used.
>   * @co: the coroutine
>   *
>   * Start a coroutine on a remote AioContext.
> diff --git a/util/async.c b/util/async.c
> index 674dbefb7c..750be555c6 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -545,6 +545,14 @@ fail:
>  
>  void aio_co_schedule(AioContext *ctx, Coroutine *co)
>  {
> +    if (!ctx) {
> +        /*
> +         * Read coroutine before co->ctx.  Matches smp_wmb in
> +         * qemu_coroutine_enter.
> +         */
> +        smp_read_barrier_depends();
> +        ctx = qatomic_read(&co->ctx);
> +    }

I'd rather not extend this interface, but add a new one on top.  And
document how it's different from aio_co_wake().

Roman.

>      trace_aio_co_schedule(ctx, co);
>      const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL,
>                                             __func__);
> -- 
> 2.29.2
>
Vladimir Sementsov-Ogievskiy April 26, 2021, 8:52 a.m. UTC | #2
23.04.2021 13:09, Roman Kagan wrote:
> On Fri, Apr 16, 2021 at 11:08:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> With the following patch we want to call wake coroutine from thread.
>> And it doesn't work with aio_co_wake:
>> Assume we have no iothreads.
>> Assume we have a coroutine A, which waits in the yield point for
>> external aio_co_wake(), and no progress can be done until it happen.
>> Main thread is in blocking aio_poll() (for example, in blk_read()).
>>
>> Now, in a separate thread we do aio_co_wake(). It calls  aio_co_enter(),
>> which goes through last "else" branch and do aio_context_acquire(ctx).
>>
>> Now we have a deadlock, as aio_poll() will not release the context lock
>> until some progress is done, and progress can't be done until
>> aio_co_wake() wake the coroutine A. And it can't because it wait for
>> aio_context_acquire().
>>
>> Still, aio_co_schedule() works well in parallel with blocking
>> aio_poll(). So we want use it. Let's add a possibility of rescheduling
>> coroutine in same ctx where it was yield'ed.
>>
>> Fetch co->ctx in same way as it is done in aio_co_wake().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/aio.h | 2 +-
>>   util/async.c        | 8 ++++++++
>>   2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/block/aio.h b/include/block/aio.h
>> index 5f342267d5..744b695525 100644
>> --- a/include/block/aio.h
>> +++ b/include/block/aio.h
>> @@ -643,7 +643,7 @@ static inline bool aio_node_check(AioContext *ctx, bool is_external)
>>   
>>   /**
>>    * aio_co_schedule:
>> - * @ctx: the aio context
>> + * @ctx: the aio context, if NULL, the current ctx of @co will be used.
>>    * @co: the coroutine
>>    *
>>    * Start a coroutine on a remote AioContext.
>> diff --git a/util/async.c b/util/async.c
>> index 674dbefb7c..750be555c6 100644
>> --- a/util/async.c
>> +++ b/util/async.c
>> @@ -545,6 +545,14 @@ fail:
>>   
>>   void aio_co_schedule(AioContext *ctx, Coroutine *co)
>>   {
>> +    if (!ctx) {
>> +        /*
>> +         * Read coroutine before co->ctx.  Matches smp_wmb in
>> +         * qemu_coroutine_enter.
>> +         */
>> +        smp_read_barrier_depends();
>> +        ctx = qatomic_read(&co->ctx);
>> +    }
> 
> I'd rather not extend this interface, but add a new one on top.  And
> document how it's different from aio_co_wake().
> 

Agree, that's better. Will do.
diff mbox series

Patch

diff --git a/include/block/aio.h b/include/block/aio.h
index 5f342267d5..744b695525 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -643,7 +643,7 @@  static inline bool aio_node_check(AioContext *ctx, bool is_external)
 
 /**
  * aio_co_schedule:
- * @ctx: the aio context
+ * @ctx: the aio context, if NULL, the current ctx of @co will be used.
  * @co: the coroutine
  *
  * Start a coroutine on a remote AioContext.
diff --git a/util/async.c b/util/async.c
index 674dbefb7c..750be555c6 100644
--- a/util/async.c
+++ b/util/async.c
@@ -545,6 +545,14 @@  fail:
 
 void aio_co_schedule(AioContext *ctx, Coroutine *co)
 {
+    if (!ctx) {
+        /*
+         * Read coroutine before co->ctx.  Matches smp_wmb in
+         * qemu_coroutine_enter.
+         */
+        smp_read_barrier_depends();
+        ctx = qatomic_read(&co->ctx);
+    }
     trace_aio_co_schedule(ctx, co);
     const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL,
                                            __func__);