From owner-svn-src-all@FreeBSD.ORG Sat Feb 25 23:33:29 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B38AA106566B; Sat, 25 Feb 2012 23:33:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail12.syd.optusnet.com.au (mail12.syd.optusnet.com.au [211.29.132.193]) by mx1.freebsd.org (Postfix) with ESMTP id 4B11D8FC0A; Sat, 25 Feb 2012 23:33:29 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail12.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q1PNXOP5027280 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 26 Feb 2012 10:33:26 +1100 Date: Sun, 26 Feb 2012 10:33:23 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Maxim Konovalov In-Reply-To: <201202251206.q1PC6eaF016823@svn.freebsd.org> Message-ID: <20120226093104.F1833@besplex.bde.org> References: <201202251206.q1PC6eaF016823@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r232156 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 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, 25 Feb 2012 23:33:29 -0000 On Sat, 25 Feb 2012, Maxim Konovalov wrote: > Log: > o Reduce chances for integer overflow. > o More verbose sysctl description added. > > MFC after: 2 weeks > Sponsored by: Nginx, Inc. > Modified: head/sys/kern/vfs_cache.c > ============================================================================== > --- head/sys/kern/vfs_cache.c Sat Feb 25 11:07:32 2012 (r232155) > +++ head/sys/kern/vfs_cache.c Sat Feb 25 12:06:40 2012 (r232156) > ... > @@ -386,7 +386,7 @@ sysctl_debug_hashstat_nchash(SYSCTL_HAND > } > SYSCTL_PROC(_debug_hashstat, OID_AUTO, nchash, CTLTYPE_INT|CTLFLAG_RD| > CTLFLAG_MPSAFE, 0, 0, sysctl_debug_hashstat_nchash, "I", > - "nchash chain lengths"); > + "nchash statistics (number of total/used buckets, maximum chain length, usage percentage)"); > #endif > > /* It is now excessively verbose, and misformats both the source code and the output. This is not the place to write missing man pages. Just "nchash statistics" is almost enough. "nchash chain lengths" was just wrong, since this sysctl only returns a summary of the chain lengths. Its description seems to have been copied from the previous sysctl where the description was correct. This and the previous sysctl are now under DIAGNOSTIC, since they took too long and spammed sysctl -a (although the spam is not usually printed) and complicated the locking. That might have been a mistake for this sysctl, since it only returns 4 integers. The previous sysctl returns the length of each chain, 1 integer at a time. 1 at a time in this sysctl is a poor organization too. Now it is very easy to use an array containing everything to be returned, and return everything using only 1 SYSCTL_OUT() with only 1 error check. Other style bugs in this sysctl: % static int % sysctl_debug_hashstat_nchash(SYSCTL_HANDLER_ARGS) % { % int error; % struct nchashhead *ncpp; % struct namecache *ncp; % int n_nchash; % int count, maxlength, used, pct; These declarations are disordered, with 3 sets of int declarations helping to give external disorder, and pct after `used' to give internal disorder. % % if (!req->oldptr) % return SYSCTL_OUT(req, 0, 4 * sizeof(int)); The magic is 4 better placed in an array with that many elements; return this array. % % n_nchash = nchash + 1; /* nchash is max index, not count */ % used = 0; % maxlength = 0; Initializations are randomly sorted too. % % /* Scan hash tables for applicable entries */ % for (ncpp = nchashtbl; n_nchash > 0; n_nchash--, ncpp++) { % count = 0; % CACHE_RLOCK(); The order of the last 2 statements is gratuitously different than in the previous sysctl. This order is better. % LIST_FOREACH(ncp, ncpp, nc_hash) { % count++; % } % CACHE_RUNLOCK(); The previous sysctl has to unlock to avoid a LOR when it copies out the data. This fine-grained locking seems to just waste time here -- I think all chains can be scanned fast enough. If not, then unlocking every 128'th or similar chain would reduce the lock flapping. The previous sysctl could use an array with 128 elements or similar to reduce the number of copyouts and/or the lock flapping. Moving the locking outside of the loop also gives a non-gratuitously different order for initializing `count'. % if (count) % used++; % if (maxlength < count) % maxlength = count; % } % n_nchash = nchash + 1; % pct = (used * 100 * 100) / n_nchash; Putting these variables in the array takes also reduces style bugs automatically. It loses their descriptive names -- e.g., pct would become array[3] (the 4th and last element copied out). % error = SYSCTL_OUT(req, &n_nchash, sizeof(n_nchash)); % if (error) % return (error); % error = SYSCTL_OUT(req, &used, sizeof(used)); % if (error) % return (error); % error = SYSCTL_OUT(req, &maxlength, sizeof(maxlength)); % if (error) % return (error); % error = SYSCTL_OUT(req, &pct, sizeof(pct)); % if (error) % return (error); The last 9 lines go away using an array. % return (0); % } The comment on the NODE for these 2 sysctls says "Grab an atomic snapshot...", but the unlocking makes both non-atomic. Bruce