Re: [PATCH 2/4] X event queue mutex
Hi Jeremy, Jeremy Huddleston escreveu: Well the problem was that mieqProcessInputEvents DOESN'T use those locks, so it can read the incremented value of miEventQueue.tail thinking that the item at head=tail-1 is ready to read when in fact mieqEnqueue is still in the process of writing to it. Further, mieqEnqueue sometimes edits miEventQueue.tail-1 (when the current and tail-1 events are MotionNotify)... but mieqProcessInputEvents is already reading it. yeah. Now your last patch makes more sense to me (but not that first hunk). For this reason, I went back to locking inside mieqProcessInputEvents with XQUARTZ. Your mutex will lags your cursor update on screen because the input thread will block before enqueuing while the main thread pops events. On this case try to keep the lock near the critical region on mieqPIE, avoiding coarse grained locking. Cheers, PS: this problem of the event queue being processed by more then one user shows how multi-threaded applications suck so much to program. Moreover, if you try to use gdb to debug then you'll see the crazy world that we're living. -- Tiago Vignatti C3SL - Centro de Computação Científica e Software Livre www.c3sl.ufpr.br ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Nov 7, 2008, at 12:23, Tiago Vignatti wrote: For this reason, I went back to locking inside mieqProcessInputEvents with XQUARTZ. Your mutex will lags your cursor update on screen because the input thread will block before enqueuing while the main thread pops events. On this case try to keep the lock near the critical region on mieqPIE, avoiding coarse grained locking. Cheers, PS: this problem of the event queue being processed by more then one user shows how multi-threaded applications suck so much to program. Moreover, if you try to use gdb to debug then you'll see the crazy world that we're living. Yeah, that's pretty much what it's doing now in the xorg-server-1.4- apple branch... the one thing I don't like is that screensaver block... can that be moved to after we copy the event (to lessen the lock time) or is there some reason that I don't see that it's first? --Jeremy smime.p7s Description: S/MIME cryptographic signature ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
Peter Hutterer escreveu: From: Peter Hutterer [EMAIL PROTECTED] Date: Tue, 4 Nov 2008 15:27:30 +1030 Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all events before processing. Copy the EventRec's information into local variables before processing them, this should make it safer for upcoming threading and also makes it easier to read. Simplify the event allocation code from the abyss it was before. This also fixes a potential bug where a custom handler could scramble the event before the same -now scrambled- event was then passed through the master's custom event handler. Signed-off-by: Peter Hutterer [EMAIL PROTECTED] Signed-off-by Tiago Vignatti [EMAIL PROTECTED] It's fine for me. Applied and tested here. Thanks, -- Tiago Vignatti C3SL - Centro de Computação Científica e Software Livre www.c3sl.ufpr.br ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
Hey,
Jeremy Huddleston escreveu:
Looks good! A recent bug report has surfaced for us since I got rid of
the locking in mieqProcessInputEvents. We need to update
miEventQueue.tail only after the data has actually been pushed into the
tail. This should take care of that problem on master, but I haven't
tested it:
Well, it won't be a problem with Xorg input thread because mieqEnqueue
is tread-safe. There's a mutex protecting writes to the tail pointer in
that function and xquartz will need it as well.
diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..5016d73 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -122,7 +122,8 @@ mieqResizeEvents(int min_size)
void
mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
{
-unsigned int oldtail = miEventQueue.tail, newtail;
+unsigned int oldtail = miEventQueue.tail;
+unsigned int newtail = oldtail;
EventListPtr evt;
intisMotion = 0;
intevlen;
@@ -184,7 +185,6 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
return;
}
stuck = 0;
-miEventQueue.tail = newtail;
}
evlen = sizeof(xEvent);
@@ -218,6 +218,7 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
miEventQueue.events[oldtail].pDev = pDev;
miEventQueue.lastMotion = isMotion;
+miEventQueue.tail = newtail;
}
Anyway this patch is not good enough. newtail is also deferenced (line
173) before his first usage, making the first hunk not utilized.
Thanks,
--
Tiago Vignatti
C3SL - Centro de Computação Científica e Software Livre
www.c3sl.ufpr.br
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Nov 3, 2008, at 22:08, Peter Hutterer wrote: ... righty-o. hunk 2 is different, with the change you suggested. To make the flow cleaner, I moved all the EQ and memory alloc stuff together and the copies into local variables below it. Almost... you need to move the pop down 4 lines. ie this: bad miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE; +dev = e-pDev; +master = (!dev-isMaster dev-u.master) ? dev-u.master : NULL; +type= e-events-event-u.u.type; +screen = e-pScreen; + /bad should be this: good +dev = e-pDev; +master = (!dev-isMaster dev-u.master) ? dev-u.master : NULL; +type= e-events-event-u.u.type; +screen = e-pScreen; + miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE; /good After you increment miEventQueue.head, you can't read from e. ... so you could also do this if you wanted to be anal about popping quickly: good +dev = e-pDev; +screen = e-pScreen; + miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE; +type= event-u.u.type; +master = (!dev-isMaster dev-u.master) ? dev-u.master : NULL; /good --Jeremy smime.p7s Description: S/MIME cryptographic signature ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Tue, Nov 04, 2008 at 09:34:23AM -0800, Jeremy Huddleston wrote:
good
+dev = e-pDev;
+screen = e-pScreen;
+
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+type= event-u.u.type;
+master = (!dev-isMaster dev-u.master) ? dev-u.master :
NULL;
From 7693b94cd08030b9668130410104f432162f2859 Mon Sep 17 00:00:00 2001
From: Peter Hutterer [EMAIL PROTECTED]
Date: Tue, 4 Nov 2008 15:27:30 +1030
Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all events before
processing.
Copy the EventRec's information into local variables before processing them,
this should make it safer for upcoming threading and also makes it easier to
read.
Simplify the event allocation code from the abyss it was before.
This also fixes a potential bug where a custom handler could scramble the
event before the same -now scrambled- event was then passed through the
master's custom event handler.
Signed-off-by: Peter Hutterer [EMAIL PROTECTED]
---
mi/mieq.c | 98
1 files changed, 46 insertions(+), 52 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 986e3a1..52bb841 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -307,8 +307,12 @@ mieqProcessInputEvents(void)
mieqHandler handler;
EventRec *e = NULL;
int x = 0, y = 0;
-xEvent* event,
-*master_event = NULL;
+int type, nevents, evlen, i;
+ScreenPtr screen;
+xEvent *event,
+ *master_event = NULL;
+DeviceIntPtr dev = NULL,
+ master = NULL;
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
@@ -322,79 +326,69 @@ mieqProcessInputEvents(void)
#endif
e = miEventQueue.events[miEventQueue.head];
+
+/* GenericEvents always have nevents == 1 */
+nevents = e-nevents;
+evlen = (nevents 1) ? sizeof(xEvent) : e-events-evlen;
+event = xcalloc(nevents, evlen);
+
+if (!event)
+FatalError([mi] No memory left for event processing.\n);
+
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
+
+
+dev = e-pDev;
+screen = e-pScreen;
+
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+type= event-u.u.type;
+master = (!dev-isMaster dev-u.master) ? dev-u.master : NULL;
+
/* Custom event handler */
-handler = miEventQueue.handlers[e-events-event-u.u.type];
+handler = miEventQueue.handlers[type];
-if (e-pScreen != DequeueScreen(e-pDev) !handler) {
+if (screen != DequeueScreen(dev) !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);
+DequeueScreen(dev) = screen;
+x = event-u.keyButtonPointer.rootX;
+y = event-u.keyButtonPointer.rootY;
+NewCurrentScreen (dev, DequeueScreen(dev), 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)
-{
-CopyGetMasterEvent(e-pDev-u.master, event,
- master_event, e-nevents);
-} else
+if (master)
+CopyGetMasterEvent(master, event,
+ master_event, nevents);
+else
master_event = NULL;
/* If someone's registered a custom event handler, let them
* 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-u.master)-myNum,
-e-events-event,
Re: [PATCH 2/4] X event queue mutex
Looks good! A recent bug report has surfaced for us since I got rid
of the locking in mieqProcessInputEvents. We need to update
miEventQueue.tail only after the data has actually been pushed into
the tail. This should take care of that problem on master, but I
haven't tested it:
diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..5016d73 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -122,7 +122,8 @@ mieqResizeEvents(int min_size)
void
mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
{
-unsigned int oldtail = miEventQueue.tail, newtail;
+unsigned int oldtail = miEventQueue.tail;
+unsigned int newtail = oldtail;
EventListPtr evt;
intisMotion = 0;
intevlen;
@@ -184,7 +185,6 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
return;
}
stuck = 0;
- miEventQueue.tail = newtail;
}
evlen = sizeof(xEvent);
@@ -218,6 +218,7 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
miEventQueue.events[oldtail].pDev = pDev;
miEventQueue.lastMotion = isMotion;
+miEventQueue.tail = newtail;
}
void
On Nov 4, 2008, at 14:18, Peter Hutterer wrote:
On Tue, Nov 04, 2008 at 09:34:23AM -0800, Jeremy Huddleston wrote:
good
+dev = e-pDev;
+screen = e-pScreen;
+
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+type= event-u.u.type;
+master = (!dev-isMaster dev-u.master) ? dev-
u.master :
NULL;
From 7693b94cd08030b9668130410104f432162f2859 Mon Sep 17 00:00:00 2001
From: Peter Hutterer [EMAIL PROTECTED]
Date: Tue, 4 Nov 2008 15:27:30 +1030
Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all
events before processing.
Copy the EventRec's information into local variables before
processing them,
this should make it safer for upcoming threading and also makes it
easier to
read.
Simplify the event allocation code from the abyss it was before.
This also fixes a potential bug where a custom handler could
scramble the
event before the same -now scrambled- event was then passed through
the
master's custom event handler.
Signed-off-by: Peter Hutterer [EMAIL PROTECTED]
---
mi/mieq.c | 98 +++
+
1 files changed, 46 insertions(+), 52 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 986e3a1..52bb841 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -307,8 +307,12 @@ mieqProcessInputEvents(void)
mieqHandler handler;
EventRec *e = NULL;
int x = 0, y = 0;
-xEvent* event,
-*master_event = NULL;
+int type, nevents, evlen, i;
+ScreenPtr screen;
+xEvent *event,
+ *master_event = NULL;
+DeviceIntPtr dev = NULL,
+ master = NULL;
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
@@ -322,79 +326,69 @@ mieqProcessInputEvents(void)
#endif
e = miEventQueue.events[miEventQueue.head];
+
+/* GenericEvents always have nevents == 1 */
+nevents = e-nevents;
+evlen = (nevents 1) ? sizeof(xEvent) : e-events-evlen;
+event = xcalloc(nevents, evlen);
+
+if (!event)
+FatalError([mi] No memory left for event processing.
\n);
+
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
+
+
+dev = e-pDev;
+screen = e-pScreen;
+
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+type= event-u.u.type;
+master = (!dev-isMaster dev-u.master) ? dev-
u.master : NULL;
+
/* Custom event handler */
-handler = miEventQueue.handlers[e-events-event-u.u.type];
+handler = miEventQueue.handlers[type];
-if (e-pScreen != DequeueScreen(e-pDev) !handler) {
+if (screen != DequeueScreen(dev) !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);
+DequeueScreen(dev) = screen;
+x = event-u.keyButtonPointer.rootX;
+y = event-u.keyButtonPointer.rootY;
+NewCurrentScreen (dev, DequeueScreen(dev), 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 =
Re: [PATCH 2/4] X event queue mutex
Sorry for the late reply, I was tied up and missed the mail.
On Thu, Oct 23, 2008 at 02:47:30PM -0700, Jeremy Huddleston wrote:
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.
with the move of the handler procesing (dac9e918) this should now be.
handler(.., event, ...);
if (!e-pDev-isMaster ...)
handler(..., master_event, ...)
otherwise we're not protected against event manipulation in the handler.
Also, why do we call the handler twice here?
Only one event is in the queue, but we need to process it twice, once as
coming from the slave device, once as coming from the master device. These two
are independent.
-e = miEventQueue.events[miEventQueue.head];
+memcpy(e, miEventQueue.events[miEventQueue.head], sizeof(EventRec));
if we're not using the EventRec as such anyway, it might just be better to
extract the things we need and improve readability.
+event = xcalloc(e.nevents, sizeof(xEvent));
that doesn't work. GenericEvents may be larger than xEvent, hence the slightly
weird copying code. What you are doing here corrupts memory after the first
device switch. How about this one here, quick test shows it works:
From b783944ac784c42ba84b3dfe77d60107b9581a30 Mon Sep 17 00:00:00 2001
From: Peter Hutterer [EMAIL PROTECTED]
Date: Tue, 4 Nov 2008 15:27:30 +1030
Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all events before
processing.
Copy the EventRec's information into local variables before processing them,
this should make it safer for upcoming threading and also makes it easier to
read.
Simplify the event allocation code from the abyss it was before.
This also fixes a potential bug where a custom handler could scramble the
event before the same -now scrambled- event was then passed through the
master's custom event handler.
---
mi/mieq.c | 95 +++-
1 files changed, 43 insertions(+), 52 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 986e3a1..ec11e00 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -307,8 +307,12 @@ mieqProcessInputEvents(void)
mieqHandler handler;
EventRec *e = NULL;
int x = 0, y = 0;
-xEvent* event,
-*master_event = NULL;
+int type, nevents, evlen, i;
+ScreenPtr screen;
+xEvent *event,
+ *master_event = NULL;
+DeviceIntPtr dev = NULL,
+ master = NULL;
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
@@ -324,77 +328,64 @@ mieqProcessInputEvents(void)
e = miEventQueue.events[miEventQueue.head];
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+dev = e-pDev;
+master = (!dev-isMaster dev-u.master) ? dev-u.master : NULL;
+type= e-events-event-u.u.type;
+nevents = e-nevents;
+screen = e-pScreen;
+
+/* GenericEvents always have nevents == 1 */
+evlen = (nevents 1) ? sizeof(xEvent) : e-events-evlen;
+event = xcalloc(nevents, evlen);
+
+if (!event)
+FatalError([mi] No memory left for event processing.\n);
+
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
+
/* Custom event handler */
-handler = miEventQueue.handlers[e-events-event-u.u.type];
+handler = miEventQueue.handlers[type];
-if (e-pScreen != DequeueScreen(e-pDev) !handler) {
+if (screen != DequeueScreen(dev) !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);
+DequeueScreen(dev) = screen;
+x = event-u.keyButtonPointer.rootX;
+y = event-u.keyButtonPointer.rootY;
+NewCurrentScreen (dev, DequeueScreen(dev), 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
Re: [PATCH 2/4] X event queue mutex
On Mon, Nov 03, 2008 at 09:41:48PM -0800, Jeremy Huddleston wrote:
That looks much better (and is much more readable), but it's still open
to data-thrashing when the queue is full. Move this:
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
after this:
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
e is the head, and it becomes writeable by the other thread after we
do the pop... so we want to only pop it after we're done copying.
With that change, I think it's golden =)
righty-o. hunk 2 is different, with the change you suggested. To make
the flow cleaner, I moved all the EQ and memory alloc stuff together and the
copies into local variables below it.
From 218f4664a032a8096d576ab8a43f33226007b6a4 Mon Sep 17 00:00:00 2001
From: Peter Hutterer [EMAIL PROTECTED]
Date: Tue, 4 Nov 2008 15:27:30 +1030
Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all events before
processing.
Copy the EventRec's information into local variables before processing them,
this should make it safer for upcoming threading and also makes it easier to
read.
Simplify the event allocation code from the abyss it was before.
This also fixes a potential bug where a custom handler could scramble the
event before the same -now scrambled- event was then passed through the
master's custom event handler.
---
mi/mieq.c | 96 -
1 files changed, 44 insertions(+), 52 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 986e3a1..33c2936 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -307,8 +307,12 @@ mieqProcessInputEvents(void)
mieqHandler handler;
EventRec *e = NULL;
int x = 0, y = 0;
-xEvent* event,
-*master_event = NULL;
+int type, nevents, evlen, i;
+ScreenPtr screen;
+xEvent *event,
+ *master_event = NULL;
+DeviceIntPtr dev = NULL,
+ master = NULL;
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
@@ -322,79 +326,67 @@ mieqProcessInputEvents(void)
#endif
e = miEventQueue.events[miEventQueue.head];
+
+/* GenericEvents always have nevents == 1 */
+nevents = e-nevents;
+evlen = (nevents 1) ? sizeof(xEvent) : e-events-evlen;
+event = xcalloc(nevents, evlen);
+
+if (!event)
+FatalError([mi] No memory left for event processing.\n);
+
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
+
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+dev = e-pDev;
+master = (!dev-isMaster dev-u.master) ? dev-u.master : NULL;
+type= e-events-event-u.u.type;
+screen = e-pScreen;
+
/* Custom event handler */
-handler = miEventQueue.handlers[e-events-event-u.u.type];
+handler = miEventQueue.handlers[type];
-if (e-pScreen != DequeueScreen(e-pDev) !handler) {
+if (screen != DequeueScreen(dev) !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);
+DequeueScreen(dev) = screen;
+x = event-u.keyButtonPointer.rootX;
+y = event-u.keyButtonPointer.rootY;
+NewCurrentScreen (dev, DequeueScreen(dev), 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)
-{
-CopyGetMasterEvent(e-pDev-u.master, event,
- master_event, e-nevents);
-} else
+if (master)
+CopyGetMasterEvent(master, event,
+ master_event, nevents);
+else
master_event = NULL;
/* If someone's registered a custom event handler, let
Re: [PATCH 2/4] X event queue mutex
That looks much better (and is much more readable), but it's still
open to data-thrashing when the queue is full. Move this:
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
after this:
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
e is the head, and it becomes writeable by the other thread after we
do the pop... so we want to only pop it after we're done copying.
With that change, I think it's golden =)
--Jeremy
On Nov 3, 2008, at 21:06, Peter Hutterer wrote:
Sorry for the late reply, I was tied up and missed the mail.
On Thu, Oct 23, 2008 at 02:47:30PM -0700, Jeremy Huddleston wrote:
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.
with the move of the handler procesing (dac9e918) this should now be.
handler(.., event, ...);
if (!e-pDev-isMaster ...)
handler(..., master_event, ...)
otherwise we're not protected against event manipulation in the
handler.
Also, why do we call the handler twice here?
Only one event is in the queue, but we need to process it twice,
once as
coming from the slave device, once as coming from the master device.
These two
are independent.
-e = miEventQueue.events[miEventQueue.head];
+memcpy(e, miEventQueue.events[miEventQueue.head],
sizeof(EventRec));
if we're not using the EventRec as such anyway, it might just be
better to
extract the things we need and improve readability.
+event = xcalloc(e.nevents, sizeof(xEvent));
that doesn't work. GenericEvents may be larger than xEvent, hence
the slightly
weird copying code. What you are doing here corrupts memory after
the first
device switch. How about this one here, quick test shows it works:
From b783944ac784c42ba84b3dfe77d60107b9581a30 Mon Sep 17 00:00:00 2001
From: Peter Hutterer [EMAIL PROTECTED]
Date: Tue, 4 Nov 2008 15:27:30 +1030
Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all
events before processing.
Copy the EventRec's information into local variables before
processing them,
this should make it safer for upcoming threading and also makes it
easier to
read.
Simplify the event allocation code from the abyss it was before.
This also fixes a potential bug where a custom handler could
scramble the
event before the same -now scrambled- event was then passed through
the
master's custom event handler.
---
mi/mieq.c | 95 ++
+-
1 files changed, 43 insertions(+), 52 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 986e3a1..ec11e00 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -307,8 +307,12 @@ mieqProcessInputEvents(void)
mieqHandler handler;
EventRec *e = NULL;
int x = 0, y = 0;
-xEvent* event,
-*master_event = NULL;
+int type, nevents, evlen, i;
+ScreenPtr screen;
+xEvent *event,
+ *master_event = NULL;
+DeviceIntPtr dev = NULL,
+ master = NULL;
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
@@ -324,77 +328,64 @@ mieqProcessInputEvents(void)
e = miEventQueue.events[miEventQueue.head];
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+dev = e-pDev;
+master = (!dev-isMaster dev-u.master) ? dev-
u.master : NULL;
+type= e-events-event-u.u.type;
+nevents = e-nevents;
+screen = e-pScreen;
+
+/* GenericEvents always have nevents == 1 */
+evlen = (nevents 1) ? sizeof(xEvent) : e-events-evlen;
+event = xcalloc(nevents, evlen);
+
+if (!event)
+FatalError([mi] No memory left for event processing.
\n);
+
+for (i = 0; i nevents; i++)
+memcpy(event[i], e-events[i].event, evlen);
+
/* Custom event handler */
-handler = miEventQueue.handlers[e-events-event-u.u.type];
+handler = miEventQueue.handlers[type];
-if (e-pScreen != DequeueScreen(e-pDev) !handler) {
+if (screen != DequeueScreen(dev) !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;
-
Re: [PATCH 2/4] X event queue mutex
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 [EMAIL PROTECTED] Date: October 19, 2008 12:59:16 PDT To: Kevin Van Vechten [EMAIL PROTECTED], Jordan Hubbard [EMAIL PROTECTED] Cc: George Peter Staplin [EMAIL PROTECTED] 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 0x 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 smime.p7s Description: S/MIME cryptographic signature ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
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)
{
-
Re: [PATCH 2/4] X event queue mutex
On Oct 23, 2008, at 16:05, Tiago Vignatti wrote: Hey Jeremy, I'm going well. Thanks my friend. Jeremy Huddleston escreveu: 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 posted another version of that patch that doesn't requires a lock in mieqProcessInputEvents(). Hmm... I must've missed that version... I thought I was looking at the Oct 1 one, but maybe I misclicked... sorry We just need to guarantee order of events enqueued (to protect writes to the tail pointer in mieqEnqueue) and don't need to serialize dequeuing events. So no locks in the server thread is needed. (well, this is the theory of the X server with Xorg DDX and the input thread craziness. Maybe your problem is a bit different from mine) I think by not locking, we open ourselves up to clobber because we reference the event on the queue after we pop the event off the event queue: server thread: e = miEventQueue.events[miEventQueue.head]; miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE; -- context switch to input thread with a 1-open-slot event queue (miEventQueue.head = miEventQueue.head - 1) -- miEventQueue.events[miEventQueue.tail] gets set, but this is what 'e' is. -- context switch to server thread -- e was just clobbered by the input thread - I think you're right, actually, that we don't need to actually lock here. We just need to copy the data locally then pop the queue. If we keep the data in the queue and point to it, then we need to either lock (bad IMO), pop on return (possibly a good solution), or cache it locally (what I did in the patch I sent a few hours ago). smime.p7s Description: S/MIME cryptographic signature ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Tue, 2008-10-07 at 00:46 -0300, Tiago Vignatti wrote: BTW, all mieqEnqueue() calls isn't needed to be wrapped by OsBlockSignals() and OsReleaseSignals()? This is not what is happening in our code. OsBlockSignals is only required when queuing events other than from the SIGIO handler as signals are blocked while that handler is running. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
Simon Thum escreveu: Keith Packard wrote: Why does inserting events do anything but pull events from the kernel, insert them to the queue and update the sprite location on the screen? All event processing should happen in the main server thread; the only latency we're looking to reduce is the mouse-sprite update. It's really not much: Middle button emu, translation/acceleration, ... The point being their guts are exposed via input properties. Most cases are trivial, which reads 'fairly safe given properly aligned writes are atomic'. For the rest, there should be provisions (like suppressing input processing) or a clear statement against complicated stuff in props. At least, that's my opinion. (Just to note: my last patch totally ignores the eventual races issues with input thread and input device properties) Sh*t, the things are starting to get confused again. Keith was right saying that the IT needs only to worry with sprite updates, and we're _not_ doing this right now. The input thread covers all the input event generation stage and more other bits. A problem that I see is in GetPointerEvents() function. It's bloated and confusing. It generates X events and also updates the cursor on screen. We don't need to mix this two actions in one function. For instance, I did a quickly hack here calling miPointerSetPosition() directly from evdev driver (!) instead inside GPE and apparently all worked nice. So if we could only put in a separate thread the pieces that deal with cursor's update and not related with X events then we'd be winning -- I failed to not see this before :( On the other hand, if we're moving to the kernel the pieces of our stack which are close with hw interaction, then it'd be more logical to move also all this not-window-related stuff to the kernel (inside drm, etc) and forget the input thread for all eternity. So yeah, I have to think a little more about this all. I think I still have a breath to start adventure again in kernel side. Cheers, -- Tiago Vignatti C3SL - Centro de Computação Científica e Software Livre www.c3sl.ufpr.br ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Fri, 2008-10-03 at 05:57 +0300, Daniel Stone wrote: Except if the lock is held across the entire event processing, because we need to queue events from event processing. I guess that's what I meant with guaranteeing order. That makes it more important that the mutex cover precisely the values which will be modified in multiple threads, inserting events into the queue, and not pulling them out, which is done only in a single thread. Fine so far. But how do we touch structures which the IT uses before enqueuing from the main thread? E.g. with input properties, you're likely to modify IT-relevant data. I guess that's mostly a single write so visibility won't hurt much anyway, but there might be more complex ops. This should be considered a no-go or we need means to cope with it. A possiblity would be to lock the queue while processing prop handlers so the unlock guarantees visiblity, but this still doesn't provide a 100%-fix for data races. Opinions? ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
Keith Packard wrote: On Wed, 2008-10-01 at 21:39 -0300, Tiago Vignatti wrote: A mutex is needed because the X event queue is a critical region. Though the X event queue is re-entrant, we cannot guarantee the simultaneous processing by both main and input threads. The input queue is written so that each user modifies only one of the two pointers (head and tail). There shouldn't be any need to have a mutex which protects both of these values together, and doing so prevents mouse motion while the server is processing events. Is there something fundamentally different between threaded input and SIGIO- or kernel-based input that I'm missing here? I believe tiagos words are a bit misleading: The mutex makes it possible to block event enqueuing, which is needed to guarantee order of events enqueued on the main thread. If I got it right, the intent is to 'emulate' OsBlockSignals(), though I'm missing that bit. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
Keith Packard wrote:
On Thu, 2008-10-02 at 18:58 -0300, Tiago Vignatti wrote:
Keith Packard escreveu:
The input queue is written so that each user modifies only one of the
two pointers (head and tail). There shouldn't be any need to have a
mutex which protects both of these values together, and doing so
prevents mouse motion while the server is processing events.
Yeah, but the input thread can change the tail pointer while the main
thread is reading that, doing dirty things.
That won't be a problem on any SMP machine where writes from one CPU are
visible to the other CPU. The key is to have each thread writing only
one of the two values, and we have that already.
It may be constructed, but IMO this means the queue size is not fully
utilizable given head is 'old':
else {
newtail = (oldtail + 1) % QUEUE_SIZE;
if (newtail == miEventQueue.head) {
ErrorF([mi] EQ overflowing. The server is probably stuck
in an infinite loop.\n);
return;
}
miEventQueue.tail = newtail;
}
It might make sense to increase QUEUE_SIZE when threaded (I'm unaware of
any reasoning behind the current size).
BTW, given SMP visibility, isn't it a tiny bit risky to have 3
'independent' miEventQueue.head reads in miEnqueue?
So, we either need to fix the users of checkForInput along with all
users of miEventQueue, or we need to assert that we're running on a
sensible CPU that doesn't require a mutex to make memory changes visible
across processors, in which case we don't need any mutex at all.
At XDS, we identified 2 scenarios:
a) Full block of input processing (halt IT at defined state)
b) block enqueuing on IT-side (so e.g. XTest can guarantee order)
(b) may suffice. Locking the queue in OsBlockSigs() should do it and fix
most miEnqueue users.
Given that more input-related stuff is going to be done in input
properties (which are processed on the main thread), (a) may be the only
viable option in some circumstances (If I'm the only, let's forget it).
Then we'd also need to assert visibility of writes when the IT is to
continue. I don't know how to do that pthready however.
Processing input properties on the input thread may avoid that (a) crap
altogether. (cc'ed peter for that). If I didn't miss something, this and
(b) should get the job done.
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Fri, 2008-10-03 at 02:01 +0200, Simon Thum wrote: It may be constructed, but IMO this means the queue size is not fully utilizable given head is 'old': Yes, that's fairly common in queues -- you often can't use the last entry. Not a huge deal if you make the queue big enough. BTW, given SMP visibility, isn't it a tiny bit risky to have 3 'independent' miEventQueue.head reads in miEnqueue? Given the old 'SIGIO' based code, it didn't matter. Even now, it doesn't really matter as in the worst case we fail to store an event when the queue is nearly full but the server is processing events. (b) may suffice. Locking the queue in OsBlockSigs() should do it and fix most miEnqueue users. Or just lock the queue in mieqEnqueue itself; keeping the lock near the code seems like a lot better plan to me. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Thu, Oct 02, 2008 at 06:53:09PM -0700, Keith Packard wrote: On Fri, 2008-10-03 at 02:01 +0200, Simon Thum wrote: (b) may suffice. Locking the queue in OsBlockSigs() should do it and fix most miEnqueue users. Or just lock the queue in mieqEnqueue itself; keeping the lock near the code seems like a lot better plan to me. Except if the lock is held across the entire event processing, because we need to queue events from event processing. Cheers, Daniel signature.asc Description: Digital signature ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Fri, 2008-10-03 at 05:57 +0300, Daniel Stone wrote: On Thu, Oct 02, 2008 at 06:53:09PM -0700, Keith Packard wrote: On Fri, 2008-10-03 at 02:01 +0200, Simon Thum wrote: (b) may suffice. Locking the queue in OsBlockSigs() should do it and fix most miEnqueue users. Or just lock the queue in mieqEnqueue itself; keeping the lock near the code seems like a lot better plan to me. Except if the lock is held across the entire event processing, because we need to queue events from event processing. That makes it more important that the mutex cover precisely the values which will be modified in multiple threads, inserting events into the queue, and not pulling them out, which is done only in a single thread. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
[PATCH 2/4] X event queue mutex
--
Tiago Vignatti
C3SL - Centro de Computação Científica e Software Livre
www.c3sl.ufpr.br
From 6e0692f6a5757b6d838d857bdb68596a5208c6e2 Mon Sep 17 00:00:00 2001
From: Tiago Vignatti [EMAIL PROTECTED]
Date: Wed, 1 Oct 2008 21:15:15 -0300
Subject: [PATCH] X event queue mutex.
A mutex is needed because the X event queue is a critical region. Though
the X event queue is re-entrant, we cannot guarantee the simultaneous
processing by both main and input threads.
Signed-off-by: Tiago Vignatti [EMAIL PROTECTED]
---
mi/mieq.c | 55 +++
1 files changed, 55 insertions(+), 0 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 0a1b740..f9f9d8f 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -53,6 +53,10 @@ in this Software without prior written authorization from The Open Group.
# include extinit.h
# include exglobals.h
+#ifdef INPUT_THREAD
+#include pthread.h
+#endif
+
#ifdef DPMSExtension
# include dpmsproc.h
# define DPMS_SERVER
@@ -81,6 +85,13 @@ typedef struct _EventQueue {
static EventQueueRec miEventQueue;
+#ifdef INPUT_THREAD
+/* We need mutex because the X event queue is a critical region.
+Though the X event queue is re-entrant, we cannot guarantee the
+simultaneous processing by both main and input threads. */
+pthread_mutex_t miEventQueueMutex = PTHREAD_MUTEX_INITIALIZER;
+#endif
+
Bool
mieqInit(void)
{
@@ -122,6 +133,9 @@ mieqResizeEvents(int min_size)
void
mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
{
+#ifdef INPUT_THREAD
+pthread_mutex_lock(miEventQueueMutex);
+#endif
unsigned int oldtail = miEventQueue.tail, newtail;
EventListPtr evt;
intisMotion = 0;
@@ -146,6 +160,9 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
if (laste-nevents 6) {
ErrorF([mi] mieqEnqueue: more than six valuator events; dropping.\n);
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
return;
}
if (oldtail == miEventQueue.head ||
@@ -157,10 +174,16 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
((lastkbp-deviceid DEVICE_BITS) !=
(v-deviceid DEVICE_BITS))) {
ErrorF([mi] mieqEnequeue: out-of-order valuator event; dropping.\n);
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
return;
}
memcpy((laste-events[laste-nevents++].event), e, sizeof(xEvent));
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
return;
}
@@ -176,6 +199,9 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
if (newtail == miEventQueue.head) {
ErrorF([mi] EQ overflowing. The server is probably stuck
in an infinite loop.\n);
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
return;
}
miEventQueue.tail = newtail;
@@ -193,6 +219,9 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
if (!evt-event)
{
ErrorF([mi] Running out of memory. Tossing event.\n);
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
return;
}
}
@@ -212,24 +241,43 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
miEventQueue.events[oldtail].pDev = pDev;
miEventQueue.lastMotion = isMotion;
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
}
void
mieqSwitchScreen(DeviceIntPtr pDev, ScreenPtr pScreen, Bool fromDIX)
{
+#ifdef INPUT_THREAD
+pthread_mutex_lock(miEventQueueMutex);
+#endif
+
EnqueueScreen(pDev) = pScreen;
if (fromDIX)
DequeueScreen(pDev) = pScreen;
+
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
}
void
mieqSetHandler(int event, mieqHandler handler)
{
+#ifdef INPUT_THREAD
+pthread_mutex_lock(miEventQueueMutex);
+#endif
+
if (handler miEventQueue.handlers[event])
ErrorF([mi] mieq: warning: overriding existing handler %p with %p for
event %d\n, miEventQueue.handlers[event], handler, event);
miEventQueue.handlers[event] = handler;
+
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
}
/**
@@ -304,6 +352,10 @@ mieqProcessInputEvents(void)
xEvent* event,
*master_event = NULL;
+#ifdef INPUT_THREAD
+pthread_mutex_lock(miEventQueueMutex);
+#endif
+
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
dixSaveScreens (serverClient, SCREEN_SAVER_OFF, ScreenSaverReset);
@@ -390,5 +442,8 @@ mieqProcessInputEvents(void)
miPointerUpdateSprite(e-pDev);
}
}
+#ifdef INPUT_THREAD
+pthread_mutex_unlock(miEventQueueMutex);
+#endif
}
--
1.5.4.3
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [PATCH 2/4] X event queue mutex
On Wed, 2008-10-01 at 21:39 -0300, Tiago Vignatti wrote: A mutex is needed because the X event queue is a critical region. Though the X event queue is re-entrant, we cannot guarantee the simultaneous processing by both main and input threads. The input queue is written so that each user modifies only one of the two pointers (head and tail). There shouldn't be any need to have a mutex which protects both of these values together, and doing so prevents mouse motion while the server is processing events. Is there something fundamentally different between threaded input and SIGIO- or kernel-based input that I'm missing here? -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
