Re: [PATCH 1/2] system/main: transfer replay mutex ownership from main thread to main loop thread

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 4/14/25 19:41, Nicholas Piggin wrote:
> On Tue Apr 15, 2025 at 1:24 AM AEST, Pierrick Bouvier wrote:
>> On 4/14/25 03:25, Philippe Mathieu-Daudé wrote:
>>> On 12/4/25 19:24, Pierrick Bouvier wrote:
>>>> On 4/11/25 22:30, Nicholas Piggin wrote:
>>>>> On Fri Apr 11, 2025 at 8:55 AM AEST, Pierrick Bouvier wrote:
>>>>>> On MacOS, UI event loop has to be ran in the main thread of a process.
>>>>>> Because of that restriction, on this platform, qemu main event loop is
>>>>>> ran on another thread [1].
>>>>>>
>>>>>> This breaks record/replay feature, which expects thread running
>>>>>> qemu_init
>>>>>> to initialize hold this lock, breaking associated functional tests on
>>>>>> MacOS.
>>>>>>
>>>>>> Thus, as a generalization, and similar to how BQL is handled, we release
>>>>>> it after init, and reacquire the lock before entering main event loop,
>>>>>> avoiding a special case if a separate thread is used.
>>>>>>
>>>>>> Tested on MacOS with:
>>>>>> $ meson test -C build --setup thorough --print-errorlogs \
>>>>>> func-x86_64-x86_64_replay func-arm-arm_replay func-aarch64-
>>>>>> aarch64_replay
>>>>>> $ ./build/qemu-system-x86_64 -nographic -icount
>>>>>> shift=auto,rr=record,rrfile=replay.log
>>>>>> $ ./build/qemu-system-x86_64 -nographic -icount
>>>>>> shift=auto,rr=replay,rrfile=replay.log
>>>>>>
>>>>>> [1] https://gitlab.com/qemu-project/qemu/-/commit/
>>>>>> f5ab12caba4f1656479c1feb5248beac1c833243
>>>>>>
>>>>>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2907
>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>> ---
>>>>>>     system/main.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/system/main.c b/system/main.c
>>>>>> index ecb12fd397c..1c022067349 100644
>>>>>> --- a/system/main.c
>>>>>> +++ b/system/main.c
>>>>>> @@ -25,6 +25,7 @@
>>>>>>     #include "qemu/osdep.h"
>>>>>>     #include "qemu-main.h"
>>>>>>     #include "qemu/main-loop.h"
>>>>>> +#include "system/replay.h"
>>>>>>     #include "system/system.h"
>>>>>>     #ifdef CONFIG_SDL
>>>>>> @@ -44,10 +45,12 @@ static void *qemu_default_main(void *opaque)
>>>>>>     {
>>>>>>         int status;
>>>>>> +    replay_mutex_lock();
>>>>>>         bql_lock();
>>>>>>         status = qemu_main_loop();
>>>>>>         qemu_cleanup(status);
>>>>>>         bql_unlock();
>>>>>> +    replay_mutex_unlock();
>>>>>>         exit(status);
>>>>>>     }
>>>>>> @@ -67,6 +70,7 @@ int main(int argc, char **argv)
>>>>>>     {
>>>>>>         qemu_init(argc, argv);
>>>>>>         bql_unlock();
>>>>>> +    replay_mutex_unlock();
>>>>>>         if (qemu_main) {
>>>>>>             QemuThread main_loop_thread;
>>>>>>             qemu_thread_create(&main_loop_thread, "qemu_main",
>>>>>
>>>>> Do we actually need to hold replay mutex (or even bql) over qemu_init()?
>>>>> Both should get dropped before we return here. But as a simple fix, I
>>>>> guess this is okay.
>>>>>
>>>>
>>>> For the bql, I don't know the exact reason.
>>>> For replay lock, we need to hold it as clock gets saved as soon as the
>>>> devices are initialized, which happens before end of qemu_init.
>>>
>>> Could be worth adding a comment with that information.
>>>
>>
>> In case someone is curious about it, changing default state of lock can
>> answer why it's needed, as it crashes immediately on an assert.
> 
> That all sounds reasonable enough and good info. I'm not suggesting to
> remove the lock from qemu_init() by assuming we are in init and init is
> single threaded (I agree it's good practice to keep locking consistent).
> 
> My question was more that we should move the locks tighter around
> the operations that require them. Move the unlock into qemu_init().
> 
> Commit f5ab12caba4f1 didn't introduce this problem, cocoa_main()
> already immediatey called bql_unlock() so effectively the issue is
> still there. The original design before cocoa I guess was that qemu_init
> would init things under the same critical section as qemu_main_loop() is
> then called, which is reasonable and conservative. It would have been
> good to see this bql split get a specific patch to epxlain why it's not
> needed across qemu_init and qemu_main_loop, but no big deal now.
> 

Looking more closely, bql_lock ensure vcpus don't start executing 
anything before init is completed. So we really want to hold the lock 
through all qemu_init().

Concerning replay_lock, during init, icount_configure calls 
qemu_clock_get_ns, that calls replay_save_clock, which expects to have 
the lock. Thus, we should hold the lock, at least during icount 
configuration.

> The patch is fine for a fix, could I suggest another patch that
> moves the lock narrower and perhaps adds a few words of comment?
> 

We would still need to acquire locks in qemu_default_main() anyway.

For bql, we definitely want to hold it anytime through init, so the 
scope is end of init.
For replay_lock, it could be moved around parts that expect it during 
initialization, but what would be the benefit, considering only one 
thread is running during init?

Moving locks narrower is usually made to allow more concurrency, at the 
price of increased complexity. In init phase, only one thread runs 
anyway, so there is no benefit to do anything around here.

What we could eventually do is move those unlock at the end of 
qemu_init, but IMHO, it's more readable to see the lock/unlock scheme in 
a single place, in system/main.c.
As well, I think it's better to have a single code path for lock/unlock, 
whether we use a background thread or not (vs adding a bool parameter to 
qemu_default_main() saying if we are in the same thread, or a different 
one).

> Thanks,
> Nick