[PATCH 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

[PATCH 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

[Stefan Hajnoczi]

AIO_WAIT_WHILE_UNLOCKED() is the future replacement for AIO_WAIT_WHILE(). Most
callers haven't been converted yet because they rely on the AioContext lock. I
looked through the code and found the easy cases that can be converted today.

Stefan Hajnoczi (6):
  block: don't acquire AioContext lock in bdrv_drain_all()
  block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()
  block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()
  block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()
  hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()
  monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

 block/block-backend.c | 8 +-------
 block/export/export.c | 2 +-
 block/graph-lock.c    | 2 +-
 block/io.c            | 2 +-
 monitor/hmp.c         | 2 +-
 monitor/monitor.c     | 4 ++--
 6 files changed, 7 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Stefan Hajnoczi]

There is no need for the AioContext lock in bdrv_drain_all() because
nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.

Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
the future it can be removed. There is an assertion in
AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
we would lose that check by dropping the argument. However, that was a
precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
duplicate check. So I think we won't lose much by dropping it, but let's
do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
confirm this is the case.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 278b04ce69..d2b6b3652d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1835,14 +1835,8 @@ void blk_drain_all(void)
     bdrv_drain_all_begin();
 
     while ((blk = blk_all_next(blk)) != NULL) {
-        AioContext *ctx = blk_get_aio_context(blk);
-
-        aio_context_acquire(ctx);
-
         /* We may have -ENOMEDIUM completions in flight */
-        AIO_WAIT_WHILE(ctx, qatomic_mb_read(&blk->in_flight) > 0);
-
-        aio_context_release(ctx);
+        AIO_WAIT_WHILE_UNLOCKED(NULL, qatomic_mb_read(&blk->in_flight) > 0);
     }
 
     bdrv_drain_all_end();
-- 
2.39.2

[PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
instead of AIO_WAIT_WHILE() to document that this code has already been
audited and converted. The AioContext argument is already NULL so
aio_context_release() is never called anyway.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/export.c b/block/export/export.c
index 28a91c9c42..e3fee60611 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -306,7 +306,7 @@ void blk_exp_close_all_type(BlockExportType type)
         blk_exp_request_shutdown(exp);
     }
 
-    AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
+    AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type));
 }
 
 void blk_exp_close_all(void)
-- 
2.39.2

[PATCH 3/6] block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

The following conversion is safe and does not change behavior:

     GLOBAL_STATE_CODE();
     ...
  -  AIO_WAIT_WHILE(qemu_get_aio_context(), ...);
  +  AIO_WAIT_WHILE_UNLOCKED(NULL, ...);

Since we're in GLOBAL_STATE_CODE(), qemu_get_aio_context() is our home
thread's AioContext. Thus AIO_WAIT_WHILE() does not unlock the
AioContext:

  if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
      while ((cond)) {                                           \
          aio_poll(ctx_, true);                                  \
          waited_ = true;                                        \
      }                                                          \

And that means AIO_WAIT_WHILE_UNLOCKED(NULL, ...) can be substituted.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/graph-lock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/graph-lock.c b/block/graph-lock.c
index 454c31e691..639526608f 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -127,7 +127,7 @@ void bdrv_graph_wrlock(void)
          * reader lock.
          */
         qatomic_set(&has_writer, 0);
-        AIO_WAIT_WHILE(qemu_get_aio_context(), reader_count() >= 1);
+        AIO_WAIT_WHILE_UNLOCKED(NULL, reader_count() >= 1);
         qatomic_set(&has_writer, 1);
 
         /*
-- 
2.39.2

[PATCH 4/6] block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

Since the AioContext argument was already NULL, AIO_WAIT_WHILE() was
never going to unlock the AioContext. Therefore it is possible to
replace AIO_WAIT_WHILE() with AIO_WAIT_WHILE_UNLOCKED().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 8974d46941..db438c7657 100644
--- a/block/io.c
+++ b/block/io.c
@@ -520,7 +520,7 @@ void bdrv_drain_all_begin(void)
     bdrv_drain_all_begin_nopoll();
 
     /* Now poll the in-flight requests */
-    AIO_WAIT_WHILE(NULL, bdrv_drain_all_poll());
+    AIO_WAIT_WHILE_UNLOCKED(NULL, bdrv_drain_all_poll());
 
     while ((bs = bdrv_next_all_states(bs))) {
         bdrv_drain_assert_idle(bs);
-- 
2.39.2

[PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

The HMP monitor runs in the main loop thread. Calling
AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
the AioContext and the latter's assertion that we're in the main loop
succeeds.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 monitor/hmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index 2aa85d3982..5ecbdac802 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
         Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
         monitor_set_cur(co, &mon->common);
         aio_co_enter(qemu_get_aio_context(), co);
-        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
+        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
     }
 
     qobject_unref(qdict);
-- 
2.39.2

[PATCH 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

monitor_cleanup() is called from the main loop thread. Calling
AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
the AioContext and the latter's assertion that we're in the main loop
succeeds.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 monitor/monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 8dc96f6af9..602535696c 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -666,7 +666,7 @@ void monitor_cleanup(void)
      * We need to poll both qemu_aio_context and iohandler_ctx to make
      * sure that the dispatcher coroutine keeps making progress and
      * eventually terminates.  qemu_aio_context is automatically
-     * polled by calling AIO_WAIT_WHILE on it, but we must poll
+     * polled by calling AIO_WAIT_WHILE_UNLOCKED on it, but we must poll
      * iohandler_ctx manually.
      *
      * Letting the iothread continue while shutting down the dispatcher
@@ -679,7 +679,7 @@ void monitor_cleanup(void)
         aio_co_wake(qmp_dispatcher_co);
     }
 
-    AIO_WAIT_WHILE(qemu_get_aio_context(),
+    AIO_WAIT_WHILE_UNLOCKED(NULL,
                    (aio_poll(iohandler_get_aio_context(), false),
                     qatomic_mb_read(&qmp_dispatcher_co_busy)));
 
-- 
2.39.2

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The HMP monitor runs in the main loop thread. Calling

Correct.

> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
> the AioContext and the latter's assertion that we're in the main loop
> succeeds.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  monitor/hmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 2aa85d3982..5ecbdac802 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
>          monitor_set_cur(co, &mon->common);
>          aio_co_enter(qemu_get_aio_context(), co);
> -        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> +        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
>      }
>  
>      qobject_unref(qdict);

Acked-by: Markus Armbruster <armbru@redhat.com>

For an R-by, I need to understand this in more detail.  I'm not familiar
with the innards of AIO_WAIT_WHILE() & friends, so I need to go real
slow.

We change

    ctx from qemu_get_aio_context() to NULL
    unlock from true to false

in

    bool waited_ = false;                                          \
    AioWait *wait_ = &global_aio_wait;                             \
    AioContext *ctx_ = (ctx);                                      \
    /* Increment wait_->num_waiters before evaluating cond. */     \
    qatomic_inc(&wait_->num_waiters);                              \
    /* Paired with smp_mb in aio_wait_kick(). */                   \
    smp_mb();                                                      \
    if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
        while ((cond)) {                                           \
            aio_poll(ctx_, true);                                  \
            waited_ = true;                                        \
        }                                                          \
    } else {                                                       \
        assert(qemu_get_current_aio_context() ==                   \
               qemu_get_aio_context());                            \
        while ((cond)) {                                           \
            if (unlock && ctx_) {                                  \
                aio_context_release(ctx_);                         \
            }                                                      \
            aio_poll(qemu_get_aio_context(), true);                \
            if (unlock && ctx_) {                                  \
                aio_context_acquire(ctx_);                         \
            }                                                      \
            waited_ = true;                                        \
        }                                                          \
    }                                                              \
    qatomic_dec(&wait_->num_waiters);                              \
    waited_; })

