svn commit: r232156 - head/sys/kern

Bruce Evans brde at optusnet.com.au
Sat Feb 25 23:33:29 UTC 2012


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


More information about the svn-src-head mailing list