Stefan Hajnoczi <stefa...@redhat.com> writes:
> On Thu, Mar 02, 2023 at 04:02:22PM +0100, Markus Armbruster wrote:
>> 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.
>
> 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 <arm...@redhat.com>