qemu_get_aio_context() is non-null here, correct?

What's the value of in_aio_context_home_thread(qemu_get_aio_context())?

Re: [PATCH 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> monitor_cleanup() is called from the main loop thread. Calling

Correct.

> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
> the AioContext and the latter's assertion that we're in the main loop
> succeeds.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  monitor/monitor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 8dc96f6af9..602535696c 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -666,7 +666,7 @@ void monitor_cleanup(void)
>       * We need to poll both qemu_aio_context and iohandler_ctx to make
>       * sure that the dispatcher coroutine keeps making progress and
>       * eventually terminates.  qemu_aio_context is automatically
> -     * polled by calling AIO_WAIT_WHILE on it, but we must poll
> +     * polled by calling AIO_WAIT_WHILE_UNLOCKED on it, but we must poll
>       * iohandler_ctx manually.
>       *
>       * Letting the iothread continue while shutting down the dispatcher
> @@ -679,7 +679,7 @@ void monitor_cleanup(void)
>          aio_co_wake(qmp_dispatcher_co);
>      }
>  
> -    AIO_WAIT_WHILE(qemu_get_aio_context(),
> +    AIO_WAIT_WHILE_UNLOCKED(NULL,
>                     (aio_poll(iohandler_get_aio_context(), false),
>                      qatomic_mb_read(&qmp_dispatcher_co_busy)));

Acked-by: Markus Armbruster <armbru@redhat.com>

For an R-by, I need to understand this in more detail.  See my reply to
the previous patch.

Re: [PATCH 3/6] block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 1/3/23 21:57, Stefan Hajnoczi wrote:
> The following conversion is safe and does not change behavior:
> 
>       GLOBAL_STATE_CODE();
>       ...
>    -  AIO_WAIT_WHILE(qemu_get_aio_context(), ...);
>    +  AIO_WAIT_WHILE_UNLOCKED(NULL, ...);
> 
> Since we're in GLOBAL_STATE_CODE(), qemu_get_aio_context() is our home
> thread's AioContext. Thus AIO_WAIT_WHILE() does not unlock the
> AioContext:
> 
>    if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>        while ((cond)) {                                           \
>            aio_poll(ctx_, true);                                  \
>            waited_ = true;                                        \
>        }                                                          \
> 
> And that means AIO_WAIT_WHILE_UNLOCKED(NULL, ...) can be substituted.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/graph-lock.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 1/3/23 21:57, Stefan Hajnoczi wrote:
> There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
> instead of AIO_WAIT_WHILE() to document that this code has already been
> audited and converted. The AioContext argument is already NULL so
> aio_context_release() is never called anyway.

Shouldn't we assert(ctx && unlock) in AIO_WAIT_WHILE_INTERNAL() then?

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/export/export.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/export/export.c b/block/export/export.c
> index 28a91c9c42..e3fee60611 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -306,7 +306,7 @@ void blk_exp_close_all_type(BlockExportType type)
>           blk_exp_request_shutdown(exp);
>       }
>   
> -    AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
> +    AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type));
>   }
>   
>   void blk_exp_close_all(void)

Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

On Thu, Mar 02, 2023 at 11:36:03AM +0100, Philippe Mathieu-Daudé wrote:
> On 1/3/23 21:57, Stefan Hajnoczi wrote:
> > There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
> > instead of AIO_WAIT_WHILE() to document that this code has already been
> > audited and converted. The AioContext argument is already NULL so
> > aio_context_release() is never called anyway.
> 
> Shouldn't we assert(ctx && unlock) in AIO_WAIT_WHILE_INTERNAL() then?

Can you show where you'd add that assertion? It's not clear to me what
the purpose is.

Stefan

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

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4209 bytes --]

On Thu, Mar 02, 2023 at 08:17:43AM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > The HMP monitor runs in the main loop thread. Calling
> 
> Correct.
> 
> > AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
> > equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
> > the AioContext and the latter's assertion that we're in the main loop
> > succeeds.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  monitor/hmp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index 2aa85d3982..5ecbdac802 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> >          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
> >          monitor_set_cur(co, &mon->common);
> >          aio_co_enter(qemu_get_aio_context(), co);
> > -        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> > +        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
> >      }
> >  
> >      qobject_unref(qdict);
> 
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 
> For an R-by, I need to understand this in more detail.  I'm not familiar
> with the innards of AIO_WAIT_WHILE() & friends, so I need to go real
> slow.
> 
> We change
> 
>     ctx from qemu_get_aio_context() to NULL
>     unlock from true to false
> 
> in
> 
>     bool waited_ = false;                                          \
>     AioWait *wait_ = &global_aio_wait;                             \
>     AioContext *ctx_ = (ctx);                                      \
>     /* Increment wait_->num_waiters before evaluating cond. */     \
>     qatomic_inc(&wait_->num_waiters);                              \
>     /* Paired with smp_mb in aio_wait_kick(). */                   \
>     smp_mb();                                                      \
>     if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>         while ((cond)) {                                           \
>             aio_poll(ctx_, true);                                  \
>             waited_ = true;                                        \
>         }                                                          \
>     } else {                                                       \
>         assert(qemu_get_current_aio_context() ==                   \
>                qemu_get_aio_context());                            \
>         while ((cond)) {                                           \
>             if (unlock && ctx_) {                                  \
>                 aio_context_release(ctx_);                         \
>             }                                                      \
>             aio_poll(qemu_get_aio_context(), true);                \
>             if (unlock && ctx_) {                                  \
>                 aio_context_acquire(ctx_);                         \
>             }                                                      \
>             waited_ = true;                                        \
>         }                                                          \
>     }                                                              \
>     qatomic_dec(&wait_->num_waiters);                              \
>     waited_; })
> 
> qemu_get_aio_context() is non-null here, correct?

qemu_get_aio_context() always returns the main loop thread's AioContext.

qemu_get_current_aio_context() returns the AioContext that was most
recently set in the my_aiocontext thread-local variable for IOThreads,
the main loop's AioContext for BQL threads, or NULL for threads
that don't use AioContext at all.

> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?

This function checks whether the given AioContext is associated with
this thread. In a BQL thread it returns true if the context is the main
loop's AioContext. In an IOThread it returns true if the context is the
IOThread's AioContext. Otherwise it returns false.

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

Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 2/3/23 14:08, Stefan Hajnoczi wrote:
> On Thu, Mar 02, 2023 at 11:36:03AM +0100, Philippe Mathieu-Daudé wrote:
>> On 1/3/23 21:57, Stefan Hajnoczi wrote:
>>> There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
>>> instead of AIO_WAIT_WHILE() to document that this code has already been
>>> audited and converted. The AioContext argument is already NULL so
>>> aio_context_release() is never called anyway.
>>
>> Shouldn't we assert(ctx && unlock) in AIO_WAIT_WHILE_INTERNAL() then?
> 
> Can you show where you'd add that assertion? It's not clear to me what
> the purpose is.

Without your series applied, using:

-- >8 --
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index dd9a7f6461..dc372e4c16 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -82,6 +82,8 @@ extern AioWait global_aio_wait;
      bool waited_ = false;                                          \
      AioWait *wait_ = &global_aio_wait;                             \
      AioContext *ctx_ = (ctx);                                      \
