From owner-freebsd-hackers@FreeBSD.ORG Sat Dec 17 17:09:17 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0CDF11065691; Sat, 17 Dec 2011 17:09:17 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe08.c2i.net [212.247.154.226]) by mx1.freebsd.org (Postfix) with ESMTP id 9F4CD8FC14; Sat, 17 Dec 2011 17:09:15 +0000 (UTC) X-T2-Spam-Status: No, hits=-0.2 required=5.0 tests=ALL_TRUSTED, BAYES_50 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe08.swip.net (CommuniGate Pro SMTP 5.4.2) with ESMTPA id 219011199; Sat, 17 Dec 2011 18:09:13 +0100 From: Hans Petter Selasky To: Andriy Gapon Date: Sat, 17 Dec 2011 18:06:43 +0100 User-Agent: KMail/1.13.5 (FreeBSD/8.2-STABLE; KDE/4.4.5; amd64; ; ) References: <4EE51CB5.1060505@FreeBSD.org> <201112160016.33776.hselasky@c2i.net> <4EECADD4.9040509@FreeBSD.org> In-Reply-To: <4EECADD4.9040509@FreeBSD.org> X-Face: *nPdTl_}RuAI6^PVpA02T?$%Xa^>@hE0uyUIoiha$pC:9TVgl.Oq, NwSZ4V"|LR.+tj}g5 %V,x^qOs~mnU3]Gn; cQLv&.N>TrxmSFf+p6(30a/{)KUU!s}w\IhQBj}[g}bj0I3^glmC( :AuzV9:.hESm-x4h240C`9=w MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112171806.43764.hselasky@c2i.net> Cc: usb@freebsd.org, freebsd-hackers@freebsd.org, mdf@freebsd.org Subject: Re: kern_yield vs ukbd_yield X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 17 Dec 2011 17:09:17 -0000 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