kern_yield vs ukbd_yield

[Andriy Gapon]

on 16/12/2011 01:16 Hans Petter Selasky said the following:
> I think I was not aware about the Giant locking maybe having something to do 
> about this. I was just thinking about this recently, that syscons and all 
> keyboard stuff, currently is Giant locked. Scary?

Nope :-)
I think that no syscons + keyboards code depends on the special properties of
the Giant, most likely with the prominent exception of the ukbd.
Also, the Giant is almost never acquired explicitly (again with the prominent
exception of the ukbd), which actually leads to a consequence that no lock is
acquired in the context where the kernel polls for input.  Which is a good thing
if we keep in mind that the kernel may poll for input in the context like panic
or ddb where any lock may be already held by a thread that is not able to run.

> I can always become better writing commit messages! What is your plan forward 
> then with regard to the Giant lock problem?
> 
> If one thread is like:
> 
> while (1)
> {
> lock(Giant);
> c = get nonblocking key from console();
> unlock(Giant);
> }
> 
> And the other is like:
> 
> if (callback complete) {
> lock(Giant);
> run callback();
> unlock(Giant);
> }
> 
> Who gets to run?

First, there is no need to acquire the Giant in the first snippet.
Second, "get nonblocking key from console" should not depend on the callback in
a parallel thread.  The polling routine should go all the way to hardware if needed.

I see where all the complications in ukbd code come from.  I think that they
started as attempts to work around the current stupid behavior of syscons during
polling:
do {
	kbd_poll_enter();
	try_to_get_one_char();
	kbd_poll_exit();
} while (!got_a_char);
For ukbd this means ugly races between the polling thread and the usb threads at
the mountroot stage.  And so you had to invent "polling autodetection" and add
extra locking.  But that lead to troubles in the contexts where no lock was
previously held.  And so on.  So now ukbd resembles a big collection of hacks
and crotches.

For example consider the following scenario which does not occur very frequently
in reality, but is quite possible.  Thread T1 on CPU P1 holds the Giant, at the
same time thread T2 on CPU P2 enters ddb.  That means that T1 is "frozen" and
won't be able to release Giant.  ddb would try to interact with an operator and
there would be a call-chain like this:
ukbd_check_char
scgetc
sc_cngetc
cncheckc
cngetc
db_readline
...

And now the code in ukbd to which I referred above:
/* check if char is waiting */
static int
ukbd_check_char(keyboard_t *kbd)
{
        struct ukbd_softc *sc = kbd->kb_data;

        if (!KBD_IS_ACTIVE(kbd))
                return (0);

        if (sc->sc_flags & UKBD_FLAG_POLLING) {
                if (!mtx_owned(&Giant)) {
                        /* XXX cludge */
                        int retval;
                        mtx_lock(&Giant);
                        retval = ukbd_check_char(kbd);
                        mtx_unlock(&Giant);
                        return (retval);

So we are in a polling mode and Giant is not owned by us (T2) because it is
owned by T1 and so mtx_lock would be called and then we would probably get into
a recursion via mi_switch and kdb_reenter.

-- 
Andriy Gapon