Re: xquartz dereferencing a NULL pointer (patch 2)

[Jeremy Huddleston]

On Nov 5, 2008, at 07:34, George Peter Staplin wrote:

On Nov 5, 2008, at 12:33 AM, Jeremy Huddleston wrote:

The locks inside mieqProcessInputEvents() should do nothing. mieqProcessInputEvents is the only thread that modifies miEventQueue.head, and we don't care if miEventQueue.tail changes out from under us so long as miEventQueue.tail is changed AFTER the data has been pushed to it (which might actually be the problem).

Your patch seems to fix the problem too. Are we relying on certain operations being atomic?

newtail = (oldtail + 1) % QUEUE_SIZE;
miEventQueue.tail = newtail;

becoming

miEventQueue.tail++;
miEventQueue.tail |= QUEUE_SIZE - 1;







diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..ac2eedb 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -167,6 +168,8 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
     if (isMotion && isMotion == miEventQueue.lastMotion &&
         oldtail != miEventQueue.head) {
        oldtail = (oldtail - 1) % QUEUE_SIZE;
+
+       miEventQueue.tail = oldtail;
     }
     else {
        static int stuck = 0;



1) Lock inside mieqProcessInputEvents


Thoughts?

That's a good point. Let's take a look at this in a bit more detail, so hopefully I can convince you that we don't actually care about the atomicity of this opperation. mieqProcessInputEvents does a check to see if head != tail in order to determine if there are more events to process. If "tail = newtail" and "head != tail" are atomic, this should work fine. I argue that we don't care about "head != tail" itself being atomic, but we do care that the value of tail copied into register be valid. We don't care if it's "old" or "new". If it's "old", then the last event won't be processed until the next time mieqProcessInputEvents is called (not really a problem). If it's "new" then we know the event is fully in the queue and ready to be processed. So miEventQueue.tail needs to be valid. Without optimization "miEventQueue.tail = newtail" should be a single store-word or copy-word type instruction. With optimization, that instruction would possibly be ignored (for the case when (isMotion && isMotion == miEventQueue.lastMotion && oldtail != miEventQueue.head) [see below as this also has a different problem I just realized]) or would be atomic. I guess we run the risk that it is optimized as: with the intermediate value actually stored in memory for some reason... in that case, the != would still succeed and mieqProcessInputEvents would behave correctly EXCEPT if the new miEventQueue.tail was now QUEUE_SIZE in which case it would process events forever until the other thread got context back to finish the mod/store... You're starting to convince me a little that we need a lock with this argument, but it really relies on a compiler being utterly stupid... this next case, however, is more persuasive... So for the case when (isMotion && isMotion == miEventQueue.lastMotion && oldtail != miEventQueue.head), we essentially modify the last item already in the queue... but this is an event that the server thread could already be processing (I think this code might be what caused that "must be reentrant with ProcessInputEvents" comment that I dismissed earlier). This is essentially an optimization (combining a delta-x1 and delta-x2 mouse motion into a single delta-(x1 + x2) mouse motion). At first glance, we could do something like this: but that only prevents the mieqProcessInputEvents thread from processing the event if it hasn't already done the check. Since we don't have a mutex, we need to assume a context switch could happen while the server thread is already in the middle of processing this event... I see two possibilities for handling this: 2) Disable this optimization when we have multiple threads playing with mieqEventQueue and just push a second MotionNotify event.

--George

Try this:

diff --git a/mi/mieq.c b/mi/mieq.c
index e93d24f..7902608 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -113,7 +113,8 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
#ifdef XQUARTZ
   pthread_mutex_lock(&miEventQueueMutex);
#endif
-    unsigned int           oldtail = miEventQueue.tail, newtail;
+    unsigned int           oldtail = miEventQueue.tail
+    unsigned int           newtail = oldtail;
   int                    isMotion = 0;
   deviceValuator         *v = (deviceValuator *) e;

@@ -173,7 +174,6 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
#endif
           return;
       }
-       miEventQueue.tail = newtail;
   }


@@ -192,6 +192,7 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
   miEventQueue.events[oldtail].pDev = pDev;

   miEventQueue.lastMotion = isMotion;
+       miEventQueue.tail = newtail;
#ifdef XQUARTZ
   pthread_mutex_unlock(&miEventQueueMutex);
#endif


--Jeremy


On Nov 4, 2008, at 22:33, George Peter Staplin wrote:

EventPtr laste = &miEventQueue.events[(oldtail - 1) % memcpy(&(miEventQueue.events[oldtail].event[0]), e, sizeof(xEvent));

<xserver_Nov_4_mieq_fix-2.patch>


/* suspected race area*/
while (miEventQueue.head != miEventQueue.tail) {
/* suspected race area */
   ...

     miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;

--George

I have yet another patch. I think this is better. The long while loop made me miss the forest from the trees. Now we lock again at the end of the loop, and unlock on exit of the function. It still works fine in the test cases with this in place. I'm not sure it's 100% perfect. I think recursion could introduce problems, but it's interesting how it makes the fault go away. Perhaps we need a recursive mutex? memcpy(&e, &miEventQueue.events[miEventQueue.head], sizeof(EventRec));

smime.p7s
Description: S/MIME cryptographic signature

_______________________________________________
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg