From owner-svn-src-all@FreeBSD.ORG Sat Nov 16 08:24:42 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8847BF8A; Sat, 16 Nov 2013 08:24:42 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 33E1623BD; Sat, 16 Nov 2013 08:24:41 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id D64D07815D4; Sat, 16 Nov 2013 19:24:32 +1100 (EST) Date: Sat, 16 Nov 2013 19:24:32 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin Subject: Re: svn commit: r258174 - head/sys/kern In-Reply-To: <201311151529.rAFFTrTN062522@svn.freebsd.org> Message-ID: <20131116183038.C1344@besplex.bde.org> References: <201311151529.rAFFTrTN062522@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=DstvpgP+ c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=ekw1U9Hkn-AA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=M4roAWbnUW4A:10 a=zY6XHOb56D_KJJVBn_wA:9 a=WmoTucS6ZT25Yo6j:21 a=z9U1w9ffVQ2peHBE:21 a=CjuIK1q_8ugA:10 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 16 Nov 2013 08:24:42 -0000 On Fri, 15 Nov 2013, John Baldwin wrote: > Log: > Don't allow vfs.lorunningspace or vfs.hirunningspace to be set such > that lorunningspace is greater than hirunningspace as the system > performs terribly if it is mistuned in this fashion. Ugh, almost all writeable sysctls have foot-shooting capabilities, and this one is especially not in need of anti-foot-shooting if it only gives terrible performance. Foot-shooting with sysctl gives things like panics if you are lucky. It is a feature that sysctls can be changed independently through possibly invalid intermediate states. This allows quick changes without having to think much about the intermediate states if you know that the invalid ones won't do any unrecoverable damage. I admit to being careful when changing the nearby sysctls for dirty buffers only through valid and not very suboptimal intermediate states because I don't know how harmful invalid intermediate states are. In my version of bge, I use the better design of scattered independent non-range-checked sysctls that mostly aren't sent to the hardware immediately; then another sysctl (or down/up) syncs the changes to the hardware. The valid range of each depends on the others in a complicated way that is too hard for the system to know. > Modified: head/sys/kern/vfs_bio.c > ============================================================================== > --- head/sys/kern/vfs_bio.c Fri Nov 15 15:14:07 2013 (r258173) > +++ head/sys/kern/vfs_bio.c Fri Nov 15 15:29:53 2013 (r258174) > @@ -118,6 +118,7 @@ static int flushbufqueues(int, int); > static void buf_daemon(void); > static void bremfreel(struct buf *bp); > static __inline void bd_wakeup(void); > +static int sysctl_runningspace(SYSCTL_HANDLER_ARGS); > #if defined(COMPAT_FREEBSD4) || defined(COMPAT_FREEBSD5) || \ > defined(COMPAT_FREEBSD6) || defined(COMPAT_FREEBSD7) > static int sysctl_bufspace(SYSCTL_HANDLER_ARGS); > @@ -167,10 +168,12 @@ static int bufdefragcnt; > SYSCTL_INT(_vfs, OID_AUTO, bufdefragcnt, CTLFLAG_RW, &bufdefragcnt, 0, > "Number of times we have had to repeat buffer allocation to defragment"); > static long lorunningspace; > -SYSCTL_LONG(_vfs, OID_AUTO, lorunningspace, CTLFLAG_RW, &lorunningspace, 0, > +SYSCTL_PROC(_vfs, OID_AUTO, lorunningspace, CTLTYPE_LONG | CTLFLAG_MPSAFE | > + CTLFLAG_RW, &lorunningspace, 0, sysctl_runningspace, "L", > "Minimum preferred space used for in-progress I/O"); > static long hirunningspace; > -SYSCTL_LONG(_vfs, OID_AUTO, hirunningspace, CTLFLAG_RW, &hirunningspace, 0, > +SYSCTL_PROC(_vfs, OID_AUTO, hirunningspace, CTLTYPE_LONG | CTLFLAG_MPSAFE | > + CTLFLAG_RW, &hirunningspace, 0, sysctl_runningspace, "L", > "Maximum amount of space to use for in-progress I/O"); > int dirtybufferflushes; > SYSCTL_INT(_vfs, OID_AUTO, dirtybufferflushes, CTLFLAG_RW, &dirtybufferflushes, > @@ -332,6 +335,34 @@ const char *buf_wmesg = BUF_WMESG; > #define VFS_BIO_NEED_FREE 0x04 /* wait for free bufs, hi hysteresis */ > #define VFS_BIO_NEED_BUFSPACE 0x08 /* wait for buf space, lo hysteresis */ > > +static int > +sysctl_runningspace(SYSCTL_HANDLER_ARGS) > +{ > + long value; > + int error; > + > + value = *(long *)arg1; > + error = sysctl_handle_long(oidp, &value, 0, req); > + if (error != 0 || req->newptr == NULL) > + return (error); > + mtx_lock(&rbreqlock); > + if (arg1 == &hirunningspace) { > + if (value < lorunningspace) > + error = EINVAL; > + else > + hirunningspace = value; > + } else { > + KASSERT(arg1 == &lorunningspace, > + ("%s: unknown arg1", __func__)); Style bug. vfs_bio.c doesn't use the __func__ obfuscation anywhere else. It had 65 KASSERT()'s. About half of these don't even print their function name. The fix is not to consistently use the obfuscation to print the function name. See an old bikeshed. > 66 times. > + if (value > hirunningspace) > + error = EINVAL; > + else > + lorunningspace = value; > + } Terrible performance is probably still possible by setting the watermarks to equal values. But it would be just a bug to prevent the user setting that, since it is a special case of a small gap between the watermarks. The system should not enforce any particular gap, since it doesn't know the correct gap and the reason for existence of these sysctls is to let the user tell it the correct gap. > + mtx_unlock(&rbreqlock); > + return (error); > +} > + > #if defined(COMPAT_FREEBSD4) || defined(COMPAT_FREEBSD5) || \ > defined(COMPAT_FREEBSD6) || defined(COMPAT_FREEBSD7) > static int FreeBSD has too many sysctls (3672 on freefall now), and this much code for each of them would take about as much object space as an entire non-bloated kernel (1-2MB). Much larger and complicated code would be required to prevent all invalid intermediate states. It is almost better to write a 10MB GUI application for each combination of related sysctls. Bruce