+    assert("Use AIO_WAIT_WHILE_UNLOCKED()" && !unlock              \
+           || (ctx && strcmp(#ctx, "qemu_get_aio_context()")));    \
      /* Increment wait_->num_waiters before evaluating cond. */     \
      qatomic_inc(&wait_->num_waiters);                              \
      /* Paired with smp_mb in aio_wait_kick(). */                   \
---

I get:

Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || (((void*)0) 
&& strcmp("((void*)0)", "qemu_get_aio_context()"))), function 
blk_exp_close_all_type, file export.c, line 309.

-> [PATCH 2/6] block: convert blk_exp_close_all_type() to 
AIO_WAIT_WHILE_UNLOCKED()

Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || 
(qemu_get_aio_context() && strcmp("qemu_get_aio_context()", 
"qemu_get_aio_context()"))), function bdrv_graph_wrlock, file 
graph-lock.c, line 130.

-> [PATCH 3/6] block: convert bdrv_graph_wrlock() to 
AIO_WAIT_WHILE_UNLOCKED()

Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || (((void*)0) 
&& strcmp("((void*)0)", "qemu_get_aio_context()"))), function 
bdrv_drain_all_begin, file io.c, line 523.

-> [PATCH 4/6] block: convert bdrv_drain_all_begin() to 
AIO_WAIT_WHILE_UNLOCKED()

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Thu, Mar 02, 2023 at 08:17:43AM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > The HMP monitor runs in the main loop thread. Calling
>> 
>> Correct.
>> 
>> > AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
>> > equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
>> > the AioContext and the latter's assertion that we're in the main loop
>> > succeeds.
>> >
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  monitor/hmp.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index 2aa85d3982..5ecbdac802 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>> >          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
>> >          monitor_set_cur(co, &mon->common);
>> >          aio_co_enter(qemu_get_aio_context(), co);
>> > -        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
>> > +        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
>> >      }
>> >  
>> >      qobject_unref(qdict);
>> 
>> Acked-by: Markus Armbruster <armbru@redhat.com>
>> 
>> For an R-by, I need to understand this in more detail.  I'm not familiar
>> with the innards of AIO_WAIT_WHILE() & friends, so I need to go real
>> slow.
>> 
>> We change
>> 
>>     ctx from qemu_get_aio_context() to NULL
>>     unlock from true to false
>> 
>> in
>> 
>>     bool waited_ = false;                                          \
>>     AioWait *wait_ = &global_aio_wait;                             \
>>     AioContext *ctx_ = (ctx);                                      \
>>     /* Increment wait_->num_waiters before evaluating cond. */     \
>>     qatomic_inc(&wait_->num_waiters);                              \
>>     /* Paired with smp_mb in aio_wait_kick(). */                   \
>>     smp_mb();                                                      \
>>     if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>>         while ((cond)) {                                           \
>>             aio_poll(ctx_, true);                                  \
>>             waited_ = true;                                        \
>>         }                                                          \
>>     } else {                                                       \
>>         assert(qemu_get_current_aio_context() ==                   \
>>                qemu_get_aio_context());                            \
>>         while ((cond)) {                                           \
>>             if (unlock && ctx_) {                                  \
>>                 aio_context_release(ctx_);                         \
>>             }                                                      \
>>             aio_poll(qemu_get_aio_context(), true);                \
>>             if (unlock && ctx_) {                                  \
>>                 aio_context_acquire(ctx_);                         \
>>             }                                                      \
>>             waited_ = true;                                        \
>>         }                                                          \
>>     }                                                              \
>>     qatomic_dec(&wait_->num_waiters);                              \
>>     waited_; })
>> 
>> qemu_get_aio_context() is non-null here, correct?
>
> qemu_get_aio_context() always returns the main loop thread's AioContext.

So it's non-null.

> qemu_get_current_aio_context() returns the AioContext that was most
> recently set in the my_aiocontext thread-local variable for IOThreads,
> the main loop's AioContext for BQL threads, or NULL for threads
> that don't use AioContext at all.
>
>> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?
>
> This function checks whether the given AioContext is associated with
> this thread. In a BQL thread it returns true if the context is the main
> loop's AioContext. In an IOThread it returns true if the context is the
> IOThread's AioContext. Otherwise it returns false.

I guess that means in_aio_context_home_thread(qemu_get_aio_context()) is
true in the main thread.

Before the patch, the if's condition is true, and we execute

           while ((cond)) {                                           \
               aio_poll(ctx_, true);                                  \
               waited_ = true;                                        \
           }                                                          \

Afterwards, it's false, and we execute

>>     }                                                              \
>>     qatomic_dec(&wait_->num_waiters);                              \
>>     waited_; })
>> 
>> qemu_get_aio_context() is non-null here, correct?
>
> qemu_get_aio_context() always returns the main loop thread's AioContext.

So it's non-null.

> qemu_get_current_aio_context() returns the AioContext that was most
> recently set in the my_aiocontext thread-local variable for IOThreads,
> the main loop's AioContext for BQL threads, or NULL for threads
> that don't use AioContext at all.
>
>> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?
>
> This function checks whether the given AioContext is associated with
> this thread. In a BQL thread it returns true if the context is the main
> loop's AioContext. In an IOThread it returns true if the context is the
> IOThread's AioContext. Otherwise it returns false.

I guess that means in_aio_context_home_thread(qemu_get_aio_context()) is
true in the main thread.

Before the patch, the if's condition is true, and we execute

           while ((cond)) {                                           \
               aio_poll(ctx_, true);                                  \
               waited_ = true;                                        \
           }                                                          \

Afterwards, it's false, and we instead execute

           assert(qemu_get_current_aio_context() ==                   \
                  qemu_get_aio_context());                            \
           while ((cond)) {                                           \
               if (unlock && ctx_) {                                  \
                   aio_context_release(ctx_);                         \
               }                                                      \
               aio_poll(qemu_get_aio_context(), true);                \
               if (unlock && ctx_) {                                  \
                   aio_context_acquire(ctx_);                         \
               }                                                      \
               waited_ = true;                                        \
           }                                                          \

The assertion is true: both operands of == are the main loop's
AioContext.

The if conditions are false, because unlock is.

Therefore, we execute the exact same code.

All correct?

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7870 bytes --]

On Thu, Mar 02, 2023 at 04:02:22PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Thu, Mar 02, 2023 at 08:17:43AM +0100, Markus Armbruster wrote:
> >> Stefan Hajnoczi <stefanha@redhat.com> writes:
> >> 
> >> > The HMP monitor runs in the main loop thread. Calling
> >> 
> >> Correct.
> >> 
> >> > AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
> >> > equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
> >> > the AioContext and the latter's assertion that we're in the main loop
> >> > succeeds.
> >> >
> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> > ---
> >> >  monitor/hmp.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> >> > index 2aa85d3982..5ecbdac802 100644
> >> > --- a/monitor/hmp.c
> >> > +++ b/monitor/hmp.c
> >> > @@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> >> >          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
> >> >          monitor_set_cur(co, &mon->common);
> >> >          aio_co_enter(qemu_get_aio_context(), co);
> >> > -        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> >> > +        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
> >> >      }
> >> >  
> >> >      qobject_unref(qdict);
> >> 
> >> Acked-by: Markus Armbruster <armbru@redhat.com>
> >> 
> >> For an R-by, I need to understand this in more detail.  I'm not familiar
> >> with the innards of AIO_WAIT_WHILE() & friends, so I need to go real
> >> slow.
> >> 
> >> We change
> >> 
> >>     ctx from qemu_get_aio_context() to NULL
> >>     unlock from true to false
> >> 
> >> in
> >> 
> >>     bool waited_ = false;                                          \
> >>     AioWait *wait_ = &global_aio_wait;                             \
> >>     AioContext *ctx_ = (ctx);                                      \
> >>     /* Increment wait_->num_waiters before evaluating cond. */     \
> >>     qatomic_inc(&wait_->num_waiters);                              \
> >>     /* Paired with smp_mb in aio_wait_kick(). */                   \
> >>     smp_mb();                                                      \
> >>     if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
> >>         while ((cond)) {                                           \
> >>             aio_poll(ctx_, true);                                  \
> >>             waited_ = true;                                        \
> >>         }                                                          \
> >>     } else {                                                       \
> >>         assert(qemu_get_current_aio_context() ==                   \
> >>                qemu_get_aio_context());                            \
> >>         while ((cond)) {                                           \
> >>             if (unlock && ctx_) {                                  \
> >>                 aio_context_release(ctx_);                         \
> >>             }                                                      \
> >>             aio_poll(qemu_get_aio_context(), true);                \
> >>             if (unlock && ctx_) {                                  \
> >>                 aio_context_acquire(ctx_);                         \
> >>             }                                                      \
> >>             waited_ = true;                                        \
> >>         }                                                          \
> >>     }                                                              \
> >>     qatomic_dec(&wait_->num_waiters);                              \
> >>     waited_; })
> >> 
> >> qemu_get_aio_context() is non-null here, correct?
> >
> > qemu_get_aio_context() always returns the main loop thread's AioContext.
> 
> So it's non-null.

