kern_yield vs ukbd_yield

[Hans Petter Selasky]

On Saturday 17 December 2011 15:57:24 Andriy Gapon wrote:
> 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?
> 

Hi,

> 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.

It can do that. This feature is usually only used in panic mode.

> 
> 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.

Right! You got it!

> 
> 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.

That's true!

If the problem is only in UKBD driver, I don't think this is a big problem to 
solve. The reason for the auto-magic locking, is that I've sometimes seen 
callers in non-polling mode call into UKBD without Giant locked. And this 
causes a panic when starting USB transfers, because the code expects Giant to 
be locked.

Requirement: I would say that to use UKBD polling mode, the scheduler must be 
stopped. Else you should not use polling mode at all. I think that mountroot 
is getting characters the wrong way in this regard.

--HPS