[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