reentrancy of mieqProcessInputEvents()?
Peter Hutterer
peter.hutterer at who-t.net
Tue Jun 12 17:48:30 PDT 2012
On Tue, Jun 12, 2012 at 05:11:49PM -0700, Andy Ritger wrote:
> Is mi/mieq.c:mieqProcessInputEvents() expected to be reentrant? Or should
> mieqProcessInputEvents() never be called in that situation?
>
> More specifically, is there a reason that the 'event' local variable in
> mieqProcessInputEvents() is declared static?
>
> static InternalEvent event;
I don't think so, this doesn't look right. looking at the log this was added
in xorg-server-1.5.99.1-136-ga939368 to avoid calloc but you're certainly
right, it breaks reentrancy. Following the history of that, it's not even
needed anymore, this came from a time when we had to callloc a list of
devices - now we can have just have one InternalEvent on the stack.
see more below though.
> Consider the following sequence:
>
> (a) User hits ctrl-alt-+ to do an old-school mode switch, and the keyboard
> event is put on the event queue.
>
> (b) mieqProcessInputEvents() pops the keyboard event from the event queue,
> storing the event data in a static local variable, and then passing a
> pointer to that variable into mieqProcessDeviceEvent():
>
> mieqProcessInputEvents(void)
> {
> [...]
> static InternalEvent event;
> [...]
> while (miEventQueue.head != miEventQueue.tail) {
> e = &miEventQueue.events[miEventQueue.head];
>
> event = *e->events;
> [...]
> mieqProcessDeviceEvent(dev, &event, screen);
> [...]
> }
>
> (c) The event pointer gets passed through several functions until we
> hit XkbHandleActions():
>
> #34 0x0000000000585596 in XkbHandleActions (dev=0x1176370, kbd=0x1176370, event=0x85cf00) at xkbActions.c:1234
> #35 0x00000000005865d8 in XkbProcessKeyboardEvent (event=0x85cf00, keybd=0x1176370) at xkbPrKeyEv.c:146
> #36 0x000000000057ad28 in AccessXFilterPressEvent (event=0x85cf00, keybd=0x1176370) at xkbAccessX.c:569
> #37 0x000000000058678e in ProcessKeyboardEvent (ev=0x85cf00, keybd=0x1176370) at xkbPrKeyEv.c:181
> #38 0x00000000005aec39 in mieqProcessDeviceEvent (dev=0x1176370, event=0x85cf00, screen=0xaa49e0) at mieq.c:557
> #39 0x00000000005aee75 in mieqProcessInputEvents () at mieq.c:624
> #40 0x000000000048adb9 in ProcessInputEvents () at xf86Events.c:164
> #41 0x0000000000430835 in Dispatch () at dispatch.c:353
> #42 0x0000000000421a45 in main (argc=1, argv=0x7fff049880d8, envp=0x7fff049880e8) at main.c:288
>
> (d) XkbHandleActions() will look at the event type to decide it is a
> keyEvent type, query the key action type, perform some operation based
> on the key action type, then pass the event into FixKeyState():
>
> void
> XkbHandleActions(DeviceIntPtr dev, DeviceIntPtr kbd, DeviceEvent *event)
> {
> [...]
> keyEvent = ((event->type == ET_KeyPress) || (event->type == ET_KeyRelease));
> [...]
> act = XkbGetKeyAction(xkbi, &xkbi->state, key);
> [...]
> switch (act.type) {
> [...]
> case XkbSA_XFree86Private:
> filter = _XkbNextFreeFilter(xkbi);
> sendEvent = _XkbFilterXF86Private(xkbi, filter, key, &act);
> break;
> }
> }
> [...]
> if (keyEvent) {
> FixKeyState(event, dev);
> }
> [...]
> }
>
> (e) FixKeyState() does this:
>
> if (event->type == ET_KeyPress)
> set_key_down(keybd, key, KEY_PROCESSED);
> else if (event->type == ET_KeyRelease)
> set_key_up(keybd, key, KEY_PROCESSED);
> else
> FatalError("Impossible keyboard event");
>
> (that will be important later...)
>
> (f) Back in XkbHandleActions(), we called _XkbFilterXF86Private which
> passes the key event to the XFree86 layer, which decodes the key event as
> "+vmode", and we call through the pScrnInfo->SwitchMode wrap chain:
>
> #6 0x00000000004df525 in xf86CursorSwitchMode (index=0, mode=0x1c54eb0, flags=0) at xf86Cursor.c:253
> #7 0x0000000000496ef1 in CMapSwitchMode (index=0, mode=0x1c54eb0, flags=0) at xf86cmap.c:492
> #8 0x0000000000485fb1 in xf86SwitchMode (pScreen=0x1c40c00, mode=0x1c54eb0) at xf86Cursor.c:232
> #9 0x00000000004863f8 in xf86ZoomViewport (pScreen=0x1c40c00, zoom=1) at xf86Cursor.c:332
> #10 0x000000000048ae8e in xf86ProcessActionEvent (action=ACTION_NEXT_MODE, arg=0x0) at xf86Events.c:191
> #11 0x00000000004eb36c in XkbDDXPrivate (dev=0x22ac580, key=86 'V', act=0x7ffff8e60770) at xkbPrivate.c:32
> #12 0x0000000000584746 in _XkbFilterXF86Private (xkbi=0x22ad470, filter=0x2265400, keycode=86,
> pAction=0x7ffff8e60770) at xkbActions.c:959
> #13 0x0000000000585596 in XkbHandleActions (dev=0x22ac580, kbd=0x22ac580, event=0x85cf00)
> at xkbActions.c:1234
>
> (g) Someone in the pScrnInfo->SwitchMode wrap chain calls RRTellChanged()
> (in my case, it is the NVIDIA X driver, but I think many
> drivers could be exposed to this through xf86SetSingleMode() ->
> xf86RandR12TellChanged()) which calls UpdateCurrentTime().
>
> (h) UpdateCurrentTime() conditionally calls ProcessInputEvents():
>
> void
> UpdateCurrentTime(void)
> {
> [...]
> if (*checkForInput[0] != *checkForInput[1])
> ProcessInputEvents();
> [...]
> }
>
> ---
>
> void
> ProcessInputEvents(void)
> {
> int x, y;
>
> mieqProcessInputEvents();
>
> /* FIXME: This is a problem if we have multiple pointers */
> miPointerGetPosition(inputInfo.pointer, &x, &y);
>
> xf86SetViewport(xf86Info.currentScreen, x, y);
> }
>
> (i) checkForInput[0] and [1] point to miEventQueue.{head,tail}; and tail
> potentially got updated earlier in this sequence:
>
> 0x00000000005ae0aa in mieqEnqueue (pDev=0x2309c00, e=0x26e2e20) at mieq.c:322
> 322 miEventQueue.tail = (oldtail + 1) % miEventQueue.nevents;
> #0 0x00000000005ae0aa in mieqEnqueue (pDev=0x2309c00, e=0x26e2e20) at mieq.c:322
> #1 0x00000000005baae7 in miPointerMove (pDev=0x2309c00, pScreen=0x21dc7f0, x=1600, y=600) at mipointer.c:703
> #2 0x00000000005b9f0c in miPointerWarpCursor (pDev=0x2309c00, pScreen=0x21dc7f0, x=1600, y=600) at mipointer.c:371
> #3 0x00000000004875e1 in xf86WarpCursor (pDev=0x2309c00, pScreen=0x21dc7f0, x=1600, y=600) at xf86Cursor.c:476
> #4 0x0000000000486f65 in xf86SwitchMode (pScreen=0x21dc7f0, mode=0x21fac80) at xf86Cursor.c:283
> #5 0x000000000048712b in xf86ZoomViewport (pScreen=0x21dc7f0, zoom=1) at xf86Cursor.c:333
> #6 0x000000000048bba2 in xf86ProcessActionEvent (action=ACTION_NEXT_MODE, arg=0x0) at xf86Events.c:191
> #7 0x00000000004ebed8 in XkbDDXPrivate (dev=0x26bbc00, key=86 'V', act=0x7fff8e43bc70) at xkbPrivate.c:32
> #8 0x00000000005845ae in _XkbFilterXF86Private (xkbi=0x26bcaf0, filter=0x233b650, keycode=86, pAction=0x7fff8e43bc70)
> at xkbActions.c:958
> #9 0x0000000000585119 in XkbHandleActions (dev=0x26bbc00, kbd=0x26bbc00, event=0x85c500) at xkbActions.c:1194
>
> So, UpdateCurrentTime() may call ProcessInputEvents(), which will reenter
> mieqProcessInputEvents().
>
> (j) mieqProcessInputEvents() will overwrite the contents of the static
> 'event' variable, such that when we finally get back up to
> XkbHandleActions() and it calls FixKeyState(), the contents of
> 'event' are no longer consistent with the value of keyEvent and we
> hit the FatalError() case.
>
> Making the 'event' variable in mieqProcessInputEvents() non-static
> avoids the FatalError() in FixKeyState(), but is it problematic to
> have the re-entered instance of mieqProcessInputEvents() process events
> from miEventQueue before the first instance of mieqProcessInputEvents()
> has completed processing of its current event?
good analysis, thanks. afaict we need a mutex in
mieqProcessInputEvents(), it's certainly not designed to be reentrant and
it's pointless anyway - if you're already processing events, why would you
need to do so again?
most of the calls to ProcessInputEvents() from the server are intended as
"process input now instead of going to sleep".
Cheers,
Peter
More information about the xorg-devel
mailing list