reentrancy of mieqProcessInputEvents()?
Andy Ritger
aritger at nvidia.com
Thu Jun 14 16:13:53 PDT 2012
Thanks, Peter and Keith. I'll send out a patch to fix this up.
Thanks,
- Andy
On Tue, Jun 12, 2012 at 05:48:30PM -0700, Peter Hutterer wrote:
> 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