Re: reentrancy of mieqProcessInputEvents()?
Jeremy Huddleston writes: > Should RRTellChanged be calling UpdateCurrentTimeIf() instead of > UpdateCurrentTime()? Probably. > Is there a way we can make this API more robust rather than just > adding assertions to the implementation? For example, why does > UpdateCurrentTime need to ProcessInputEvents? To keep event timestamps monotonic and accurate. Without processing events, server time can race ahead of input timestamps. -- keith.pack...@intel.com pgpzQvEJRj69W.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: reentrancy of mieqProcessInputEvents()?
On Jun 12, 2012, at 5:48 PM, Peter Hutterer wrote: > On Tue, Jun 12, 2012 at 05:11:49PM -0700, Andy Ritger wrote: >> ... >> 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? FWIW, XQuartz does have a mutex in mieqProcessInputEvents, and I haven't had any reports of deadlock, so my guess is that DIX doesn't re-enter mieqProcessInputEvents for most cases. This UpdateCurrentTime() case is certainly worrisome. Should RRTellChanged be calling UpdateCurrentTimeIf() instead of UpdateCurrentTime()? Is there a way we can make this API more robust rather than just adding assertions to the implementation? For example, why does UpdateCurrentTime need to ProcessInputEvents? ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: reentrancy of mieqProcessInputEvents()?
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 0x00585596 in XkbHandleActions (dev=0x1176370, kbd=0x1176370,
> > event=0x85cf00) at xkbActions.c:1234
> > #35 0x005865d8 in XkbProcessKeyboardEvent (event=0x85cf00,
> > keybd=0x1176370) at xkbPrKeyEv.c:146
> > #36 0x0057ad28 in AccessXFilterPressEvent (event=0x85cf00,
> > keybd=0x1176370) at xkbAccessX.c:569
> > #37 0x0058678e in ProcessKeyboardEvent (ev=0x85cf00,
> > keybd=0x1176370) at xkbPrKeyEv.c:181
> > #38 0x005aec39 in mieqProcessDeviceEvent (dev=0x1176370,
> > event=0x85cf00, screen=0xaa49e0) at mieq.c:557
> > #39 0x005aee75 in mieqProcessInputEvents () at mieq.c:624
> > #40 0x0048adb9 in ProcessInputEvents () at xf86Events.c:164
> > #41 0x00430835 in Dispatch () at dispatch.c:353
> > #42 0x00421a45 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 0x004df525 in xf86CursorSwitchMode (index=0, mode=0x1c54eb0,
> > flags=0) at xf86Cursor.c:253
> > #7 0x00496ef1 in CMapSwitchMode (index=0, mode=0x1c54eb0, flags=0)
> > at xf86cmap.c:492
> > #8 0x00485fb1 in xf86SwitchMode (pScreen=0x1c40c00,
> > mode=0x1c54eb0) at xf86Cursor.c:232
> > #9 0x004863f8 in xf86ZoomViewport (pScreen=0x1c40c00, zoom=1) at
> > xf86Cursor.c:332
> > #10 0x0048ae8e in xf86ProcessActionEvent (action=ACTION_NEXT_MODE,
> > arg=0x0) at xf86Events.c:191
> > #11 0x004eb36c in XkbDDXPrivate (dev=0x22ac580, key=86 'V',
> > act=0x78e60770) at xkbPrivate.c:32
> > #12 0x00584746 in _XkbFilterXF86Private (xkbi=0x22ad470,
> > fil
Re: reentrancy of mieqProcessInputEvents()?
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 0x00585596 in XkbHandleActions (dev=0x1176370, kbd=0x1176370,
> event=0x85cf00) at xkbActions.c:1234
> #35 0x005865d8 in XkbProcessKeyboardEvent (event=0x85cf00,
> keybd=0x1176370) at xkbPrKeyEv.c:146
> #36 0x0057ad28 in AccessXFilterPressEvent (event=0x85cf00,
> keybd=0x1176370) at xkbAccessX.c:569
> #37 0x0058678e in ProcessKeyboardEvent (ev=0x85cf00, keybd=0x1176370)
> at xkbPrKeyEv.c:181
> #38 0x005aec39 in mieqProcessDeviceEvent (dev=0x1176370,
> event=0x85cf00, screen=0xaa49e0) at mieq.c:557
> #39 0x005aee75 in mieqProcessInputEvents () at mieq.c:624
> #40 0x0048adb9 in ProcessInputEvents () at xf86Events.c:164
> #41 0x00430835 in Dispatch () at dispatch.c:353
> #42 0x00421a45 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 0x004df525 in xf86CursorSwitchMode (index=0, mode=0x1c54eb0,
> flags=0) at xf86Cursor.c:253
> #7 0x00496ef1 in CMapSwitchMode (index=0, mode=0x1c54eb0, flags=0)
> at xf86cmap.c:492
> #8 0x00485fb1 in xf86SwitchMode (pScreen=0x1c40c00, mode=0x1c54eb0)
> at xf86Cursor.c:232
> #9 0x004863f8 in xf86ZoomViewport (pScreen=0x1c40c00, zoom=1) at
> xf86Cursor.c:332
> #10 0x0048ae8e in xf86ProcessActionEvent (action=ACTION_NEXT_MODE,
> arg=0x0) at xf86Events.c:191
> #11 0x004eb36c in XkbDDXPrivate (dev=0x22ac580, key=86 'V',
> act=0x78e60770) at xkbPrivate.c:32
> #12 0x00584746 in _XkbFilterXF86Private (xkbi=0x22ad470,
> filter=0x2265400, keycode=86,
> pAction=0x78e60770) at xkbActions.c:959
> #13 0x00585596 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 thr
Re: reentrancy of mieqProcessInputEvents()?
Andy Ritger writes: > Is mi/mieq.c:mieqProcessInputEvents() expected to be reentrant? Or should > mieqProcessInputEvents() never be called in that situation? It should never be re-entered; probably would be a good idea to check for that and whinge loudly in the log. > (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(). This function needs to use UpdateCurrentTimeIf to avoid recursing. > 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? Yeah, that's not a good plan. -- keith.pack...@intel.com pgpa6PxNAGubt.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
reentrancy of mieqProcessInputEvents()?
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;
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 0x00585596 in XkbHandleActions (dev=0x1176370, kbd=0x1176370,
event=0x85cf00) at xkbActions.c:1234
#35 0x005865d8 in XkbProcessKeyboardEvent (event=0x85cf00,
keybd=0x1176370) at xkbPrKeyEv.c:146
#36 0x0057ad28 in AccessXFilterPressEvent (event=0x85cf00,
keybd=0x1176370) at xkbAccessX.c:569
#37 0x0058678e in ProcessKeyboardEvent (ev=0x85cf00, keybd=0x1176370)
at xkbPrKeyEv.c:181
#38 0x005aec39 in mieqProcessDeviceEvent (dev=0x1176370,
event=0x85cf00, screen=0xaa49e0) at mieq.c:557
#39 0x005aee75 in mieqProcessInputEvents () at mieq.c:624
#40 0x0048adb9 in ProcessInputEvents () at xf86Events.c:164
#41 0x00430835 in Dispatch () at dispatch.c:353
#42 0x00421a45 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 0x004df525 in xf86CursorSwitchMode (index=0, mode=0x1c54eb0,
flags=0) at xf86Cursor.c:253
#7 0x00496ef1 in CMapSwitchMode (index=0, mode=0x1c54eb0, flags=0) at
xf86cmap.c:492
#8 0x00485fb1 in xf86SwitchMode (pScreen=0x1c40c00, mode=0x1c54eb0) at
xf86Cursor.c:232
#9 0x004863f8 in xf86ZoomViewport (pScreen=0x1c40c00, zoom=1) at
xf86Cursor.c:332
#10 0x0048ae8e in xf86ProcessActionEvent (action=ACTION_NEXT_MODE,
arg=0x0) at xf86Events.c:191
#11 0x004eb36c in XkbDDXPrivate (dev=0x22ac580, key=86 'V',
act=0x78e60770) at xkbPrivate.c:32
#12 0x00584746 in _XkbFilterXF86Private (xkbi=0x22ad470,
filter=0x2265400, keycode=86,
pAction=0x78e60770) at xkbActions.c:959
#13 0x00585596 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,tai