Yes. Sorry, I should have answered directly :).

> > qemu_get_current_aio_context() returns the AioContext that was most
> > recently set in the my_aiocontext thread-local variable for IOThreads,
> > the main loop's AioContext for BQL threads, or NULL for threads
> > that don't use AioContext at all.
> >
> >> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?
> >
> > This function checks whether the given AioContext is associated with
> > this thread. In a BQL thread it returns true if the context is the main
> > loop's AioContext. In an IOThread it returns true if the context is the
> > IOThread's AioContext. Otherwise it returns false.
> 
> I guess that means in_aio_context_home_thread(qemu_get_aio_context()) is
> true in the main thread.
> 
> Before the patch, the if's condition is true, and we execute
> 
>            while ((cond)) {                                           \
>                aio_poll(ctx_, true);                                  \
>                waited_ = true;                                        \
>            }                                                          \
> 
> Afterwards, it's false, and we execute
> 
> >>     }                                                              \
> >>     qatomic_dec(&wait_->num_waiters);                              \
> >>     waited_; })
> >> 
> >> qemu_get_aio_context() is non-null here, correct?
> >
> > qemu_get_aio_context() always returns the main loop thread's AioContext.
> 
> So it's non-null.
> 
> > qemu_get_current_aio_context() returns the AioContext that was most
> > recently set in the my_aiocontext thread-local variable for IOThreads,
> > the main loop's AioContext for BQL threads, or NULL for threads
> > that don't use AioContext at all.
> >
> >> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?
> >
> > This function checks whether the given AioContext is associated with
> > this thread. In a BQL thread it returns true if the context is the main
> > loop's AioContext. In an IOThread it returns true if the context is the
> > IOThread's AioContext. Otherwise it returns false.
> 
> I guess that means in_aio_context_home_thread(qemu_get_aio_context()) is
> true in the main thread.

Yes.

> Before the patch, the if's condition is true, and we execute
> 
>            while ((cond)) {                                           \
>                aio_poll(ctx_, true);                                  \
>                waited_ = true;                                        \
>            }                                                          \
> 
> Afterwards, it's false, and we instead execute
> 
>            assert(qemu_get_current_aio_context() ==                   \
>                   qemu_get_aio_context());                            \
>            while ((cond)) {                                           \
>                if (unlock && ctx_) {                                  \
>                    aio_context_release(ctx_);                         \
>                }                                                      \
>                aio_poll(qemu_get_aio_context(), true);                \
>                if (unlock && ctx_) {                                  \
>                    aio_context_acquire(ctx_);                         \
>                }                                                      \
>                waited_ = true;                                        \
>            }                                                          \
> 
> The assertion is true: both operands of == are the main loop's
> AioContext.

Yes.

> The if conditions are false, because unlock is.
> 
> Therefore, we execute the exact same code.
> 
> All correct?

Yes, exactly.

Stefan

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

Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

