Stefan Hajnoczi <stefa...@redhat.com> writes:
> On Thu, Mar 02, 2023 at 08:17:43AM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefa...@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 <stefa...@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 <arm...@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?
