[PATCH 2/4] X event queue mutex
Jeremy Huddleston
jeremyhu at freedesktop.org
Tue Nov 4 23:57:38 PST 2008
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;
int isMotion = 0;
int evlen;
@@ -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 <peter.hutterer at redhat.com>
> 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 <peter.hutterer at redhat.com>
> ---
> 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, e->pDev->u.master, e-
> >nevents);
> - }
> + handler(DequeueScreen(dev)->myNum, event, dev,
> nevents);
> + if (master)
> + handler(DequeueScreen(master)->myNum,
> + master_event, master, nevents);
> } else
> {
> /* process slave first, then master */
> - e->pDev->public.processInputProc(event, e->pDev, e-
> >nevents);
> + dev->public.processInputProc(event, dev, nevents);
>
> - if (!e->pDev->isMaster && e->pDev->u.master)
> - {
> - e->pDev->u.master-
> >public.processInputProc(master_event,
> - e->pDev->u.master, e->nevents);
> - }
> + if (master)
> + master->public.processInputProc(master_event,
> master,
> + 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)
> - {
> - miPointerUpdateSprite(e->pDev);
> - }
> + if (type == DeviceMotionNotify && dev->coreEvents)
> + miPointerUpdateSprite(dev);
> }
> }
>
> --
> 1.6.0.3
>
>
-------------- 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/20081104/42186358/attachment.bin>
More information about the xorg
mailing list