On Thu, Mar 02, 2023 at 03:16:32PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/3/23 14:08, Stefan Hajnoczi wrote:
> > On Thu, Mar 02, 2023 at 11:36:03AM +0100, Philippe Mathieu-Daudé wrote:
> > > On 1/3/23 21:57, Stefan Hajnoczi wrote:
> > > > There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
> > > > instead of AIO_WAIT_WHILE() to document that this code has already been
> > > > audited and converted. The AioContext argument is already NULL so
> > > > aio_context_release() is never called anyway.
> > > 
> > > Shouldn't we assert(ctx && unlock) in AIO_WAIT_WHILE_INTERNAL() then?
> > 
> > Can you show where you'd add that assertion? It's not clear to me what
> > the purpose is.
> 
> Without your series applied, using:
> 
> -- >8 --
> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> index dd9a7f6461..dc372e4c16 100644
> --- a/include/block/aio-wait.h
> +++ b/include/block/aio-wait.h
> @@ -82,6 +82,8 @@ extern AioWait global_aio_wait;
>      bool waited_ = false;                                          \
>      AioWait *wait_ = &global_aio_wait;                             \
>      AioContext *ctx_ = (ctx);                                      \
> +    assert("Use AIO_WAIT_WHILE_UNLOCKED()" && !unlock              \
> +           || (ctx && strcmp(#ctx, "qemu_get_aio_context()")));    \
>      /* Increment wait_->num_waiters before evaluating cond. */     \
>      qatomic_inc(&wait_->num_waiters);                              \
>      /* Paired with smp_mb in aio_wait_kick(). */                   \
> ---

Ah, I see. You are suggesting adding an assertion to catch
AIO_WAIT_WHILE() usage in cases where AIO_WAIT_WHILE_UNLOCKED() should
be used instead.

I think it's a bit too clever, especially the strcmp trick, but we can
add it as the final patch. I have grepped the code and don't think there
are any remaining instances where the assertion fails.

Stefan

> I get:
> 
> Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || (((void*)0) &&
> strcmp("((void*)0)", "qemu_get_aio_context()"))), function
> blk_exp_close_all_type, file export.c, line 309.
> 
> -> [PATCH 2/6] block: convert blk_exp_close_all_type() to
> AIO_WAIT_WHILE_UNLOCKED()
> 
> Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 ||
> (qemu_get_aio_context() && strcmp("qemu_get_aio_context()",
> "qemu_get_aio_context()"))), function bdrv_graph_wrlock, file graph-lock.c,
> line 130.
> 
> -> [PATCH 3/6] block: convert bdrv_graph_wrlock() to
> AIO_WAIT_WHILE_UNLOCKED()
> 
> Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || (((void*)0) &&
> strcmp("((void*)0)", "qemu_get_aio_context()"))), function
> bdrv_drain_all_begin, file io.c, line 523.
> 
> -> [PATCH 4/6] block: convert bdrv_drain_all_begin() to
> AIO_WAIT_WHILE_UNLOCKED()
> 

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

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Thu, Mar 02, 2023 at 04:02:22PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > On Thu, Mar 02, 2023 at 08:17:43AM +0100, Markus Armbruster wrote:
>> >> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> >> 
>> >> > The HMP monitor runs in the main loop thread. Calling
>> >> 
>> >> Correct.
>> >> 
>> >> > AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
>> >> > equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
>> >> > the AioContext and the latter's assertion that we're in the main loop
>> >> > succeeds.
>> >> >
>> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> > ---
>> >> >  monitor/hmp.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> >> > index 2aa85d3982..5ecbdac802 100644
>> >> > --- a/monitor/hmp.c
>> >> > +++ b/monitor/hmp.c
>> >> > @@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>> >> >          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
>> >> >          monitor_set_cur(co, &mon->common);
>> >> >          aio_co_enter(qemu_get_aio_context(), co);
>> >> > -        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
>> >> > +        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
>> >> >      }
>> >> >  
>> >> >      qobject_unref(qdict);
>> >> 
>> >> Acked-by: Markus Armbruster <armbru@redhat.com>
>> >> 
>> >> For an R-by, I need to understand this in more detail.  I'm not familiar
>> >> with the innards of AIO_WAIT_WHILE() & friends, so I need to go real
>> >> slow.
>> >> 
>> >> We change
>> >> 
>> >>     ctx from qemu_get_aio_context() to NULL
>> >>     unlock from true to false
>> >> 
>> >> in
>> >> 
>> >>     bool waited_ = false;                                          \
>> >>     AioWait *wait_ = &global_aio_wait;                             \
>> >>     AioContext *ctx_ = (ctx);                                      \
>> >>     /* Increment wait_->num_waiters before evaluating cond. */     \
>> >>     qatomic_inc(&wait_->num_waiters);                              \
>> >>     /* Paired with smp_mb in aio_wait_kick(). */                   \
>> >>     smp_mb();                                                      \
>> >>     if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>> >>         while ((cond)) {                                           \
>> >>             aio_poll(ctx_, true);                                  \
>> >>             waited_ = true;                                        \
>> >>         }                                                          \
>> >>     } else {                                                       \
>> >>         assert(qemu_get_current_aio_context() ==                   \
>> >>                qemu_get_aio_context());                            \
>> >>         while ((cond)) {                                           \
>> >>             if (unlock && ctx_) {                                  \
>> >>                 aio_context_release(ctx_);                         \
>> >>             }                                                      \
>> >>             aio_poll(qemu_get_aio_context(), true);                \
>> >>             if (unlock && ctx_) {                                  \
>> >>                 aio_context_acquire(ctx_);                         \
>> >>             }                                                      \
>> >>             waited_ = true;                                        \
>> >>         }                                                          \
>> >>     }                                                              \
>> >>     qatomic_dec(&wait_->num_waiters);                              \
>> >>     waited_; })
>> >> 
>> >> qemu_get_aio_context() is non-null here, correct?
>> >
>> > qemu_get_aio_context() always returns the main loop thread's AioContext.
>> 
>> So it's non-null.
>
> Yes. Sorry, I should have answered directly :).
>
>> > qemu_get_current_aio_context() returns the AioContext that was most
>> > recently set in the my_aiocontext thread-local variable for IOThreads,
>> > the main loop's AioContext for BQL threads, or NULL for threads
>> > that don't use AioContext at all.
>> >
>> >> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?
>> >
>> > This function checks whether the given AioContext is associated with
>> > this thread. In a BQL thread it returns true if the context is the main
>> > loop's AioContext. In an IOThread it returns true if the context is the
>> > IOThread's AioContext. Otherwise it returns false.
>> 
>> I guess that means in_aio_context_home_thread(qemu_get_aio_context()) is
>> true in the main thread.
>> 
>> Before the patch, the if's condition is true, and we execute
>> 
>>            while ((cond)) {                                           \
>>                aio_poll(ctx_, true);                                  \
>>                waited_ = true;                                        \
>>            }                                                          \
>> 
>> Afterwards, it's false, and we execute
>> 
>> >>     }                                                              \
>> >>     qatomic_dec(&wait_->num_waiters);                              \
>> >>     waited_; })
>> >> 
>> >> qemu_get_aio_context() is non-null here, correct?
>> >
>> > qemu_get_aio_context() always returns the main loop thread's AioContext.
>> 
>> So it's non-null.
>> 
>> > qemu_get_current_aio_context() returns the AioContext that was most
>> > recently set in the my_aiocontext thread-local variable for IOThreads,
>> > the main loop's AioContext for BQL threads, or NULL for threads
>> > that don't use AioContext at all.
>> >
>> >> What's the value of in_aio_context_home_thread(qemu_get_aio_context())?
>> >
>> > This function checks whether the given AioContext is associated with
>> > this thread. In a BQL thread it returns true if the context is the main
>> > loop's AioContext. In an IOThread it returns true if the context is the
>> > IOThread's AioContext. Otherwise it returns false.
>> 
>> I guess that means in_aio_context_home_thread(qemu_get_aio_context()) is
>> true in the main thread.
>
> Yes.
>
>> Before the patch, the if's condition is true, and we execute
>> 
>>            while ((cond)) {                                           \
>>                aio_poll(ctx_, true);                                  \
>>                waited_ = true;                                        \
>>            }                                                          \
>> 
>> Afterwards, it's false, and we instead execute
>> 
>>            assert(qemu_get_current_aio_context() ==                   \
>>                   qemu_get_aio_context());                            \
>>            while ((cond)) {                                           \
>>                if (unlock && ctx_) {                                  \
>>                    aio_context_release(ctx_);                         \
>>                }                                                      \
>>                aio_poll(qemu_get_aio_context(), true);                \
>>                if (unlock && ctx_) {                                  \
>>                    aio_context_acquire(ctx_);                         \
>>                }                                                      \
>>                waited_ = true;                                        \
>>            }                                                          \
>> 
>> The assertion is true: both operands of == are the main loop's
>> AioContext.
>
> Yes.
>
>> The if conditions are false, because unlock is.
>> 
>> Therefore, we execute the exact same code.
>> 
>> All correct?
>
> Yes, exactly.

Thank you!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> monitor_cleanup() is called from the main loop thread. Calling
>
> Correct.
>
>> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
>> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
>> the AioContext and the latter's assertion that we're in the main loop
>> succeeds.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  monitor/monitor.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 8dc96f6af9..602535696c 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -666,7 +666,7 @@ void monitor_cleanup(void)
>>       * We need to poll both qemu_aio_context and iohandler_ctx to make
>>       * sure that the dispatcher coroutine keeps making progress and
>>       * eventually terminates.  qemu_aio_context is automatically
>> -     * polled by calling AIO_WAIT_WHILE on it, but we must poll
>> +     * polled by calling AIO_WAIT_WHILE_UNLOCKED on it, but we must poll
>>       * iohandler_ctx manually.
>>       *
>>       * Letting the iothread continue while shutting down the dispatcher
>> @@ -679,7 +679,7 @@ void monitor_cleanup(void)
>>          aio_co_wake(qmp_dispatcher_co);
>>      }
>>  
>> -    AIO_WAIT_WHILE(qemu_get_aio_context(),
>> +    AIO_WAIT_WHILE_UNLOCKED(NULL,
>>                     (aio_poll(iohandler_get_aio_context(), false),
>>                      qatomic_mb_read(&qmp_dispatcher_co_busy)));
>
> Acked-by: Markus Armbruster <armbru@redhat.com>
>
> For an R-by, I need to understand this in more detail.  See my reply to
> the previous patch.

Upgrading to
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Kevin Wolf]

Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> There is no need for the AioContext lock in bdrv_drain_all() because
> nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.
> 
> Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
> the future it can be removed.

It can be removed for all callers that run in the main loop context. For
code running in an iothread, it's still important to pass a non-NULL
context. This makes me doubt that the ctx parameter can really be
removed without changing more.

Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and
to poll qemu_get_current_aio_context() instead of ctx_ or the main
context?

