Re: [RFC uncompiled PATCH] cocoa: run qemu_init in the main thread

[Akihiko Odaki]

On Tue, Mar 8, 2022 at 2:21 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/7/22 17:41, Akihiko Odaki wrote:
> >> That series doesn't remove the general design that has quite a bit
> >> of "we know some other thread holds the lock and waits for us" code.
> > Well, I don't think so. main no longer calls back QEMU code (and it
> > should never do so in my opinion).
>
> That's an arbitrary limitation.  With my patch, until [NSApp run] only
> the main thread has the lock and therefore it can do anything.  In my
> patch I decided to minimize the changes, but if
> register_displaychangelistener() and qemu_clipboard_peer_register() can
> be moved earlier, then the iothread could even be unlocked before [NSApp
> run].

I meant the thread without iothread lock shouldn't call QEMU functions
even if it is clear that another thread is holding the lock and
waiting. The rule should apply after unlocking the mutex in
[-QemuCocoaAppController applicationDidFinishLaunching:].

>
> There's also the advantage that the flow is a lot more similar between
> "-display cocoa" and "-display none", and also between --enable-cocoa
> and --disable-cocoa builds.
>
> Then main() remains as
>
>      /* doesn't hurt in the !cocoa case */
>      NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
>
>      qemu_init(argc, argv);
>      if (!have_cocoa_display) {
>          qemu_main_loop();
>          qemu_cleanup();
>      } else {
>          QemuCocoaAppController *appController =
> [[QemuCocoaAppController alloc] init];
>          [NSApp setDelegate:appController];
>          [NSApp run];
>          [appController release];
>      }
>      [pool release];
>      return 0;
>
> which is in my opinion the best of both worlds.  But my patch (assuming
> it works) really fixes the threading model.  Without it, moving menus to
> the iothread makes it even more complicated.

I don't think the threading model is broken in the current code. There
is always the iothread and main thread and ui/cocoa has to be aware of
that. We could unify them when initializing but it is exceptional. The
broken part is the menu creation code which touches iothread.

The nice part of this change is that it gives control on how to model
threading. It can provide a conventional model when running without
cocoa, and leave a room for changes like moving all of the
initialization code before [+NSApp run].

>
> >> It also gives us the opposite problem that we're now calling a lot
> >> of Cocoa APIs from something other than the main Cocoa thread.
> > Basically NSView is the only thing prohibited from calling from
> > another thread so it shouldn't be a problem.
>
> I'd rather have a single thread that does all things related to Cocoa.
> If my patch doesn't work, I would just call qmp_query_block(NULL) from
> cocoa_display_init() and save the result in a global variable.

The distinction between Cocoa and other platform APIs is arbitrary.
The APIs of Cocoa are mostly thread-independent and NSView is rather
exceptional.

Regards,
Akihiko Odaki

>
> Paolo