evdev - special lock-less mode for keyboard drivers

[wulf]

Summary

Currently, evdev_push(), the core of evdev KPI, takes several locks during execution.
That can lead to deadlock in keyboard driver if it has been reentered from special single-threaded context (KDB/DDB) as locks are left taken by previous caller and this caller can not complete his task and drop locks while KDB/DDB is active.

One way to avoid this deadlock is to make evdev_push() lock-less:

1. Internal evdev locks are aliased to external caller's (driver) lock so they are already taken when evdev_push() is called
2. Userland notifications that can take private locks are made asynchronously from callout that polls internal evdev state in 20Hz (tunable) interval

Test Plan

To exploit e.g. queue lock to trigger deadlock reliably one should:

1. Rebuild kernel with KDB enabled
2. Make queue lock ever-taken

a. Insert busy loop (DELAY(1000);) in to FIONREAD ioctl handler in between LOCK and UNLOCK invocations (line 374 of sys/dev/evdev/cdev.c) and rebuild kernel
b. compile and run following snippet. ("/dev/input/event2" is a path to active keyboard evdev device node)

#include <sys/ioctl.h>
#include <fcntl.h>
int main() {
    int len, fd;
    fd = open("/dev/input/event2", O_RDONLY);
    while (1) ioctl(fd, FIONREAD, &len);
    return (0);
}

3. Enter KDB with pressing CTRL+ALT+ESC from kernel virtual terminal while running 2.b.

UPDATE: This test case does not work since r339824

[dumbbell]

I'm trying this patch right now. Do you have any advice to reproduce the deadlock when the patch is not applied?

[wulf]

In D15070#320905, @dumbbell wrote:

Do you have any advice to reproduce the deadlock when the patch is not applied?

TEST PLAN added. The deadlock is 100% reproducible with DELAY(1000); inserted in to a code region protected by the lock.

[dumbbell]

I didn't try to reproduce the deadlock yet. However, I got two panics:

• First one was: panic: mutex Giant owned at /mnt/home/dumbbell/Projects/freebsd/src/SVN/head/sys/kern/kern_event.c:2288 (while I was away and the laptop had an active X session).
• Second one was: panic: page fault (during boot).

The two stacktraces are identical:

__curthread () at ./machine/pcpu.h:231
231     ./machine/pcpu.h: No such file or directory.
(kgdb) #0  __curthread () at ./machine/pcpu.h:231
#1  doadump (textdump=0)
    at /mnt/home/dumbbell/Projects/freebsd/src/SVN/head/sys/kern/kern_shutdown.c:365
#2  0xffffffff83561398 in ?? ()
#3  0xffffffff81b134b0 in M_VT_uninit_sys_uninit ()
#4  0xffffffff81b135a0 in vt_consdev ()
#5  0x0000000000000000 in ?? ()
(kgdb)

[wulf]

In D15070#321033, @dumbbell wrote:

However, I got two panics:

You can try to:

1. revert only ukbd.c/atkbd.c/kbdmux.c changes to restore old behavior
2. Comment out only evdev_set_flag(evdev, EVDEV_FLAG_LOCKLESS); lines after patch has been applied to use Giant lock instead of internal as evdev lock
3. Set kern.evdev.poll_rate sysctl to 0 disable notification callout.

I have been running patched evdev for about 1 year on 2 hosts and did not observe panics like this. Both hosts have PS/2 keyboards so atkbd is better tested than ukbd or kbdmux.

[dumbbell]

About my setup: I'm using an USB keyboard on a laptop, so all atkbd/ukbd/kbdmux are attached. I'm using ukbd directly (using libinput) for the X session (99% of the time on this laptop) and I believe kbdmux is the one used by the console. I never had a panic with the unpatched evdev.

I don't understand what you suggest: do you mean I should try one of those three options and see if the panic disappears? Note that I have no way for now to reproduce the panic. They just happened at some point.

[wulf]

Sorry for bad wording. Could you start from p.1: revert only ukbd.c, kbdmux.c and atkbd.c. This should effectively revert all changes

[dumbbell]

I continued to use the initial patch for a few days and got several similar panics (5 in total). The three panic strings I encountered are:

