svn commit: r349791 - head/sys/vm

[Bruce Evans]

On Sat, 6 Jul 2019, Doug Moore wrote:

> Log:
>  Fix style(9) violations involving division by PAGE_SIZE.

It is style violation to even use an explicit division by PAGE_SIZE
instead of the btoc() conversion macro (*).

> Modified: head/sys/vm/swap_pager.c
> ==============================================================================
> --- head/sys/vm/swap_pager.c	Sat Jul  6 15:34:23 2019	(r349790)
> +++ head/sys/vm/swap_pager.c	Sat Jul  6 15:55:16 2019	(r349791)
> @@ -523,7 +523,7 @@ swap_pager_swap_init(void)
> 	 * but it isn't very efficient).
> 	 *
> 	 * The nsw_cluster_max is constrained by the bp->b_pages[]
> -	 * array (MAXPHYS/PAGE_SIZE) and our locally defined
> +	 * array MAXPHYS / PAGE_SIZE and our locally defined
> 	 * MAX_PAGEOUT_CLUSTER.   Also be aware that swap ops are
> 	 * constrained by the swap device interleave stripe size.
> 	 *

The macro is less readable in comments.

> @@ -538,7 +538,7 @@ swap_pager_swap_init(void)
> 	 * have one NFS swap device due to the command/ack latency over NFS.
> 	 * So it all works out pretty well.
> 	 */
> -	nsw_cluster_max = min((MAXPHYS/PAGE_SIZE), MAX_PAGEOUT_CLUSTER);
> +	nsw_cluster_max = min(MAXPHYS / PAGE_SIZE, MAX_PAGEOUT_CLUSTER);
>
> 	nsw_wcount_async = 4;
> 	nsw_wcount_async_max = nsw_wcount_async;
>
> Modified: head/sys/vm/vnode_pager.c
> ==============================================================================
> --- head/sys/vm/vnode_pager.c	Sat Jul  6 15:34:23 2019	(r349790)
> +++ head/sys/vm/vnode_pager.c	Sat Jul  6 15:55:16 2019	(r349791)
> @@ -544,8 +544,8 @@ vnode_pager_addr(struct vnode *vp, vm_ooffset_t addres
> 			*rtaddress += voffset / DEV_BSIZE;
> ...

Using explicit division by DEV_BSIZE instead of the btodb() conversion
macro is another style bug.

(*) The macros use shifts while the divisions use division.  The hard-coded
versions could use shifts too, so there is another set of style bugs from
only using shifts sometimes.  Shifts are faster if the type of the dividend
is signed.

Oops.  The macro also rounds up.  So hard-coding the division is not just
a style bug.  It is wrong unless the dividend is a multiple of the page
size.  This shouldn't be assumed for values like MAXBSIZE.

There are many more style bugs involving btoc():
- 'c' in it means 'click', which might mean a virtual page size while
   PAGE_SIZE might mean the physical page size.  Or vice versa.  dyson
   retired from BSD not long after I asked him to clean this up 20+
   years ago.
- btoc() is the only clearly MI macro for this.  The better-named macros
   amd64_ptob() and i386_ptob() are of course MD.  amd64_ptob() is never
   used in sys/amd64.  i386_ptob() is used 8 times in sys/i386 (all in
   pmap.c), and 1 of these uses is in a macro which is used 22 times.
   These uses give another set of style bugs.  They are just obfuscations
   if clicks are the same as pages, and probably incomplete otherwise.
   However, it is correct for MD code to use physical pages unless it is
   implementing virtual pages.

   These macros don't round up, so they are not equivalent to btoc() even
   if the click size is PAGE_SIZE.
- there is also the better-named macro atop().  This doesn't round up.
   I think it is supposed to be MI.  It is replicated <machine/param.h>
   for all arches.  'a' in it means 'address', which is less general than
   'b' for 'byte, so it is a worse name than btop().
- the macro with the best name btop() doesn't exist.

In the opposite direction, there are ctob(), {amd64,i386}_ptob(),
ptoa(), and direct multiplications by PAGE_SIZE and direct shifts by
PAGE_SHIFT.  {amd64,i386}_ptob() is not used even on i386.  This direction
is trickier since the (npages << PAGE_SHIFT) overflows if npages has the
correct type (int).  The caller should convert npages to something like
vm_offset_t before using the macro and the need for this is less obvious
than for a direct expression.  This is of course undocumented except by
the source code which shows:
- ctob(), ptoa() and i386_ptob() need conversion in the caller, but
   amd64_ptob() converts to unsigned long in the macro.  Since the last
   macro is unused, the caller should convert in all used cases.  Coversion
   is most important on 64-bit arches.  On i386, overflow occurs at 2G
   starting with int npages but u_int npages works up to 4G which is
   enough for i386 addresses but not for amd64 addresses or i386-PAE
   byte counts.

I once sprinkled conversions in the macros.  btodb() still uses my old
code for this, where the code is sophisticated so as to avoid using
the long long abomination, but which became mostly nonsense when daddr_t
was expanded from 32 bits to 64.  Since this macro shifts right,
conversion is not really necessary, and conversion to daddr_t gives
much the same pessimations as conversion to the abomination.  dbtodb()
simply converts to daddr_t.  I forget if I wrote that.  btoc() converts
to vm_offset_t, but since it shifts right conversion is not really
necessary.

Conversions in macros are a wrong way to do this.  jake taught me this
in connection with sparc64 i386-PAE work.  Conversion in the caller
allows the caller to control the type of the result and to not pessimize
by expanding the type.  I think jake intentionally left out conversions
in the sparc64 macros but didn't change the x86 macros much.  Other arches
use a random mixture.  E.g., ptoa() converts to unsigned long on amd64,
arm64, riscv and sparc64 (so jake didn't change this?), to unsigned
on arm, and doesn't convert on i386, mips or powerpc.  Always converting
to vm_offset_t would be reasonable, but gives namespace problems.  It
is stupid that the MI <sys/param.h> uses vm_offset_t for btoc() where
conversion is not really needed, while <machine/param.h> is more careful
about namespace problems so it avoids using even u_long for ptoa().

Bruce