> There is an assertion in
> AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
> we would lose that check by dropping the argument. However, that was a
> precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
> duplicate check. So I think we won't lose much by dropping it, but let's
> do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
> confirm this is the case.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Yes, it seems that we don't lose much, except maybe some consistency in
the intermediate state. The commit message could state a bit more
directly what we gain, though. Since you mention removing the parameter
as a future possibility, I assume that's the goal with it, but I
wouldn't be sure just from reading the commit message.

Kevin

Re: [PATCH 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

[Kevin Wolf]

Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> AIO_WAIT_WHILE_UNLOCKED() is the future replacement for AIO_WAIT_WHILE(). Most
> callers haven't been converted yet because they rely on the AioContext lock. I
> looked through the code and found the easy cases that can be converted today.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3556 bytes --]

On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote:
> Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> > There is no need for the AioContext lock in bdrv_drain_all() because
> > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.
> > 
> > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
> > the future it can be removed.
> 
> It can be removed for all callers that run in the main loop context. For
> code running in an iothread, it's still important to pass a non-NULL
> context. This makes me doubt that the ctx parameter can really be
> removed without changing more.
> 
> Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and
> to poll qemu_get_current_aio_context() instead of ctx_ or the main
> context?

This is what I'd like once everything has been converted to
AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it
AIO_WAIT_WHILE() again:

  #define AIO_WAIT_WHILE(cond) ({                                    \
      bool waited_ = false;                                          \
      AioWait *wait_ = &global_aio_wait;                             \
      /* Increment wait_->num_waiters before evaluating cond. */     \
      qatomic_inc(&wait_->num_waiters);                              \
      /* Paired with smp_mb in aio_wait_kick(). */                   \
      smp_mb();                                                      \
      while ((cond)) {                                               \
          aio_poll(qemu_get_current_aio_context(), true);            \
          waited_ = true;                                            \
      }                                                              \
      qatomic_dec(&wait_->num_waiters);                              \
      waited_; })

However, I just realized this only works in the main loop thread because
that's where aio_wait_kick() notifications are received. An IOThread
running AIO_WAIT_WHILE() won't be woken when another thread (including
the main loop thread) calls aio_wait_kick().

I would propose introducing a QemuCond for each condition that we wait
on, but QemuCond lacks event loop integration. The current thread would
be unable to run aio_poll() while also waiting on a QemuCond.

Life outside coroutines is hard, man! I need to think about this more.
Luckily this problem doesn't block this patch series.

> > There is an assertion in
> > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
> > we would lose that check by dropping the argument. However, that was a
> > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
> > duplicate check. So I think we won't lose much by dropping it, but let's
> > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
> > confirm this is the case.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Yes, it seems that we don't lose much, except maybe some consistency in
> the intermediate state. The commit message could state a bit more
> directly what we gain, though. Since you mention removing the parameter
> as a future possibility, I assume that's the goal with it, but I
> wouldn't be sure just from reading the commit message.

AIO_WAIT_WHILE() callers need to be weened of the AioContext lock.
That's the main motivation and this patch series converts the easy cases
where we already don't need the lock. Dropping the function argument
eventually is a side benefit.

Stefan

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

Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 1/3/23 21:57, Stefan Hajnoczi wrote:
> There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
> instead of AIO_WAIT_WHILE() to document that this code has already been
> audited and converted. The AioContext argument is already NULL so
> aio_context_release() is never called anyway.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/export/export.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/6] block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 2/3/23 11:19, Philippe Mathieu-Daudé wrote:
> On 1/3/23 21:57, Stefan Hajnoczi wrote:
>> The following conversion is safe and does not change behavior:
>>
>>       GLOBAL_STATE_CODE();
>>       ...
>>    -  AIO_WAIT_WHILE(qemu_get_aio_context(), ...);
>>    +  AIO_WAIT_WHILE_UNLOCKED(NULL, ...);
>>
>> Since we're in GLOBAL_STATE_CODE(), qemu_get_aio_context() is our home
>> thread's AioContext. Thus AIO_WAIT_WHILE() does not unlock the
>> AioContext:
>>
>>    if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>>        while ((cond)) {                                           \
>>            aio_poll(ctx_, true);                                  \
>>            waited_ = true;                                        \
>>        }                                                          \
>>
>> And that means AIO_WAIT_WHILE_UNLOCKED(NULL, ...) can be substituted.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/graph-lock.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/6] block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 1/3/23 21:57, Stefan Hajnoczi wrote:
> Since the AioContext argument was already NULL, AIO_WAIT_WHILE() was
> never going to unlock the AioContext. Therefore it is possible to
> replace AIO_WAIT_WHILE() with AIO_WAIT_WHILE_UNLOCKED().
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/io.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 8974d46941..db438c7657 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -520,7 +520,7 @@ void bdrv_drain_all_begin(void)
>       bdrv_drain_all_begin_nopoll();
>   
>       /* Now poll the in-flight requests */
> -    AIO_WAIT_WHILE(NULL, bdrv_drain_all_poll());
> +    AIO_WAIT_WHILE_UNLOCKED(NULL, bdrv_drain_all_poll());
>   
>       while ((bs = bdrv_next_all_states(bs))) {
>           bdrv_drain_assert_idle(bs);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 1/3/23 21:58, Stefan Hajnoczi wrote:
> The HMP monitor runs in the main loop thread. Calling
> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
> the AioContext and the latter's assertion that we're in the main loop
> succeeds.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   monitor/hmp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

[Philippe Mathieu-Daudé]

On 1/3/23 21:58, Stefan Hajnoczi wrote:
> monitor_cleanup() is called from the main loop thread. Calling
> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
> the AioContext and the latter's assertion that we're in the main loop
> succeeds.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   monitor/monitor.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 5031 bytes --]

Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben:
> On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote:
> > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> > > There is no need for the AioContext lock in bdrv_drain_all() because
> > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.
> > > 
> > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
> > > the future it can be removed.
> > 
> > It can be removed for all callers that run in the main loop context. For
> > code running in an iothread, it's still important to pass a non-NULL
> > context. This makes me doubt that the ctx parameter can really be
> > removed without changing more.
> > 
> > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and
> > to poll qemu_get_current_aio_context() instead of ctx_ or the main
> > context?
> 
> This is what I'd like once everything has been converted to
> AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it
> AIO_WAIT_WHILE() again:
> 
>   #define AIO_WAIT_WHILE(cond) ({                                    \
>       bool waited_ = false;                                          \
>       AioWait *wait_ = &global_aio_wait;                             \
>       /* Increment wait_->num_waiters before evaluating cond. */     \
>       qatomic_inc(&wait_->num_waiters);                              \
>       /* Paired with smp_mb in aio_wait_kick(). */                   \
>       smp_mb();                                                      \
>       while ((cond)) {                                               \
>           aio_poll(qemu_get_current_aio_context(), true);            \
>           waited_ = true;                                            \
>       }                                                              \
>       qatomic_dec(&wait_->num_waiters);                              \
>       waited_; })

Ok, yes, this is what I tried to describe above.

> However, I just realized this only works in the main loop thread because
> that's where aio_wait_kick() notifications are received. An IOThread
> running AIO_WAIT_WHILE() won't be woken when another thread (including
> the main loop thread) calls aio_wait_kick().

Which is of course a limitation we already have today. You can wait for
things in your own iothread, or for all threads from the main loop.

However, in the future multiqueue world, the first case probably becomes
pretty much useless because even for the same node, you could get
activity in any thread.

