Page MenuHomeFreeBSD

evdev - special lock-less mode for keyboard drivers
AbandonedPublic

Authored by wulf on Apr 15 2018, 12:56 AM.

Details

Reviewers
dumbbell
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);
}
  1. Enter KDB with pressing CTRL+ALT+ESC from kernel virtual terminal while running 2.b.

UPDATE: This test case does not work since r339824

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

wulf created this revision.Apr 15 2018, 12:56 AM

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

wulf edited the summary of this revision. (Show Details)Apr 29 2018, 11:05 AM
wulf edited the test plan for this revision. (Show Details)
wulf added a comment.Apr 29 2018, 11:23 AM

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 added a comment.EditedApr 29 2018, 1:01 PM

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 added a comment.Apr 29 2018, 1:52 PM

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.

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 added a comment.Apr 29 2018, 3:19 PM

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

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 added a comment.May 5 2018, 9:21 AM

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 added a comment.EditedJun 10 2018, 4:14 PM

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 updated this revision to Diff 46379.Aug 7 2018, 7:46 PM

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 updated this revision to Diff 48372.Sep 23 2018, 8:55 AM
wulf edited the test plan for this revision. (Show Details)

Regenerate diff after r337720

What is holding this from going in to the tree?

wulf added a comment.Oct 24 2018, 6:06 PM

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?

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 added a comment.Oct 25 2018, 12:24 PM

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.

In D15070#378135, @wulf 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 updated this revision to Diff 49713.Oct 27 2018, 11:16 PM
wulf edited the test plan for this revision. (Show Details)

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 abandoned this revision.Oct 31 2018, 6:50 PM

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

Big thanks to Jean-Sébastien for testing.