Date: Sat, 17 Dec 2011 18:06:43 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: Andriy Gapon <avg@freebsd.org> Cc: usb@freebsd.org, freebsd-hackers@freebsd.org, John Baldwin <jhb@freebsd.org>, mdf@freebsd.org Subject: Re: kern_yield vs ukbd_yield Message-ID: <201112171806.43764.hselasky@c2i.net> In-Reply-To: <4EECADD4.9040509@FreeBSD.org> References: <4EE51CB5.1060505@FreeBSD.org> <201112160016.33776.hselasky@c2i.net> <4EECADD4.9040509@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112171806.43764.hselasky>