kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c

[Eygene Ryabinkin]

Maksim, good day.

Wed, Sep 17, 2008 at 10:52:15AM -0700, Maksim Yevmenkin wrote:
> yes, giant is recursive. i think it should be fine for now (and yes, i
> agree, its not very clean)

OK, I had tried substituting KBDMUX_LOCK/UNLOCK with Giant operations --
it works as expected.  I am sligtly concerned with the fact that, for
example, kbdmux_kbd_event() will grab Giant for some more time than the
initial version that protects only polling loop.

Probably it is not a big concern: the call chain from syscons's cngetc()
(via cncheckc(), syscons->cn_getc() == sc_cngetc(), sccngetch(),
scgetc() and kbd_read_char()) to the kbdmux_read_char() is the only code
path that is not protected by Giant, being called from the kernel
directly:

- user-level code is notified about key presses by syscons that signals
  tty layer about key press from sckbdevent();

- no other kbdmux routine seem to be called without being
  Giant-protected (at least, I see no parts that can race with the
  low-level keyboard events).

So the typical overhead of mangling with Giant at KBDMUX_{LOCK,UNLOCK}
is only in extra calls to the _mtx_lock_flags/_mtx_unlock_flags.  The
only extra code that will hold Giant for a longer time is the kernel's
interactive input routines, but their performance is user-bounded ;))

There is one interesting question: I assume that clock interrupts are
lost when Giant is hold?  If so, then holding Giant for some extra time
upon system's initialization when kernel waits for user input will
enable the system to drop bigger amounts of clock interrupts -- I assume
that the code in kbdmux_read_char() that translates the scancode takes
the biggest amount of run-time compared to the polling loop.  Sure, the
overhead will be added just for the typed characters -- when there is no
input, overhead is rather small.

May be this will not lead to any bad (or visible/measurable)
consequences -- can't tell now.
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual   
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook 
    {_.-``-'         {_/            #
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20080918/c1219d15/attachment.pgp