svn commit: r256295 - in projects/camlock/sys: geom kern

[Justin T. Gibbs]

On Oct 10, 2013, at 2:03 PM, Alexander Motin <mav at FreeBSD.org> wrote:

> Author: mav
> Date: Thu Oct 10 20:03:54 2013
> New Revision: 256295
> URL: http://svnweb.freebsd.org/changeset/base/256295
> 
> Log:
>  Use the same satistics for disk and its GEOM provider.
> 
>  Avoiding double accounting allows to reduce CPU load on I/O, especially on
>  machines with slow timecounter.  As side effect it also makes gstat show
>  statistics for raw disk providers even when kern.geom.collectstats is set
>  to zero, making that mode more usable.

…

> Modified: projects/camlock/sys/geom/geom_io.c
> ==============================================================================
> --- projects/camlock/sys/geom/geom_io.c	Thu Oct 10 19:56:31 2013	(r256294)
> +++ projects/camlock/sys/geom/geom_io.c	Thu Oct 10 20:03:54 2013	(r256295)
> @@ -510,7 +510,7 @@ g_io_request(struct bio *bp, struct g_co
> 
> 	KASSERT(!(bp->bio_flags & BIO_ONQUEUE),
> 	    ("Bio already on queue bp=%p", bp));
> -	if (g_collectstats)
> +	if ((g_collectstats & ~(pp->stat ? 0 : 1)) != 0)
> 		binuptime(&bp->bio_t0);
> 	else
> 		getbinuptime(&bp->bio_t0);


This is pretty obscure and further compounds the original bug that
there is no enum defining the bits in g_collectstats.  Can you
please:

1) Add an enum with constants for the two bits in g_collectstats
   with nice comments for what they do.

2) Use those constants everywhere and change your original logic to make it
   more readable?

e.g.

	if ((g_collecstats & G_CONSUMER_STATS) != 0 ||
	    ((g_collecstats & G_PROVIDER_STATS) != 0 && pp->stat != NULL))

I would bet the compiler optimizes this just as well as your original logic.

Thanks,
Justin