Date: Mon, 29 Nov 2010 08:31:35 -0800 From: mdf@FreeBSD.org To: John Baldwin <jhb@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: Fixing sysctl LOR Message-ID: <AANLkTik2iZGSF4wCQr3g8JvuyCyDRpvwUAzRoB4hwR-u@mail.gmail.com> In-Reply-To: <201011290811.50262.jhb@freebsd.org> References: <AANLkTi=yTwfMh8tLtfEFcjQmPYggxo6Nk1T=TeOieYvA@mail.gmail.com> <201011290811.50262.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Nov 29, 2010 at 5:11 AM, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTik2iZGSF4wCQr3g8JvuyCyDRpvwUAzRoB4hwR-u>