From owner-freebsd-hackers@FreeBSD.ORG Thu Sep 18 07:10:22 2008 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2A4041065689; Thu, 18 Sep 2008 07:10:22 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id C9BC28FC19; Thu, 18 Sep 2008 07:10:21 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) DomainKey-Signature: a=rsa-sha1; q=dns; c=simple; s=one; d=codelabs.ru; h=Received:Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:Sender; b=XqeyMbme9aEM1rFkb9TFp7a9x18RfvU0oHEGg4SXoFuB2dqO/R6dm56h2XXADdRpscvsbViNKSGwiym0tcbJkgN2FoJRItn93ZFo7LMcW3wDztZWEBV2HYeFndj2D9oKDZIDAP2foXS/CrI2W4y4QZSG7ute3YW46jxzb3bF0rs=; Received: from void.codelabs.ru (void.codelabs.ru [144.206.177.25]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1KgDed-000Pgj-3v; Thu, 18 Sep 2008 11:10:19 +0400 Date: Thu, 18 Sep 2008 11:10:17 +0400 From: Eygene Ryabinkin To: Maksim Yevmenkin Message-ID: References: <20080917161633.9E2F717101@shadow.codelabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="RwxaKO075aXzzOz0" Content-Disposition: inline In-Reply-To: Sender: rea-fbsd@codelabs.ru Cc: rik@freebsd.org, hackers@freebsd.org, bug-followup@freebsd.org Subject: Re: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c 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: Thu, 18 Sep 2008 07:10:22 -0000 --RwxaKO075aXzzOz0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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() =3D=3D 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. --=20 Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual =20 )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook=20 {_.-``-' {_/ # --RwxaKO075aXzzOz0 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (FreeBSD) iEYEARECAAYFAkjR/tkACgkQthUKNsbL7Yg0aQCfWt7wmcfpSO+b6MUYqatkYCLt RjcAn24xyFKL23AE2lCIAQDV1ht0/Igi =1kiS -----END PGP SIGNATURE----- --RwxaKO075aXzzOz0--