• panic: mutex Giant owned at /mnt/home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_event.c:2288
• panic: sleepq_add: td 0xfffff8001875d560 to sleep on wchan 0xffffffff836e0fc0 with sleeping prohibited
• panic: page fault

The backtrace is always the same and is the one I shared earlier.

After reverting the recommended files, I didn't see these panic for the past two days.

[wulf]

In D15070#322602, @dumbbell wrote:

I continued to use the initial patch for a few days and got several similar panics (5 in total). The three panic strings I encountered are:

• panic: mutex Giant owned at /mnt/home/dumbbell/Projects/freebsd/src/GIT/sys/kern/kern_event.c:2288
• panic: sleepq_add: td 0xfffff8001875d560 to sleep on wchan 0xffffffff836e0fc0 with sleeping prohibited
• panic: page fault

The backtrace is always the same and is the one I shared earlier.

Unfortunately, it looks like backtrace of a panic in the kernel dumper. Actual panic precedes it

After reverting the recommended files, I didn't see these panic for the past two days.

Ok. Then go to p.2 to narrow things down: Apply patch but delete only

evdev_set_flag(evdev, EVDEV_FLAG_LOCKLESS);

lines in ukbd.c, kbdmux.c and atkbd.c. If you has already reverted these files, than

evdev_register(evdev)

can be just replaced with

evdev_register_mtx(evdev, &Giant)

to avoid revert all/patch path

If panics appear again than please, change the files one by one, including kbdmux.c to find culprit.

It would be nice to have INVARIANTS and INVARIANTS_SUPPORT enabled, but as I see they are enabled already

[dumbbell]

Hi!

Here is a bit of feedback after nearly a month: the system is stable with only the evdev_set_flag(evdev, EVDEV_FLAG_LOCKLESS); line in kbdmux commented out.

First, I tried to keep the line in ukbd.c. I then restored the line in atkbd.v a week ago. I will uncomment it in kbdmux in a week (and probably comment out the two others).

[wulf]

Workaround knlist_cleardel() 'Mutex owned' assertion that happens on 'lockless' device detach.
It is caused by devfs which calls evdev_dtor() with driver's lock (Giant) already taken.

[wulf]

Regenerate diff after r337720

[zeising]

What is holding this from going in to the tree?

[wulf]

In D15070#377715, @zeising wrote:

What is holding this from going in to the tree?

Nothing except maybe a lack of some ukbd testing and the strange crashes reported before in this thread. Unfortunately, no good crashdumps or backtraces of them are available.
I was able to reproduce at least one "mutex Giant owned" false positive assertion and workaround it. But I have no idea in which way "sleep on wchan" can be triggered by this change.

I'm running the patch on 2.5 hosts (atkbd only, 2 hosts in everyday use and one from time to time) for 1.5 years and see no regressions.

Hi Jean-Sébastien,

Do you still observe the panics? Is it possible that they were unrelated to this change?

[dumbbell]

Yes, I confirm the patch is working for me. I'm using both the internal keyboard of the laptop and a USB keyboard (also connected to that laptop when at my desk).

[wulf]

In D15070#377715, @zeising wrote:

What is holding this from going in to the tree?

I should test https://reviews.freebsd.org/D17687 also. It could make all this changes pointless.

[zeising]

In D15070#378135, @wulf wrote:

In D15070#377715, @zeising wrote:

What is holding this from going in to the tree?

I should test https://reviews.freebsd.org/D17687 also. It could make all this changes pointless.

Hi!
If you do this, please keep me and this review updated on the results.
Thank you!
Regards
Niclas

[wulf]

Most important parts has been committed as r339823 and r339824. They fix evdev/KDB interaction with disabling dangerous codepaths.
This way raised some objections in the past which is addressed in remaining part of the review (true lockless mode). But, I dislike this part as It introduces some complications like polling callouts. That is why it has not been committed yet.
The further plan is to wait untill r339823/r339824 MFC and than abandon this revision if no new objections raised.

[wulf]

D17687 committed as r339917 made remaining parts of this revision senseless.

Big thanks to Jean-Sébastien for testing.