| Message ID | 20190214102816.3393-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ui/cocoa: Use OSX's main loop | expand |
On Thu, Feb 14, 2019 at 10:28:10AM +0000, Peter Maydell wrote: > The Cocoa UI should run on the main thread; this is enforced > in OSX Mojave. In order to be able to run on the main thread, > we need to make sure we hold the iothread lock whenever we > call into various QEMU UI midlayer functions. > Hi Peter, I wonder if qmp_stop and qmp_cont calls should be made in locked context within pauseQEMU and resumeQEMU methods, respectively? I also think it's better to clarify that the reason of the commit is not Mojave enforcing usage of event loop in main thread but an improvement of event processing in Cocoa UI, because Cocoa UI works on Mojave. As of now qemu main loop and Cocoa events processing is done in the same thread. And you can see from below that invocation of the qemu_main blocks Cocoa event loop [NSApp run] in applicationDidFinishLaunching as qemu_main doesn't quit. Here's a backtrace that shows lockup of the primary Cocoa event loop: (lldb) b cocoa_refresh Breakpoint 1: where = qemu-system-x86_64`cocoa_refresh + 12 at cocoa.m:1634, address = 0x0000000109a03a0c (lldb) c Process 46179 resuming Process 46179 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 frame #0: 0x0000000109a03a0c qemu-system-x86_64`cocoa_refresh(dcl=0x00007f914eccefe0) at cocoa.m:1634 1631 1632 static void cocoa_refresh(DisplayChangeListener *dcl) 1633 { -> 1634 NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; 1635 1636 COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n"); 1637 graphic_hw_update(NULL); Target 0: (qemu-system-x86_64) stopped. (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 * frame #0: 0x0000000109a03a0c qemu-system-x86_64`cocoa_refresh(dcl=0x00007f914eccefe0) at cocoa.m:1634 frame #1: 0x00000001099f6888 qemu-system-x86_64`dpy_refresh(s=0x00007f915074cbd0) at console.c:1622 frame #2: 0x00000001099f66dd qemu-system-x86_64`gui_update(opaque=0x00007f915074cbd0) at console.c:205 frame #3: 0x0000000109ba6009 qemu-system-x86_64`timerlist_run_timers(timer_list=0x00007f915070c390) at qemu-timer.c:574 frame #4: 0x0000000109ba60e0 qemu-system-x86_64`qemu_clock_run_timers(type=QEMU_CLOCK_REALTIME) at qemu-timer.c:588 frame #5: 0x0000000109ba662a qemu-system-x86_64`qemu_clock_run_all_timers at qemu-timer.c:708 frame #6: 0x0000000109ba6b8f qemu-system-x86_64`main_loop_wait(nonblocking=0) at main-loop.c:521 frame #7: 0x0000000109760464 qemu-system-x86_64`main_loop at vl.c:1923 frame #8: 0x000000010975b624 qemu-system-x86_64`qemu_main(argc=5, argv=0x00007ffee66a0688, envp=0x00007ffee66a06b8) at vl.c:4578 frame #9: 0x00000001099ffef9 qemu-system-x86_64`-[QemuCocoaAppController startEmulationWithArgc:argv:](self=0x00007f914ee05e10, _cmd="startEmulationWithArgc:argv:", argc=5, argv=0x00007ffee66a0688) at cocoa.m:1135 frame #10: 0x00000001099ffd97 qemu-system-x86_64`-[QemuCocoaAppController applicationDidFinishLaunching:](self=0x00007f914ee05e10, _cmd="applicationDidFinishLaunching:", note=@"NSApplicationDidFinishLaunchingNotification") at cocoa.m:1087 frame #11: 0x00007fff46bd5632 CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12 frame #12: 0x00007fff46bd55ac CoreFoundation`___CFXRegistrationPost_block_invoke + 63 frame #13: 0x00007fff46bd54cd CoreFoundation`_CFXRegistrationPost + 398 frame #14: 0x00007fff46bdd929 CoreFoundation`___CFXNotificationPost_block_invoke + 87 frame #15: 0x00007fff46b450ca CoreFoundation`-[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1633 frame #16: 0x00007fff46b4448d CoreFoundation`_CFXNotificationPost + 742 frame #17: 0x00007fff48ecca7b Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 66 frame #18: 0x00007fff440c964a AppKit`-[NSApplication _postDidFinishNotification] + 313 frame #19: 0x00007fff440c8f6e AppKit`-[NSApplication _sendFinishLaunchingNotification] + 209 frame #20: 0x00007fff440c68c8 AppKit`-[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] + 552 frame #21: 0x00007fff440c6517 AppKit`-[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] + 690 frame #22: 0x00007fff48f17144 Foundation`-[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] + 287 frame #23: 0x00007fff48f16fc0 Foundation`_NSAppleEventManagerGenericHandler + 102 frame #24: 0x00007fff47dfcb93 AE`aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) + 1855 frame #25: 0x00007fff47dfc3fd AE`dispatchEventAndSendReply(AEDesc const*, AEDesc*) + 41 frame #26: 0x00007fff47dfc2d5 AE`aeProcessAppleEvent + 439 frame #27: 0x00007fff45e1110e HIToolbox`AEProcessAppleEvent + 55 frame #28: 0x00007fff440c2644 AppKit`_DPSNextEvent + 1734 frame #29: 0x00007fff440c1102 AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1362 frame #30: 0x00007fff440bb165 AppKit`-[NSApplication run] + 699 frame #31: 0x0000000109a03041 qemu-system-x86_64`main(argc=5, argv=0x00007ffee66a0688) at cocoa.m:1589 frame #32: 0x00007fff73dc2ed9 libdyld.dylib`start + 1 frame #33: 0x00007fff73dc2ed9 libdyld.dylib`start + 1 Each millisecond cocoa_refresh gets called by display handler. The function extracts all available events from the cocoa event queue and dispatches them to handleEvent: event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast inMode: NSDefaultRunLoopMode dequeue:YES]; if (event != nil) { [cocoaView handleEvent:event]; } handleEvent enqueues keyboard and mouse events into ps2 queue and the other events are enqueued to Cocoa event queue. I'm curious what happens to the events that are enqueued with: [NSApp sendEvent:event]; Perhaps the events are enqueued in handleEvent over and over and never get out of the event queue. And there could be a chance of the event queue overrun. -- Thanks, Roman
On Fri, 22 Feb 2019 at 15:19, Roman Bolshakov <r.bolshakov@yadro.com> wrote: > > On Thu, Feb 14, 2019 at 10:28:10AM +0000, Peter Maydell wrote: > > The Cocoa UI should run on the main thread; this is enforced > > in OSX Mojave. In order to be able to run on the main thread, > > we need to make sure we hold the iothread lock whenever we > > call into various QEMU UI midlayer functions. > > > > Hi Peter, > > I wonder if qmp_stop and qmp_cont calls should be made in locked context > within pauseQEMU and resumeQEMU methods, respectively? Yes, you're right, I missed these, but they should also be called within a with_iothread_lock wrapper. > I also think it's better to clarify that the reason of the commit is not > Mojave enforcing usage of event loop in main thread but an improvement > of event processing in Cocoa UI, because Cocoa UI works on Mojave. Hmm? The point of this patchset is exactly that Mojave enforces that things go on the main thread, where previous OSX versions did not, and so in some situations QEMU will crash on Mojave where it did not on older versions. So I'm not sure what you're suggesting should be clarified here. > As of now qemu main loop and Cocoa events processing is done in the same > thread. And you can see from below that invocation of the qemu_main blocks > Cocoa event loop [NSApp run] in applicationDidFinishLaunching as qemu_main > doesn't quit. Yes, indeed. This is how we've traditionally done the Cocoa event handling. It's worked OK for everything up to Mojave... > Each millisecond cocoa_refresh gets called by display handler. The function > extracts all available events from the cocoa event queue and dispatches them to > handleEvent: > > event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast > inMode: NSDefaultRunLoopMode dequeue:YES]; > if (event != nil) { > [cocoaView handleEvent:event]; > } > > handleEvent enqueues keyboard and mouse events into ps2 queue and the > other events are enqueued to Cocoa event queue. I'm curious what happens > to the events that are enqueued with: > [NSApp sendEvent:event]; As I understand it, this sendEvent method doesn't queue anything. It just hands the event to the usual OSX event handling chain, which will either use it (for example, if it's a menu-item key accelerator event) or drop it on the floor. The logic is basically "QEMU-specific code gets first chance to decide whether to consume the event; the default (and a few other odd cases) if it does not is that we let the OSX framework code do whatever it wants to do with it". thanks -- PMM
On Fri, Feb 22, 2019 at 03:41:05PM +0000, Peter Maydell wrote: > On Fri, 22 Feb 2019 at 15:19, Roman Bolshakov <r.bolshakov@yadro.com> wrote: > > > > On Thu, Feb 14, 2019 at 10:28:10AM +0000, Peter Maydell wrote: > > > The Cocoa UI should run on the main thread; this is enforced > > > in OSX Mojave. In order to be able to run on the main thread, > > > we need to make sure we hold the iothread lock whenever we > > > call into various QEMU UI midlayer functions. > > > > > > > I also think it's better to clarify that the reason of the commit is not > > Mojave enforcing usage of event loop in main thread but an improvement > > of event processing in Cocoa UI, because Cocoa UI works on Mojave. > > Hmm? The point of this patchset is exactly that Mojave enforces > that things go on the main thread, where previous OSX versions > did not, and so in some situations QEMU will crash on Mojave > where it did not on older versions. So I'm not sure what you're > suggesting should be clarified here. > I'm not exactly sure there's an issue with QEMU on Mojave. But I lean towards the opinion because I haven't seen it :) > > As of now qemu main loop and Cocoa events processing is done in the same > > thread. And you can see from below that invocation of the qemu_main blocks > > Cocoa event loop [NSApp run] in applicationDidFinishLaunching as qemu_main > > doesn't quit. > > Yes, indeed. This is how we've traditionally done the Cocoa > event handling. It's worked OK for everything up to Mojave... > Oh, you also mention that in the cover letter. I somehow skipped it. > > Each millisecond cocoa_refresh gets called by display handler. The function > > extracts all available events from the cocoa event queue and dispatches them to > > handleEvent: > > > > event = [NSApp nextEventMatchingMask:NSEventMaskAny untilDate:distantPast > > inMode: NSDefaultRunLoopMode dequeue:YES]; > > if (event != nil) { > > [cocoaView handleEvent:event]; > > } > > > > handleEvent enqueues keyboard and mouse events into ps2 queue and the > > other events are enqueued to Cocoa event queue. I'm curious what happens > > to the events that are enqueued with: > > [NSApp sendEvent:event]; > > As I understand it, this sendEvent method doesn't queue anything. > It just hands the event to the usual OSX event handling chain, > which will either use it (for example, if it's a menu-item > key accelerator event) or drop it on the floor. The logic is > basically "QEMU-specific code gets first chance to decide whether > to consume the event; the default (and a few other odd cases) > if it does not is that we let the OSX framework code do whatever > it wants to do with it". > Thanks, I see! I should have consulted apple reference before raising the issue. FWIW there's a method to post an event to the event queue but sendEvent is used to dispatch an event within an app and the event that is already taken from the queue. Besides qmp_stop and qmp_cont not being called under iothread lock, Reviewed-by: Roman Bolshakov <r.bolshakv@yadro.com> Tested-by: Roman Bolshakov <r.bolshakov@yadro.com> Thanks, Roman
On Fri, 22 Feb 2019 at 21:48, Roman Bolshakov <r.bolshakov@yadro.com> wrote: > > On Fri, Feb 22, 2019 at 03:41:05PM +0000, Peter Maydell wrote: > > On Fri, 22 Feb 2019 at 15:19, Roman Bolshakov <r.bolshakov@yadro.com> wrote: > > > > > > On Thu, Feb 14, 2019 at 10:28:10AM +0000, Peter Maydell wrote: > > > > The Cocoa UI should run on the main thread; this is enforced > > > > in OSX Mojave. In order to be able to run on the main thread, > > > > we need to make sure we hold the iothread lock whenever we > > > > call into various QEMU UI midlayer functions. > > > > > > > > > > I also think it's better to clarify that the reason of the commit is not > > > Mojave enforcing usage of event loop in main thread but an improvement > > > of event processing in Cocoa UI, because Cocoa UI works on Mojave. > > > > Hmm? The point of this patchset is exactly that Mojave enforces > > that things go on the main thread, where previous OSX versions > > did not, and so in some situations QEMU will crash on Mojave > > where it did not on older versions. So I'm not sure what you're > > suggesting should be clarified here. > > > > I'm not exactly sure there's an issue with QEMU on Mojave. But I lean > towards the opinion because I haven't seen it :) It only happens for some guest workloads. The "usual" case is that the cocoa_refresh callback is called from the QEMU main loop, which happens to be on the OSX main thread, which means OSX is still happy. But in some cases cocoa_refresh can be called from a guest vCPU thread -- I think we've seen this when a guest initiates a screen resolution change: the call from the guest vCPU thread goes into the model of the graphics device, which makes a call into the UI midlayer to say "resolution changed", which immediately triggers a refresh callback to the UI frontend layer from that thread. In Mojave this causes OSX to terminate QEMU. I think in older OSX versions it would probably be a race condition, so it's technically a bug but not one that usually has any visible bad effects; it's only surfaced as a problem now that Mojave actively checks for this condition and kills the process. thanks -- PMM
diff --git a/ui/cocoa.m b/ui/cocoa.m index e2567d6946..2931c751fd 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -129,6 +129,21 @@ NSTextField *pauseLabel; NSArray * supportedImageFileTypes; +// Utility function to run specified code block with iothread lock held +typedef void (^CodeBlock)(void); + +static void with_iothread_lock(CodeBlock block) +{ + bool locked = qemu_mutex_iothread_locked(); + if (!locked) { + qemu_mutex_lock_iothread(); + } + block(); + if (!locked) { + qemu_mutex_unlock_iothread(); + } +} + // Mac to QKeyCode conversion const int mac_to_qkeycode_map[] = { [kVK_ANSI_A] = Q_KEY_CODE_A, @@ -306,6 +321,7 @@ - (void) ungrabMouse; - (void) toggleFullScreen:(id)sender; - (void) handleMonitorInput:(NSEvent *)event; - (void) handleEvent:(NSEvent *)event; +- (void) handleEventLocked:(NSEvent *)event; - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled; /* The state surrounding mouse grabbing is potentially confusing. * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated @@ -649,8 +665,14 @@ - (void) handleMonitorInput:(NSEvent *)event - (void) handleEvent:(NSEvent *)event { - COCOA_DEBUG("QemuCocoaView: handleEvent\n"); + with_iothread_lock(^{ + [self handleEventLocked:event]; + }); +} +- (void) handleEventLocked:(NSEvent *)event +{ + COCOA_DEBUG("QemuCocoaView: handleEvent\n"); int buttons = 0; int keycode = 0; bool mouse_event = false; @@ -945,15 +967,18 @@ - (QEMUScreen) gscreen {return screen;} */ - (void) raiseAllKeys { - int index; const int max_index = ARRAY_SIZE(modifiers_state); - for (index = 0; index < max_index; index++) { - if (modifiers_state[index]) { - modifiers_state[index] = 0; - qemu_input_event_send_key_qcode(dcl->con, index, false); - } - } + with_iothread_lock(^{ + int index; + + for (index = 0; index < max_index; index++) { + if (modifiers_state[index]) { + modifiers_state[index] = 0; + qemu_input_event_send_key_qcode(dcl->con, index, false); + } + } + }); } @end @@ -1215,13 +1240,17 @@ - (void)removePause /* Restarts QEMU */ - (void)restartQEMU:(id)sender { - qmp_system_reset(NULL); + with_iothread_lock(^{ + qmp_system_reset(NULL); + }); } /* Powers down QEMU */ - (void)powerDownQEMU:(id)sender { - qmp_system_powerdown(NULL); + with_iothread_lock(^{ + qmp_system_powerdown(NULL); + }); } /* Ejects the media. @@ -1237,9 +1266,11 @@ - (void)ejectDeviceMedia:(id)sender return; } - Error *err = NULL; - qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding], - false, NULL, false, false, &err); + __block Error *err = NULL; + with_iothread_lock(^{ + qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding], + false, NULL, false, false, &err); + }); handleAnyDeviceErrors(err); } @@ -1271,16 +1302,18 @@ - (void)changeDeviceMedia:(id)sender return; } - Error *err = NULL; - qmp_blockdev_change_medium(true, - [drive cStringUsingEncoding: - NSASCIIStringEncoding], - false, NULL, - [file cStringUsingEncoding: - NSASCIIStringEncoding], - true, "raw", - false, 0, - &err); + __block Error *err = NULL; + with_iothread_lock(^{ + qmp_blockdev_change_medium(true, + [drive cStringUsingEncoding: + NSASCIIStringEncoding], + false, NULL, + [file cStringUsingEncoding: + NSASCIIStringEncoding], + true, "raw", + false, 0, + &err); + }); handleAnyDeviceErrors(err); } } @@ -1419,7 +1452,9 @@ - (void)adjustSpeed:(id)sender // get the throttle percentage throttle_pct = [sender tag]; - cpu_throttle_set(throttle_pct); + with_iothread_lock(^{ + cpu_throttle_set(throttle_pct); + }); COCOA_DEBUG("cpu throttling at %d%c\n", cpu_throttle_get_percentage(), '%'); }
The Cocoa UI should run on the main thread; this is enforced in OSX Mojave. In order to be able to run on the main thread, we need to make sure we hold the iothread lock whenever we call into various QEMU UI midlayer functions. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- ui/cocoa.m | 83 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 24 deletions(-) -- 2.17.2 (Apple Git-113)