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