So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is
probably a good idea anyway, but I'm not entirely sure how many places
we currently have where it's called from an iothread. I know the drain
in mirror_run(), but Emanuele already had a patch in his queue where
bdrv_co_yield_to_drain() schedules drain in the main context, so if that
works, mirror_run() would be solved.

https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad

> I would propose introducing a QemuCond for each condition that we wait
> on, but QemuCond lacks event loop integration. The current thread would
> be unable to run aio_poll() while also waiting on a QemuCond.
> 
> Life outside coroutines is hard, man! I need to think about this more.
> Luckily this problem doesn't block this patch series.

I hope that we don't really need all of this if we can limit running
synchronous code to the main loop.

> > > There is an assertion in
> > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
> > > we would lose that check by dropping the argument. However, that was a
> > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
> > > duplicate check. So I think we won't lose much by dropping it, but let's
> > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
> > > confirm this is the case.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Yes, it seems that we don't lose much, except maybe some consistency in
> > the intermediate state. The commit message could state a bit more
> > directly what we gain, though. Since you mention removing the parameter
> > as a future possibility, I assume that's the goal with it, but I
> > wouldn't be sure just from reading the commit message.
> 
> AIO_WAIT_WHILE() callers need to be weened of the AioContext lock.
> That's the main motivation and this patch series converts the easy
> cases where we already don't need the lock. Dropping the function
> argument eventually is a side benefit.

Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with
ctx instead of NULL. So moving to NULL is a separate change that needs a
separate explanation. You could even argue that it should be a separate
patch if it's an independent change.

Or am I missing something and keeping ctx would actually break things?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5683 bytes --]

On Wed, Mar 08, 2023 at 09:48:17AM +0100, Kevin Wolf wrote:
> Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben:
> > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote:
> > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> > > > There is no need for the AioContext lock in bdrv_drain_all() because
> > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.
> > > > 
> > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
> > > > the future it can be removed.
> > > 
> > > It can be removed for all callers that run in the main loop context. For
> > > code running in an iothread, it's still important to pass a non-NULL
> > > context. This makes me doubt that the ctx parameter can really be
> > > removed without changing more.
> > > 
> > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and
> > > to poll qemu_get_current_aio_context() instead of ctx_ or the main
> > > context?
> > 
> > This is what I'd like once everything has been converted to
> > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it
> > AIO_WAIT_WHILE() again:
> > 
> >   #define AIO_WAIT_WHILE(cond) ({                                    \
> >       bool waited_ = false;                                          \
> >       AioWait *wait_ = &global_aio_wait;                             \
> >       /* Increment wait_->num_waiters before evaluating cond. */     \
> >       qatomic_inc(&wait_->num_waiters);                              \
> >       /* Paired with smp_mb in aio_wait_kick(). */                   \
> >       smp_mb();                                                      \
> >       while ((cond)) {                                               \
> >           aio_poll(qemu_get_current_aio_context(), true);            \
> >           waited_ = true;                                            \
> >       }                                                              \
> >       qatomic_dec(&wait_->num_waiters);                              \
> >       waited_; })
> 
> Ok, yes, this is what I tried to describe above.
> 
> > However, I just realized this only works in the main loop thread because
> > that's where aio_wait_kick() notifications are received. An IOThread
> > running AIO_WAIT_WHILE() won't be woken when another thread (including
> > the main loop thread) calls aio_wait_kick().
> 
> Which is of course a limitation we already have today. You can wait for
> things in your own iothread, or for all threads from the main loop.
> 
> However, in the future multiqueue world, the first case probably becomes
> pretty much useless because even for the same node, you could get
> activity in any thread.
> 
> So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is
> probably a good idea anyway, but I'm not entirely sure how many places
> we currently have where it's called from an iothread. I know the drain
> in mirror_run(), but Emanuele already had a patch in his queue where
> bdrv_co_yield_to_drain() schedules drain in the main context, so if that
> works, mirror_run() would be solved.
> 
> https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad
> 
> > I would propose introducing a QemuCond for each condition that we wait
> > on, but QemuCond lacks event loop integration. The current thread would
> > be unable to run aio_poll() while also waiting on a QemuCond.
> > 
> > Life outside coroutines is hard, man! I need to think about this more.
> > Luckily this problem doesn't block this patch series.
> 
> I hope that we don't really need all of this if we can limit running
> synchronous code to the main loop.

Great idea, I think you're right.

I'll audit the code to find the IOThread AIO_WAIT_WHILE() callers and
maybe a future patch series can work on that.

> > > > There is an assertion in
> > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
> > > > we would lose that check by dropping the argument. However, that was a
> > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
> > > > duplicate check. So I think we won't lose much by dropping it, but let's
> > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
> > > > confirm this is the case.
> > > > 
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > 
> > > Yes, it seems that we don't lose much, except maybe some consistency in
> > > the intermediate state. The commit message could state a bit more
> > > directly what we gain, though. Since you mention removing the parameter
> > > as a future possibility, I assume that's the goal with it, but I
> > > wouldn't be sure just from reading the commit message.
> > 
> > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock.
> > That's the main motivation and this patch series converts the easy
> > cases where we already don't need the lock. Dropping the function
> > argument eventually is a side benefit.
> 
> Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with
> ctx instead of NULL. So moving to NULL is a separate change that needs a
> separate explanation. You could even argue that it should be a separate
> patch if it's an independent change.
> 
> Or am I missing something and keeping ctx would actually break things?

Yes, ctx argument does not need to be modified when converting from
AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED(). Passing it bothers me
because we don't really use it when unlock=false.

Would you like me to keep ctx non-NULL for now?

Stefan

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

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 6210 bytes --]

