Date: Mon, 28 Oct 2013 03:07:45 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r257199 - in head/sys/arm: allwinner arm broadcom/bcm2835 freescale/imx include lpc mv mv/orion rockchip sa11x0 samsung/exynos tegra ti ti/omap4/pandaboard versatile xilinx xscale/i8032... Message-ID: <20131028015840.V929@besplex.bde.org> In-Reply-To: <201310270051.r9R0pl3j024025@svn.freebsd.org> References: <201310270051.r9R0pl3j024025@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 27 Oct 2013, Ian Lepore wrote:
> Log:
> Remove all #include <machine/pmap.h> from arm code. It's already
> included by vm/pmap.h, which is a prerequisite for arm/machine/pmap.h
> so there's no reason to ever include it directly.
>
> Thanks to alc@ for pointing this out.
I noticed this style bug in the recent commit, but didn't complain before.
Including machine/pmap.h directly is unwarranted chumminess with the
implementation. When vm/pmap.h is also included, it less than that.
The chumminess is merely unwarranted in just a few cases. vm/map.h
is not includable in some asm files. machine/pmap.h is includable in
asm files, but shouldn't be on some arches. genassym should be used.
The hack of including machine/pmap.h is currently used in 5 asm files
(essentially the same 2: 2 on i386 cloned to amd64 and 1 of these
cloned to xen/i386). All file counts are for /sys.
In 2004, there were:
- 2 .c files that include only machine/pmap.h (chummy)
- 20 .c files that include vm/pmap.h and machine/pmap.h (less than that)
- 231 .c files that include vm/pmap.h only (correct)
The style bugs have expanded exponentially since then of course :-(. In
-current before this comment, there are:
- 9 .c files that include only machine/pmap.h (chummy)
- 80 .c files that include vm/pmap.h and machine/pmap.h (less than that)
- 421 .c files that include vm/pmap.h only (correct)
The files with mere unwarranted chumminess are 2 in sparc64 (in 2004 and
now), 6 in mips (now) and 1 in powerpc (now). MD files can reasonably
use only the <machine> level, but there is no reason to avoid the vm
level for just pmap. The 9 files that avoid vm for pmap use vm includes
for most other things.
These file counts by grep don't show the complexities from nested pollution.
machine/pmap.h is now included nested in 7 headers. This is pollution
except in vm/vmap.h. vm/pmap.h is now included in 32 headers. This is
gross pollution in vm/vm_page.h. It is perhaps needed in sys/user.h,
where it is marked as a mistake by XXX. The others are mostly for drivers
doing bad things like using vtophys().
Similarly for many other nested includes, except it is usually not so
clear that the machine includes should only be included directly in 1
place.
> Modified: head/sys/arm/allwinner/a10_machdep.c
> ==============================================================================
> --- head/sys/arm/allwinner/a10_machdep.c Sat Oct 26 23:41:11 2013 (r257198)
> +++ head/sys/arm/allwinner/a10_machdep.c Sun Oct 27 00:51:46 2013 (r257199)
> @@ -45,7 +45,6 @@ __FBSDID("$FreeBSD$");
> #include <machine/bus.h>
> #include <machine/frame.h> /* For trapframe_t, used in <machine/machdep.h> */
> #include <machine/machdep.h>
> -#include <machine/pmap.h>
>
> #include <dev/fdt/fdt_common.h>
Some other style bugs are visible in the patch.
Saying what an include is for is a bogus "come from" comment. Such comments
are usually rotten, and the above is no exception. trapframe_t is a
style bug (an "opaque" type that must be visible to use it) that still
exists (only arm has it, at least now). But arm/machine/machdep.h doesn't
use it now. I don't know if machine/machdep.h needed to use it. But the
optimization of passing full trap frames without copying them has been lost
because it depended on undefined behaviour in C. Everything now passes
pointers to trap frames instead. arm/machine/machdep.h spells the pointers
correctly as "struct trapframe *" after forward-declaring an incomplete
"struct trapframe", so it doesn't actually use trapframe_t. Perhaps the
include is now included for some unclaimed reason.
arm spells "struct trapframe" correctly in 45 places. It uses trapframe_t
in 29 places. 6 of these are for the same false "come from" comment.
machine/frame.h is still included nested in too many headers. arm is not
worse than i386 here.
> Modified: head/sys/arm/arm/minidump_machdep.c
> ==============================================================================
> --- head/sys/arm/arm/minidump_machdep.c Sat Oct 26 23:41:11 2013 (r257198)
> +++ head/sys/arm/arm/minidump_machdep.c Sun Oct 27 00:51:46 2013 (r257199)
> @@ -43,7 +43,6 @@ __FBSDID("$FreeBSD$");
> #endif
> #include <vm/vm.h>
> #include <vm/pmap.h>
> -#include <machine/pmap.h>
> #include <machine/atomic.h>
> #include <machine/elf.h>
> #include <machine/md_var.h>
Missing blank line before the <machine> includes. There order is improved.
> Modified: head/sys/arm/arm/nexus.c
> ==============================================================================
> --- head/sys/arm/arm/nexus.c Sat Oct 26 23:41:11 2013 (r257198)
> +++ head/sys/arm/arm/nexus.c Sun Oct 27 00:51:46 2013 (r257199)
> @@ -56,7 +56,6 @@ __FBSDID("$FreeBSD$");
> #include <machine/pcb.h>
> #include <vm/vm.h>
> #include <vm/pmap.h>
> -#include <machine/pmap.h>
>
> #include <machine/resource.h>
> #include <machine/intr.h>
This also fixes the misplaced blank line. The order of the 2 <machine>
includes visible is still backwards.
> --- head/sys/arm/freescale/imx/imx_machdep.c Sat Oct 26 23:41:11 2013 (r257198)
> +++ head/sys/arm/freescale/imx/imx_machdep.c Sun Oct 27 00:51:46 2013 (r257199)
> @@ -41,7 +41,6 @@ __FBSDID("$FreeBSD$");
> #include <machine/bus.h>
> #include <machine/frame.h> /* For trapframe_t, used in <machine/machdep.h> */
> #include <machine/machdep.h>
> -#include <machine/pmap.h>
>
> #include <arm/freescale/imx/imx_machdep.h>
> #include <arm/freescale/imx/imx_wdogreg.h>
The above count of 6 of these false "come from" comments is for a ~Sep 10
version of -current. There are now 10 of them visible in this patch alone.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131028015840.V929>