From owner-freebsd-arch@FreeBSD.ORG Mon Nov 29 15:32:18 2010 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D800F106566C; Mon, 29 Nov 2010 15:32:18 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 97EFD8FC1A; Mon, 29 Nov 2010 15:32:18 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 3DA5546B06; Mon, 29 Nov 2010 10:32:18 -0500 (EST) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 3B1538A027; Mon, 29 Nov 2010 10:32:17 -0500 (EST) From: John Baldwin To: freebsd-arch@freebsd.org Date: Mon, 29 Nov 2010 08:00:51 -0500 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20101102; KDE/4.4.5; amd64; ; ) References: <20101124213627.2d95840f@kan.dnsalias.net> In-Reply-To: <20101124213627.2d95840f@kan.dnsalias.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201011290800.51498.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Mon, 29 Nov 2010 10:32:17 -0500 (EST) X-Virus-Scanned: clamav-milter 0.96.3 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.9 required=4.2 tests=BAYES_00 autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on bigwig.baldwin.cx Cc: mdf@freebsd.org Subject: Re: LOR with sysctl lock X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Nov 2010 15:32:19 -0000 On Wednesday, November 24, 2010 9:36:27 pm Alexander Kabaev wrote: > On Wed, 24 Nov 2010 17:23:04 -0800 > mdf@FreeBSD.org wrote: > > > On Wed, Nov 24, 2010 at 3:49 PM, Alexander Kabaev > > wrote: > > > On Wed, 24 Nov 2010 11:53:58 -0800 > > > mdf@FreeBSD.org wrote: > > > > > >> The sysctl lock can cause "random" LOR warnings. These usually > > >> show on reboot/shutdown when sysctl_ctx_free() is called by a > > >> kernel module, since the mod handler is called with the module > > >> lock. The reason for the LOR is that, at least theoretically, the > > >> sysctl lock is the first lock in any hierarchy, because a > > >> SYSCTL_PROC handler can take any locks it wants, and will be > > >> called with the sysctl lock held shared. > > >> > > >> The below patch will fix the problem generically and with no > > >> changes to other code. I slightly prefer this to an explicit > > >> sysctl_ctx_free_sched(9), because many times code doesn't know if > > >> some caller holds *any* lock at all; this is especially true for > > >> mod handlers who shouldn't be expected to know how FreeBSD locks > > >> calls to the handler. > > >> > > >> I also note that the return value from sysctl_ctx_free(9) is almost > > >> never checked on CURRENT, and the only places it is, the value is > > >> merely used to print a warning. The only exception is > > >> canbus_detach() in pc98/pc98/canbus.c. So I wonder if > > >> sysctl_ctx_free(9) should return void and print a warning itself. > > >> > > >> Patch: > > >> http://people.freebsd.org/~mdf/0001-Proposed-patch-to-fix-LORs-with- sysctl-lock.patch > > >> > > >> If there are no objections, I'd like to commit this next week. > > >> > > >> Thanks, > > >> matthew > > > > > > Correct me if I am wrong, but doesn't this open a race where, say, > > > device detach routine destroys the device softc and schedules sysctl > > > context to be destroyed asynchronously via task queue? Since sysctl > > > entries are still visible in between the point where softc is > > > destroyed and the point where task queue picks the sysctl destroy > > > task up, can any access to said sysctls potentially operate on now > > > freed softc data? > > > > D'oh, yeah. > > > > I'm thinking of something a little more grand, now, that increments a > > hold count on the sysctl oid and releases the lock before calling the > > handler. This would prevent the LOR, and allow sysctl_ctx_free(9) to > > be run in-line, but it would then have to wait for any in progress > > sysctl calls to an oid to drain. > > > > I think this will work out; I'm planning to work on the code over the > > Thanksgiving holiday and have something ready in a few days. > > > > Thanks, > > matthew > > This sounds like a useful strategy, but I think you are attacking wrong > problem here. The linker running initialization code code under lock is > problematic and something like mutex to protect linker module lists and > some kind of gate with global flag allowing only one thread into module > load/unload code path should address the same annoying LOR as well and > possibly many more. That's called an 'sx' lock and is what the linker uses. -- John Baldwin From owner-freebsd-arch@FreeBSD.ORG Mon Nov 29 15:32:20 2010 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 07C701065673; Mon, 29 Nov 2010 15:32:20 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id CEB6B8FC1B; Mon, 29 Nov 2010 15:32:19 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 87F2146B23; Mon, 29 Nov 2010 10:32:19 -0500 (EST) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 740B78A009; Mon, 29 Nov 2010 10:32:18 -0500 (EST) From: John Baldwin To: freebsd-arch@freebsd.org Date: Mon, 29 Nov 2010 08:11:50 -0500 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20101102; KDE/4.4.5; amd64; ; ) References: In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201011290811.50262.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Mon, 29 Nov 2010 10:32:18 -0500 (EST) X-Virus-Scanned: clamav-milter 0.96.3 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.9 required=4.2 tests=BAYES_00 autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on bigwig.baldwin.cx Cc: mdf@freebsd.org Subject: Re: Fixing sysctl LOR X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Nov 2010 15:32:20 -0000 On Thursday, November 25, 2010 8:44:07 pm mdf@freebsd.org wrote: > My previous thought on the matter was incorrect. This patch (and two > small cleanups before it) mean the sysctl lock is no longer held > across the call to oid_handler, which means that WITNESS will no > longer make entries where sysctl lock is taken before any random lock > in the handler. > > The idea is simple; keep track of the number of threads running the > handler so that the oid is not deleted (and sysctl_ctx_free(9) doesn't > return) before all threads are drained. It does unfortunately mean > that the sysctl lock is only taken in exclusive mode, but since it's > held for less time I don't anticipate a significant loss of > concurrency. If there is a simple benchmark someone can recommend I'd > be happy to check the difference. > > I would like at some point to also reduce the number of calls to > sysctl(2) made by sysctl(8); this would also help performance. Among > other things I wonder if eliminating the numerical array to describe > an oid would be acceptable; in a few circumstances it would mean > longer comparisons (strcmp versus integer comparison) but for many > uses it eliminates the name2oid step, and it would also mean there's > no longer a need for fixed numbered entries. > > Cleanup: > http://people.freebsd.org/~mdf/0001-Use-the-SYSCTL_CHILDREN-macro-in- kern_sysctl.c-to-he.patch > http://people.freebsd.org/~mdf/0002-Slightly-modify-the-logic-in- sysctl_find_oid-to-redu.patch These look fine to me. > The patch: > http://people.freebsd.org/~mdf/0003-Do-not-hold-the-sysctl-lock-across-a- call-to-the-han.patch Concurrent sysctl's aren't that big of a gain anyway, especially since it only worked for in-kernel invocations (very rare) and not for userland invocations. Minor nit: --- a/sys/sys/sysctl.h +++ b/sys/sys/sysctl.h @@ -87,7 +87,7 @@ struct ctlname { #define CTLFLAG_MPSAFE 0x00040000 /* Handler is MP safe */ #define CTLFLAG_VNET 0x00020000 /* Prisons with vnet can fiddle */ #define CTLFLAG_RDTUN (CTLFLAG_RD|CTLFLAG_TUN) - +#define CTLFLAG_DYING 0x00010000 /* oid is being removed */ /* * Secure level. Note that CTLFLAG_SECURE == CTLFLAG_SECURE1. * The newline before the block comment should stay. Also, this isn't MFC'able since it breaks the KBI. If you wanted to MFC I suppose you could break the oid_refcnt into two shorts for the MFC patch (but keep it as full int's in HEAD). -- John Baldwin From owner-freebsd-arch@FreeBSD.ORG Mon Nov 29 16:31:38 2010 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id ABB3C1065694; Mon, 29 Nov 2010 16:31:38 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-iw0-f182.google.com (mail-iw0-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id 63B328FC18; Mon, 29 Nov 2010 16:31:38 +0000 (UTC) Received: by iwn39 with SMTP id 39so5834241iwn.13 for ; Mon, 29 Nov 2010 08:31:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:content-type:content-transfer-encoding; bh=X7sAdFHKPwAp1WeFSpmLBhhnedih6CWBQt7sVaxtkBE=; b=jWoBIRAc20LeASYpLooi84F2Da2T+3sMxOQCfkEG+ez688FruQub4+3EtLK0Ti0NLv zIHoW8WIUQR/I9PYInEZ7M+01OUjDndl2dhnGdpTjMU5sTJhrrAZjXpL85iz2cLVBbyE VahPuPeKgPbld3vHQTTH06BeUFKSRkdnZIUgM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:content-type :content-transfer-encoding; b=D/akEA5AhRcWb8hxEaUc4WDStMQtI6dXdLKfywn3RzJVth4H0o903MP3OnNfv1CdDX akPwVecJwyTDe5sm2iwlmrYyRoX843DNjz4YzN7sSjG1OiYIc8MtaafAhTHfP4fg5LB9 L7dgOds0LEYeUSYwGMxfhWRHF3orQo587VtgQ= MIME-Version: 1.0 Received: by 10.231.10.69 with SMTP id o5mr5828356ibo.122.1291048297892; Mon, 29 Nov 2010 08:31:37 -0800 (PST) Sender: mdf356@gmail.com Received: by 10.231.21.35 with HTTP; Mon, 29 Nov 2010 08:31:35 -0800 (PST) In-Reply-To: <201011290811.50262.jhb@freebsd.org> References: <201011290811.50262.jhb@freebsd.org> Date: Mon, 29 Nov 2010 08:31:35 -0800 X-Google-Sender-Auth: _6PfAXqzV4P-KypEBM91BpGnpj4 Message-ID: From: mdf@FreeBSD.org To: John Baldwin , freebsd-arch@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Subject: Re: Fixing sysctl LOR X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Nov 2010 16:31:38 -0000 On Mon, Nov 29, 2010 at 5:11 AM, John Baldwin wrote: > On Thursday, November 25, 2010 8:44:07 pm mdf@freebsd.org wrote: >> My previous thought on the matter was incorrect. =A0This patch (and two >> small cleanups before it) mean the sysctl lock is no longer held >> across the call to oid_handler, which means that WITNESS will no >> longer make entries where sysctl lock is taken before any random lock >> in the handler. >> >> The idea is simple; keep track of the number of threads running the >> handler so that the oid is not deleted (and sysctl_ctx_free(9) doesn't >> return) before all threads are drained. =A0It does unfortunately mean >> that the sysctl lock is =A0only taken in exclusive mode, but since it's >> held for less time I don't anticipate a significant loss of >> concurrency. =A0If there is a simple benchmark someone can recommend I'd >> be happy to check the difference. >> >> I would like at some point to also reduce the number of calls to >> sysctl(2) made by sysctl(8); this would also help performance. =A0Among >> other things I wonder if eliminating the numerical array to describe >> an oid would be acceptable; in a few circumstances it would mean >> longer comparisons (strcmp versus integer comparison) but for many >> uses it eliminates the name2oid step, and it would also mean there's >> no longer a need for fixed numbered entries. >> >> Cleanup: >> http://people.freebsd.org/~mdf/0001-Use-the-SYSCTL_CHILDREN-macro-in- > kern_sysctl.c-to-he.patch >> http://people.freebsd.org/~mdf/0002-Slightly-modify-the-logic-in- > sysctl_find_oid-to-redu.patch > > These look fine to me. > >> The patch: >> http://people.freebsd.org/~mdf/0003-Do-not-hold-the-sysctl-lock-across-a= - > call-to-the-han.patch > > Concurrent sysctl's aren't that big of a gain anyway, especially since it= only > worked for in-kernel invocations (very rare) and not for userland invocat= ions. > > Minor nit: > > --- a/sys/sys/sysctl.h > +++ b/sys/sys/sysctl.h > @@ -87,7 +87,7 @@ struct ctlname { > =A0#define CTLFLAG_MPSAFE 0x00040000 =A0 =A0 =A0/* Handler is MP safe */ > =A0#define CTLFLAG_VNET =A0 0x00020000 =A0 =A0 =A0/* Prisons with vnet ca= n fiddle */ > =A0#define CTLFLAG_RDTUN =A0(CTLFLAG_RD|CTLFLAG_TUN) > - > +#define =A0 =A0 =A0 =A0CTLFLAG_DYING =A0 0x00010000 =A0 =A0 =A0/* oid is= being removed */ > =A0/* > =A0* Secure level. =A0 Note that CTLFLAG_SECURE =3D=3D CTLFLAG_SECURE1. > =A0* > > The newline before the block comment should stay. > > Also, this isn't MFC'able since it breaks the KBI. =A0If you wanted to MF= C I > suppose you could break the oid_refcnt into two shorts for the MFC patch = (but > keep it as full int's in HEAD). I wasn't sure it was useful enough a change to be MFC'd, since few people seem to have reported issues, much less full deadlocks like we have at Isilon due to our increased use of sysctl oid's for everything under the sun. Thanks, matthew From owner-freebsd-arch@FreeBSD.ORG Thu Dec 2 15:42:24 2010 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B227D10656A8 for ; Thu, 2 Dec 2010 15:42:24 +0000 (UTC) (envelope-from avg@freebsd.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 099DF8FC1E for ; Thu, 2 Dec 2010 15:42:23 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id RAA07786 for ; Thu, 02 Dec 2010 17:23:15 +0200 (EET) (envelope-from avg@freebsd.org) Message-ID: <4CF7B9E2.40106@freebsd.org> Date: Thu, 02 Dec 2010 17:23:14 +0200 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.12) Gecko/20101029 Lightning/1.0b2 Thunderbird/3.1.6 MIME-Version: 1.0 To: freebsd-arch@freebsd.org X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: accessing data on audio cd X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Dec 2010 15:42:24 -0000 This is a pretty minor issue in the grand scheme of things, but still seems to be an architectural one. Currently we do not have a uniform way between cd(4) and acd(4) to extract data from Audio CDs. There are shared ioctls to query table of contents, start analog playback of a given track and whatnot. But no common way to digitally extract the audio data. acd(4) allows read(2) with block size of 2352 bytes. With cd(4) the only was is to use pass(4). I see a few options: 1. do nothing 1a. use of audio CDs on PCs is going to go away eventually; 1b. acd(4) is going to be replaced by cd(4) eventually, via ahci, ATA_CAM, atapicam, etc; 1c. userland programs should take care of the differences or use some form of pass-through interface and use only MMC command; 2. add support for 2352-reading to cd(4); 3. add a new shared ioctl to read a given 2352-byte block of an audio CD. I am personally torn between #1 and #3. #2 is not bad, but it's an extra complexity. The reason I am raising this issue at all is a result of my frustration with some userland libraries/programs. Some libraries handle both cd and acd but have quite a bit of duplicate code, others (correctly) handle only one of the interfaces. What do you think? Thanks! -- Andriy Gapon