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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature