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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]