Am 08.03.2023 um 15:26 hat Stefan Hajnoczi geschrieben:
> On Wed, Mar 08, 2023 at 09:48:17AM +0100, Kevin Wolf wrote:
> > Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben:
> > > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote:
> > > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> > > > > There is no need for the AioContext lock in bdrv_drain_all() because
> > > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.
> > > > > 
> > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
> > > > > the future it can be removed.
> > > > 
> > > > It can be removed for all callers that run in the main loop context. For
> > > > code running in an iothread, it's still important to pass a non-NULL
> > > > context. This makes me doubt that the ctx parameter can really be
> > > > removed without changing more.
> > > > 
> > > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and
> > > > to poll qemu_get_current_aio_context() instead of ctx_ or the main
> > > > context?
> > > 
> > > This is what I'd like once everything has been converted to
> > > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it
> > > AIO_WAIT_WHILE() again:
> > > 
> > >   #define AIO_WAIT_WHILE(cond) ({                                    \
> > >       bool waited_ = false;                                          \
> > >       AioWait *wait_ = &global_aio_wait;                             \
> > >       /* Increment wait_->num_waiters before evaluating cond. */     \
> > >       qatomic_inc(&wait_->num_waiters);                              \
> > >       /* Paired with smp_mb in aio_wait_kick(). */                   \
> > >       smp_mb();                                                      \
> > >       while ((cond)) {                                               \
> > >           aio_poll(qemu_get_current_aio_context(), true);            \
> > >           waited_ = true;                                            \
> > >       }                                                              \
> > >       qatomic_dec(&wait_->num_waiters);                              \
> > >       waited_; })
> > 
> > Ok, yes, this is what I tried to describe above.
> > 
> > > However, I just realized this only works in the main loop thread because
> > > that's where aio_wait_kick() notifications are received. An IOThread
> > > running AIO_WAIT_WHILE() won't be woken when another thread (including
> > > the main loop thread) calls aio_wait_kick().
> > 
> > Which is of course a limitation we already have today. You can wait for
> > things in your own iothread, or for all threads from the main loop.
> > 
> > However, in the future multiqueue world, the first case probably becomes
> > pretty much useless because even for the same node, you could get
> > activity in any thread.
> > 
> > So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is
> > probably a good idea anyway, but I'm not entirely sure how many places
> > we currently have where it's called from an iothread. I know the drain
> > in mirror_run(), but Emanuele already had a patch in his queue where
> > bdrv_co_yield_to_drain() schedules drain in the main context, so if that
> > works, mirror_run() would be solved.
> > 
> > https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad
> > 
> > > I would propose introducing a QemuCond for each condition that we wait
> > > on, but QemuCond lacks event loop integration. The current thread would
> > > be unable to run aio_poll() while also waiting on a QemuCond.
> > > 
> > > Life outside coroutines is hard, man! I need to think about this more.
> > > Luckily this problem doesn't block this patch series.
> > 
> > I hope that we don't really need all of this if we can limit running
> > synchronous code to the main loop.
> 
> Great idea, I think you're right.
> 
> I'll audit the code to find the IOThread AIO_WAIT_WHILE() callers and
> maybe a future patch series can work on that.
> 
> > > > > There is an assertion in
> > > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
> > > > > we would lose that check by dropping the argument. However, that was a
> > > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
> > > > > duplicate check. So I think we won't lose much by dropping it, but let's
> > > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
> > > > > confirm this is the case.
> > > > > 
> > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > 
> > > > Yes, it seems that we don't lose much, except maybe some consistency in
> > > > the intermediate state. The commit message could state a bit more
> > > > directly what we gain, though. Since you mention removing the parameter
> > > > as a future possibility, I assume that's the goal with it, but I
> > > > wouldn't be sure just from reading the commit message.
> > > 
> > > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock.
> > > That's the main motivation and this patch series converts the easy
> > > cases where we already don't need the lock. Dropping the function
> > > argument eventually is a side benefit.
> > 
> > Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with
> > ctx instead of NULL. So moving to NULL is a separate change that needs a
> > separate explanation. You could even argue that it should be a separate
> > patch if it's an independent change.
> > 
> > Or am I missing something and keeping ctx would actually break things?
> 
> Yes, ctx argument does not need to be modified when converting from
> AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED(). Passing it bothers me
> because we don't really use it when unlock=false.
> 
> Would you like me to keep ctx non-NULL for now?

I don't really mind doing both changes in one commit because they are so
small, but at least I'd like the commit message to be more explicit
about the eventual goal we have with switching to NULL instead of just
stating that it's odd, but harmless.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 6539 bytes --]

On Wed, Mar 08, 2023 at 06:25:43PM +0100, Kevin Wolf wrote:
> Am 08.03.2023 um 15:26 hat Stefan Hajnoczi geschrieben:
> > On Wed, Mar 08, 2023 at 09:48:17AM +0100, Kevin Wolf wrote:
> > > Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben:
> > > > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote:
> > > > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben:
> > > > > > There is no need for the AioContext lock in bdrv_drain_all() because
> > > > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.
> > > > > > 
> > > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In
> > > > > > the future it can be removed.
> > > > > 
> > > > > It can be removed for all callers that run in the main loop context. For
> > > > > code running in an iothread, it's still important to pass a non-NULL
> > > > > context. This makes me doubt that the ctx parameter can really be
> > > > > removed without changing more.
> > > > > 
> > > > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and
> > > > > to poll qemu_get_current_aio_context() instead of ctx_ or the main
> > > > > context?
> > > > 
> > > > This is what I'd like once everything has been converted to
> > > > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it
> > > > AIO_WAIT_WHILE() again:
> > > > 
> > > >   #define AIO_WAIT_WHILE(cond) ({                                    \
> > > >       bool waited_ = false;                                          \
> > > >       AioWait *wait_ = &global_aio_wait;                             \
> > > >       /* Increment wait_->num_waiters before evaluating cond. */     \
> > > >       qatomic_inc(&wait_->num_waiters);                              \
> > > >       /* Paired with smp_mb in aio_wait_kick(). */                   \
> > > >       smp_mb();                                                      \
> > > >       while ((cond)) {                                               \
> > > >           aio_poll(qemu_get_current_aio_context(), true);            \
> > > >           waited_ = true;                                            \
> > > >       }                                                              \
> > > >       qatomic_dec(&wait_->num_waiters);                              \
> > > >       waited_; })
> > > 
> > > Ok, yes, this is what I tried to describe above.
> > > 
> > > > However, I just realized this only works in the main loop thread because
> > > > that's where aio_wait_kick() notifications are received. An IOThread
> > > > running AIO_WAIT_WHILE() won't be woken when another thread (including
> > > > the main loop thread) calls aio_wait_kick().
> > > 
> > > Which is of course a limitation we already have today. You can wait for
> > > things in your own iothread, or for all threads from the main loop.
> > > 
> > > However, in the future multiqueue world, the first case probably becomes
> > > pretty much useless because even for the same node, you could get
> > > activity in any thread.
> > > 
> > > So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is
> > > probably a good idea anyway, but I'm not entirely sure how many places
> > > we currently have where it's called from an iothread. I know the drain
> > > in mirror_run(), but Emanuele already had a patch in his queue where
> > > bdrv_co_yield_to_drain() schedules drain in the main context, so if that
> > > works, mirror_run() would be solved.
> > > 
> > > https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad
> > > 
> > > > I would propose introducing a QemuCond for each condition that we wait
> > > > on, but QemuCond lacks event loop integration. The current thread would
> > > > be unable to run aio_poll() while also waiting on a QemuCond.
> > > > 
> > > > Life outside coroutines is hard, man! I need to think about this more.
> > > > Luckily this problem doesn't block this patch series.
> > > 
> > > I hope that we don't really need all of this if we can limit running
> > > synchronous code to the main loop.
> > 
> > Great idea, I think you're right.
> > 
> > I'll audit the code to find the IOThread AIO_WAIT_WHILE() callers and
> > maybe a future patch series can work on that.
> > 
> > > > > > There is an assertion in
> > > > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and
> > > > > > we would lose that check by dropping the argument. However, that was a
> > > > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a
> > > > > > duplicate check. So I think we won't lose much by dropping it, but let's
> > > > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to
> > > > > > confirm this is the case.
> > > > > > 
> > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > 
> > > > > Yes, it seems that we don't lose much, except maybe some consistency in
> > > > > the intermediate state. The commit message could state a bit more
> > > > > directly what we gain, though. Since you mention removing the parameter
> > > > > as a future possibility, I assume that's the goal with it, but I
> > > > > wouldn't be sure just from reading the commit message.
> > > > 
> > > > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock.
> > > > That's the main motivation and this patch series converts the easy
> > > > cases where we already don't need the lock. Dropping the function
> > > > argument eventually is a side benefit.
> > > 
> > > Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with
> > > ctx instead of NULL. So moving to NULL is a separate change that needs a
> > > separate explanation. You could even argue that it should be a separate
> > > patch if it's an independent change.
> > > 
> > > Or am I missing something and keeping ctx would actually break things?
> > 
> > Yes, ctx argument does not need to be modified when converting from
> > AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED(). Passing it bothers me
> > because we don't really use it when unlock=false.
> > 
> > Would you like me to keep ctx non-NULL for now?
> 
> I don't really mind doing both changes in one commit because they are so
> small, but at least I'd like the commit message to be more explicit
> about the eventual goal we have with switching to NULL instead of just
> stating that it's odd, but harmless.

Got it, I'll send another revision.

Stefan

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