From owner-svn-src-head@freebsd.org Sat Jul 6 17:49:13 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 17F6415CDD7A; Sat, 6 Jul 2019 17:49:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 526A76EC92; Sat, 6 Jul 2019 17:49:11 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 0E57D43A27F; Sun, 7 Jul 2019 03:48:59 +1000 (AEST) Date: Sun, 7 Jul 2019 03:48:54 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Doug Moore cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349791 - head/sys/vm In-Reply-To: <201907061555.x66FtGsg025314@repo.freebsd.org> Message-ID: <20190707023441.B2047@besplex.bde.org> References: <201907061555.x66FtGsg025314@repo.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.2 cv=P6RKvmIu c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=k1N1_3MYVckZGDfFxwYA:9 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 526A76EC92 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.90 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.90)[-0.896,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Jul 2019 17:49:13 -0000 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 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 uses vm_offset_t for btoc() where conversion is not really needed, while is more careful about namespace problems so it avoids using even u_long for ptoa(). Bruce