[PATCH 2/4] X event queue mutex
Jeremy Huddleston
jeremyhu at freedesktop.org
Mon Nov 3 21:41:48 PST 2008
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 <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.
> ---
> 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 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/20081103/b5984344/attachment.bin>
More information about the xorg
mailing list