Re: [PATCH v6 01/33] main-loop.h: introduce qemu_in_main_thread()

[Kevin Wolf <kwolf@redhat.com>]

Am 31.01.2022 um 12:25 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> On 27/01/2022 11:56, Kevin Wolf wrote:
> > Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
> >> When invoked from the main loop, this function is the same
> >> as qemu_mutex_iothread_locked, and returns true if the BQL is held.
> > 
> > So its name is misleading because it doesn't answer the question whether
> > we're in the main thread, but whethere we're holding the BQL (which is
> > mostly equivalent to holding an AioContext lock, not being in the home
> > thread of that AioContext).
> > 
> >> When invoked from iothreads or tests, it returns true only
> >> if the current AioContext is the Main Loop.
> >>
> >> This essentially just extends qemu_mutex_iothread_locked to work
> >> also in unit tests or other users like storage-daemon, that run
> >> in the Main Loop but end up using the implementation in
> >> stubs/iothread-lock.c.
> >>
> >> Using qemu_mutex_iothread_locked in unit tests defaults to false
> >> because they use the implementation in stubs/iothread-lock,
> >> making all assertions added in next patches fail despite the
> >> AioContext is still the main loop.
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > This adds a new function that is almost the same as an existing
> > function, but the documentation is unclear when I should use one or the
> > other.
> 
> Why do you think it is unclear? as explained in commit and
> documentation, unit tests for example use stubs/iothread-lock, which
> returns always false. So we can (and should!) use this function in
> APIs invoked by unit tests, because qemu_mutex_iothread_locked will
> just return false, making all tests crash. On the other side, I am
> pretty sure it won't cause any failure when running QEMU or
> qemu-iotests, because there most of the API use the cpu
> implementation.

I feel "use this function if your code is going to be used by unit
tests, but use qemu_mutex_iothread_locked() otherwise" is a very strange
interface. I'm relatively sure that qemu_mutex_iothread_locked() isn't
primarily used to make unit tests crash.

Documentation and the definition of the interface of a function
shouldn't be about the caller, but about the semantics of the function
itself. So, which question does qemu_mutex_iothread_locked() answer, and
which question does qemu_in_main_thread() answer?

The meaning of their result must be different, otherwise there wouldn't
be two different functions that must be used in different contexts.

> > What are the use cases for the existing qemu_mutex_iothread_locked()
> > stub where we rely on false being returned?
> 
> I don't think we ever rely on stub being false. There are only 2
> places where I see !qemu_mutex_iothread_locked(), and are in
> helper_regs.c and cpus.c
> 
> So being cpu related functions they use the cpu implementation.
> 
> However, commit 5f50be9b5810293141bb53cfd0cb46e765367d56 changed the
> stub to return false for a specific reason.

I must admit I don't really understand the reasoning there:

    With this change, the stub qemu_mutex_iothread_locked() must be changed
    from true to false.  The previous value of true was needed because the
    main thread did not have an AioContext in the thread-local variable,
    but now it does have one.

This explains why it _can_ be changed to false for this caller, but not
why it _must_ be changed.

> > If there aren't any, then maybe we should change the stub for that one
> > instead of adding a new function that behaves the same in the system
> > emulator and different only when it's stubbed out?
> 
> I don't think we can replace it, bcause stubs/qemu_in_main_thread()
> calls qemu_get_current_aio_context, that in turn calls
> qemu_mutex_iothread_locked. So we implicitly rely on that "false".

qemu_get_current_aio_context() will just return my_aiocontext, which is
qemu_aio_context because all tools call qemu_init_main_loop(). I don't
think it will ever call qemu_mutex_iothread_locked() in tools, except
for worker threads.

Ooh, and that's why we return false. Because the only kind of non-main
threads accessing global state in tools are worker threads which never
hold the BQL in the system emulator.

So is the problem with the unit tests really just that they never call
qemu_init_main_loop() and therefore never set the AioContext for the
main thread?

Kevin