Bug 97880

Created attachment 126702 [details] [review]
proposed patch v2

Short-circuit the input loop conditional.

Created attachment 126746 [details] [review]
proposed patch v3

After running (in xorg/proto dir)

  evince kbproto/specs/*.svg

Attempting to switch VT causes freeze.
The only visible change is mouse pointer disappearing.

This patch fixes the deadlock.

Created attachment 126759 [details] [review]
proposed patch v4

Fix stalls before VT switches.

Created attachment 127144 [details]
log 1

Log fragment for Xserver with the bug, with comments.

X was run as: timeout 20 xinit ...

Created attachment 127145 [details] [review]
log 1 patch

Patch used to produce the log 1.
Needs patch [1] being applied first.

Note about commented lines:
uncommenting them results in bug disappearing
(as (second) LogMessageVerb() takes time,
 the window for input thread increases).
See the next comment for explanation.

[1] https://patchwork.freedesktop.org/patch/113763/

Looks like the lack of logs have misled upstream into patch [1],
which does address the problem, but only partially.

In short, the patch does not account for non-input-thread device state.
Below is the detailed explanation.

X server keeps a list of input devices (dev list).

The sequence of events leading to the bug, as seen in the log 1, is as follows:
- user switches from X server VT
- main thread enters xf86DisableDevicesForVTSwitch

- main thread sets all dev->state in dev list to device_state_removed.
  For each dev, it (in the InputThreadUnregisterDevice):
  * acquires input_lock
  * sets dev->state to device_state_removed
  * sets inputThreadInfo->changed to true
  * releases input_lock

- as main thread traverses the list, input thread runs its main loop.
  The loop is as follows:
  * see if inputThreadInfo->changed is true.
    If it is:
    - acquire input_lock
    - process any dev->state-s which are not device_state_running
    - release input_lock
  * wait for input events
  * signal main thread about events (like with inputThreadInfo->changed)

- typically, after each `dev->state = device_state_removed` by main thread
  the input thread acquires the lock, and processes the dev->state.
  (Somehow it does not gets stuck in waiting for input events;
   this must be due to removes being input events also).

- but for the last device, the main thread releases the input_lock,
  then exits the loop in xf86Disable, and then does input_lock again,
  for all the time of VT switch.
  This leaves no chance for input thread to process the last device before
  the switch.

- upon return from VT switch, main thread attaches devices
  (in the InputThreadRegisterDevice).
  For the unprocessed device, it leaves dev->state as is,
  i.e. device_state_removed.
  All the patch [1] does is to make main thread set dev->state
  to device_state_running.
  The patch, therefore, has no effect on input thread's behaviour
  upon returning from VT.

The commit that caused the regression added input_lock to the main thread traversal of the dev list.
Before, the main thread would, for each dev:
* set dev->state to device_state_removed
* set inputThreadInfo->chaged to true
As soon as dev->state changed, the input thread would acquire the lock, and process the dev->state.
For the last device, the input thread would process it, then release the lock.
The main thread would then acquire the lock for all the VT switch.
All releases are processed; all devices would be attached (by input thread) properly upon returning from VT switch.
This is what happens when InputThreadUnregisterDev is un-patched from input_lock (comment 1).

In case the input thread would not do that in time (which never happened),
it still likely had a chance to process (any number of)
`dev->state == device_state_removed` before the main thread would reach to them.
How? Because of the order devices got attached/detached:
if the 6th device was not processed, 5 preceding ones would first be added,
and only then the presence of 6th examined
(input thread would remove the dev by that time, forcing main
 into adding the dev to the list).
The order of devices does not change, as it is determined by values of their file descriptors (corresponding to /dev/input/eventN), which do not get closed/re-opened on VT leave/enter, therefore staying the same.
This is what happens when InputThreadRegisterDev is un-patched from input_lock (comment 1).

The non-input-thread device state
(be it OS, X server's but not a dev->state one, or their combination)
change could only be initiated by input thread, through processing the dev->state-s.
That is why thread synchronization is necessary - to trigger proper detach.

Without a detach, there would be no attach
(by the input thread processing dev->state-s).
The device would not work therefore.

For the [1] to work, it must be that "non-input-thread" device state is not changed during VT switch, so that there is no need to change it upon returning.
This means that mouse would still be moving around
and key events would still be sent to windows.
This did not happen with any InputThread modifications.

[1] https://patchwork.freedesktop.org/patch/113763/

Typo in comment 6:

> - upon return from VT switch, main thread attaches devices
>   (in the InputThreadRegisterDevice).
>   For the unprocessed device, it leaves dev->state as is,
>   i.e. device_state_removed.
>   All the patch [1] does is to make main thread set dev->state
>   to device_state_running.
>   The patch, therefore, has no effect on input thread's behaviour
>   upon returning from VT.

[1] does affect the input thread, but only subtly:
- previously the sixth device was fully removed, but not re-added at all
  * both input thread and OS (or X, or their combination)
    thought it was disabled)
- now it just stays "hanged" in half-removed state
  * input thread thinks it should be turned on
  * OS (or ...) disabled it before VT switch, but not re-enabled afterwards;
    input thread would not receive any events

> This means that mouse would still be moving around
> and key events would still be sent to windows.

Or rather, all the while-on-another-VT events would be delivered at burst,
as soon as main thread releases the input_lock.

Proper working fix is found at:
https://lists.x.org/archives/xorg-devel/2016-October/051593.html