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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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.

> 
> 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.

> 
> 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".

Thank you,
Emanuele