[PATCH 2/4] X event queue mutex
Jeremy Huddleston
jeremyhu at freedesktop.org
Thu Oct 23 14:47:30 PDT 2008
And here's a stab at setting up mieqProcessInputEvents in master to be
more friendly towards this locking. master doesn't work for us on
OSX, so I can't really verify that it works... I may have missed an e-
> to e. or e->events[i]->event to event[i] somewhere...
This also fixes what I think is a bug in the custom event handler...
right now we have:
handler(DequeueScreen(e->pDev)->myNum, e->events->event,
e->pDev, e->nevents);
if (!e->pDev->isMaster && e->pDev->u.master) {
handler(DequeueScreen(e->pDev->u.master)->myNum,
e->events->event, e->pDev->u.master, e->nevents);
}
But that fails if e->nevents > 1 ... granted all current custom event
handlers have nevents=1, but it's worth noting.
Also, why do we call the handler twice here?
--Jeremy
diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..3e1d2f8 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -305,8 +305,9 @@ void
mieqProcessInputEvents(void)
{
mieqHandler handler;
- EventRec *e = NULL;
+ EventRec e;
int x = 0, y = 0;
+ int i;
xEvent* event,
*master_event = NULL;
@@ -321,43 +322,44 @@ mieqProcessInputEvents(void)
DPMSSet(serverClient, DPMSModeOn);
#endif
- e = &miEventQueue.events[miEventQueue.head];
+ memcpy(&e, &miEventQueue.events[miEventQueue.head],
sizeof(EventRec));
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+ event = xcalloc(e.nevents, sizeof(xEvent));
+
+ /* FIXME: Bad hack. The only event where we actually get
multiple
+ * events at once is a DeviceMotionNotify followed by
+ * DeviceValuators. For now it's safe enough to just take the
+ * event directly or copy the bunch of events and pass in the
+ * copy. Eventually the interface for the processInputProc
needs
+ * to be changed. (whot)
+ */
+
+ if (!event)
+ FatalError("[mi] No memory left for event processing.\n");
+ for (i = 0; i < e.nevents; i++) {
+ memcpy(&event[i], e.events[i].event, sizeof(xEvent));
+ }
+
+ /* Just using event array, null this out now since we don't
want to
+ * rely on it (another thread can change it during an enqueue
and we
+ * just copied all we care about)
+ */
+ e.events = NULL;
/* Custom event handler */
- handler = miEventQueue.handlers[e->events->event->u.u.type];
+ handler = miEventQueue.handlers[event[0].u.u.type];
- if (e->pScreen != DequeueScreen(e->pDev) && !handler) {
+ if (e.pScreen != DequeueScreen(e.pDev) && !handler) {
/* Assumption - screen switching can only occur on
motion events. */
- DequeueScreen(e->pDev) = e->pScreen;
- x = e->events[0].event->u.keyButtonPointer.rootX;
- y = e->events[0].event->u.keyButtonPointer.rootY;
- NewCurrentScreen (e->pDev, DequeueScreen(e->pDev), x, y);
- }
- else {
- /* FIXME: Bad hack. The only event where we actually get
multiple
- * events at once is a DeviceMotionNotify followed by
- * DeviceValuators. For now it's safe enough to just take
the
- * event directly or copy the bunch of events and pass in
the
- * copy. Eventually the interface for the
processInputProc needs
- * to be changed. (whot)
- */
- if (e->nevents > 1)
- {
- int i;
- event = xcalloc(e->nevents, sizeof(xEvent));
- if (!event)
- FatalError("[mi] No memory left for event
processing.\n");
- for (i = 0; i < e->nevents; i++)
- {
- memcpy(&event[i], e->events[i].event,
sizeof(xEvent));
- }
- } else
- event = e->events->event;
- if (!e->pDev->isMaster && e->pDev->u.master)
+ DequeueScreen(e.pDev) = e.pScreen;
+ x = event[0].u.keyButtonPointer.rootX;
+ y = event[0].u.keyButtonPointer.rootY;
+ NewCurrentScreen (e.pDev, DequeueScreen(e.pDev), x, y);
+ } else {
+ if (!e.pDev->isMaster && e.pDev->u.master)
{
- CopyGetMasterEvent(e->pDev->u.master, event,
- &master_event, e->nevents);
+ CopyGetMasterEvent(e.pDev->u.master, event,
+ &master_event, e.nevents);
} else
master_event = NULL;
@@ -365,35 +367,33 @@ mieqProcessInputEvents(void)
* steal it. */
if (handler)
{
- handler(DequeueScreen(e->pDev)->myNum, e->events-
>event,
- e->pDev, e->nevents);
- if (!e->pDev->isMaster && e->pDev->u.master)
+ handler(DequeueScreen(e.pDev)->myNum, event,
+ e.pDev, e.nevents);
+ if (!e.pDev->isMaster && e.pDev->u.master)
{
- handler(DequeueScreen(e->pDev->u.master)->myNum,
- e->events->event, e->pDev->u.master, e-
>nevents);
+ handler(DequeueScreen(e.pDev->u.master)->myNum,
+ event, e.pDev->u.master, e.nevents);
}
} else
{
/* process slave first, then master */
- e->pDev->public.processInputProc(event, e->pDev, e-
>nevents);
+ e.pDev->public.processInputProc(event, e.pDev,
e.nevents);
- if (!e->pDev->isMaster && e->pDev->u.master)
+ if (!e.pDev->isMaster && e.pDev->u.master)
{
- e->pDev->u.master-
>public.processInputProc(master_event,
- e->pDev->u.master, e->nevents);
+ e.pDev->u.master-
>public.processInputProc(master_event,
+ e.pDev->u.master, e.nevents);
}
}
- if (e->nevents > 1)
- xfree(event);
+ xfree(event);
xfree(master_event);
}
/* Update the sprite now. Next event may be from different
device. */
- if (e->events->event[0].u.u.type == DeviceMotionNotify
- && e->pDev->coreEvents)
+ if (event[0].u.u.type == DeviceMotionNotify && e.pDev-
>coreEvents)
{
- miPointerUpdateSprite(e->pDev);
+ miPointerUpdateSprite(e.pDev);
}
}
}
On Oct 23, 2008, at 14:08, Jeremy Huddleston wrote:
> Whoops... I forgot the diff to show you what I meant... here's what
> I'm going to push to 1.4-apple to address our problem.
>
> --Jeremy
>
> diff --git a/mi/mieq.c b/mi/mieq.c
> index 3f50f27..543b7e7 100644
> --- a/mi/mieq.c
> +++ b/mi/mieq.c
> @@ -231,7 +231,7 @@ mieqSetHandler(int event, mieqHandler handler)
> void
> mieqProcessInputEvents(void)
> {
> - EventRec *e = NULL;
> + EventRec e;
> int x = 0, y = 0;
> DeviceIntPtr dev = NULL;
>
> @@ -250,45 +250,46 @@ mieqProcessInputEvents(void)
> DPMSSet(DPMSModeOn);
> #endif
>
> - e = &miEventQueue.events[miEventQueue.head];
> + memcpy(&e, &miEventQueue.events[miEventQueue.head],
> sizeof(EventRec));
> miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
>
> - if (miEventQueue.handlers[e->event->u.u.type]) {
> +#ifdef XQUARTZ
> + pthread_mutex_unlock(&miEventQueueMutex);
> +#endif
> +
> + if (miEventQueue.handlers[e.event->u.u.type]) {
> /* If someone's registered a custom event handler, let
> them
> * steal it. */
> - miEventQueue.handlers[e->event->u.u.type]
> (miEventQueue.pDequeueScreen->myNum,
> - e->event, dev,
> - e->nevents);
> + miEventQueue.handlers[e.event->u.u.type](e.pScreen-
> >myNum,
> + e.event,
> e.pDev,
> + e.nevents);
> }
> - else if (e->pScreen != miEventQueue.pDequeueScreen) {
> + else if (e.pScreen != miEventQueue.pDequeueScreen) {
> /* Assumption - screen switching can only occur on
> motion events. */
> - miEventQueue.pDequeueScreen = e->pScreen;
> - x = e->event[0].u.keyButtonPointer.rootX;
> - y = e->event[0].u.keyButtonPointer.rootY;
> - NewCurrentScreen (miEventQueue.pDequeueScreen, x, y);
> + miEventQueue.pDequeueScreen = e.pScreen;
> + x = e.event[0].u.keyButtonPointer.rootX;
> + y = e.event[0].u.keyButtonPointer.rootY;
> + NewCurrentScreen(e.pScreen, x, y);
> }
> else {
> /* If this is a core event, make sure our keymap, et al,
> is
> * changed to suit. */
> - if (e->event[0].u.u.type == KeyPress ||
> - e->event[0].u.u.type == KeyRelease) {
> - SwitchCoreKeyboard(e->pDev);
> + if (e.event[0].u.u.type == KeyPress ||
> + e.event[0].u.u.type == KeyRelease) {
> + SwitchCoreKeyboard(e.pDev);
> dev = inputInfo.keyboard;
> }
> - else if (e->event[0].u.u.type == MotionNotify ||
> - e->event[0].u.u.type == ButtonPress ||
> - e->event[0].u.u.type == ButtonRelease) {
> - SwitchCorePointer(e->pDev);
> + else if (e.event[0].u.u.type == MotionNotify ||
> + e.event[0].u.u.type == ButtonPress ||
> + e.event[0].u.u.type == ButtonRelease) {
> + SwitchCorePointer(e.pDev);
> dev = inputInfo.pointer;
> }
> else {
> - dev = e->pDev;
> + dev = e.pDev;
> }
>
> - dev->public.processInputProc(e->event, dev, e->nevents);
> + dev->public.processInputProc(e.event, dev, e.nevents);
> }
> }
> -#ifdef XQUARTZ
> - pthread_mutex_unlock(&miEventQueueMutex);
> -#endif
> }
>
>
>
> On Oct 23, 2008, at 13:59, Jeremy Huddleston wrote:
>
>> Hey Tiago,
>>
>> I hope things are going well for you. I've recently hit an issue
>> using locks in miEq. We're doing it the same way in mieq.c as your
>> proposal (patch 2/4) and this causes us to hit a deadlock when the
>> enqueueing thread is waiting for the lock to push an event and the
>> dequeuing thread (the server thread) is processing an event that
>> requires a message to be sent to the enqueueing thread.
>>
>> I am going to try solving this by making the lock a bit more
>> conservative in mieqProcessInputEvents. Our current implementation
>> (we're still on 1.4) passes the event to the handler as a pointer
>> to the event within the queue. In 1.5, mieqProcessInputEvents now
>> copies the nevents first... so we might be able to release that a
>> bit sooner...
>>
>> The thing is that we have:
>>
>> e = &miEventQueue.events[miEventQueue.head];
>>
>> Then e->events is copied, but 'e' itself is still directly
>> referenced throughout mieqProcessInputEvents. Could we actually
>> just copy all of miEventQueue.events[miEventQueue.head] to a local
>> copy and release the lock after that? I see us using:
>>
>> e->pDev
>> e->nevents
>> e->events (already copied in 1.5.x and later if nevents > 1)
>>
>> --Jeremy
>>
>> Begin forwarded message:
>>
>>> From: Jeremy Huddleston <jeremyhu at apple.com>
>>> Date: October 19, 2008 12:59:16 PDT
>>> To: Kevin Van Vechten <kvv at apple.com>, Jordan Hubbard
>>> <jkh at apple.com>
>>> Cc: George Peter Staplin <gstaplin at apple.com>
>>> Subject: mach_msg async? thread communication question
>>>
>>> So... I've made some progress with fullscreen...
>>>
>>> It now renders the "weaved" background and only crashes if there
>>> are windows other than the root window... but if you just want to
>>> stare at a black and white X11 weave background, we're there!
>>>
>>> Aside from this crash, I noticed a thread communication deadlock
>>> issue (see the stack traces below):
>>>
>>> Thread 2's mieqProcessInputEvents and mieqEnqueue lock the input
>>> event queue... but some of the handlers for mieqProcessInputEvents
>>> actually need to msg the Appkit thread which appears to cause the
>>> deadlock.
>>>
>>> Is there a way to allow mach_msg to be async? In these cases, we
>>> don't care about return values.
>>>
>>> Or do I need to do something clever (read: annoying) to address
>>> this issue?
>>>
>>> Call graph:
>>> 4783 Thread_2503
>>> 4783 start
>>> 4783 main
>>> 4783 mach_msg_server
>>> 4783 mach_startup_server
>>> 4783 _Xstart_x11_server
>>> 4783 do_start_x11_server
>>> 4783 server_main
>>> 4783 X11ControllerMain
>>> 4783 X11ApplicationMain
>>> 4783 -[NSApplication run]
>>> 4783 -[X11Application sendEvent:]
>>> 4783 -[NSApplication sendEvent:]
>>> 4783 -[NSApplication
>>> _handleKeyEquivalent:]
>>> 4783 -[NSMenu performKeyEquivalent:]
>>> 4783 -[NSCarbonMenuImpl
>>> performActionWithHighlightingForItemAtIndex:]
>>> 4783 -[NSMenu
>>> performActionForItemAtIndex:]
>>> 4783 -[NSApplication
>>> sendAction:to:from:]
>>> 4783 -[X11Controller
>>> toggle_fullscreen:]
>>> 4783 DarwinSendDDXEvent
>>> 4783 mieqEnqueue
>>> 4768 pthread_mutex_lock
>>> 4768
>>> semaphore_wait_trap
>>> 4768
>>> semaphore_wait_trap
>>> 15 0xffffffff
>>> 15 _sigtramp
>>> 15 _sigtramp
>>> 4783 Thread_2703
>>> 4783 thread_start
>>> 4783 _pthread_start
>>> 4783 server_thread
>>> 4783 dix_main
>>> 4783 Dispatch
>>> 4783 ProcessInputEvents
>>> 4783 mieqProcessInputEvents
>>> 4783 DarwinEventHandler
>>> 4783 QuartzHide
>>> 4783 QuartzSetFullscreen
>>> 4783 X11ApplicationShowHideMenubar
>>> 4783 message_kit_thread
>>> 4783 mach_msg
>>> 4783 mach_msg_trap
>>> 4783 mach_msg_trap
>>>
>>
>>
>
> _______________________________________________
> xorg mailing list
> xorg at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xorg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3040 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081023/f30ba80a/attachment.bin>
More information about the xorg
mailing list